Bug 107637

Created attachment 184159 [details] Patch

Attachment 184159 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/We..." exit_code: 1 Source/WTF/wtf/chromium/MainThreadChromium.cpp:60: Extra space before ( in function call [whitespace/parens] [4] Source/WTF/wtf/chromium/MainThreadChromium.cpp:65: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 184172 [details] Patch

Attachment 184172 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Fu..." exit_code: 1 Source/WTF/wtf/chromium/MainThreadChromium.cpp:60: Extra space before ( in function call [whitespace/parens] [4] Source/WTF/wtf/chromium/MainThreadChromium.cpp:65: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 184172 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184172&action=review > Source/WTF/wtf/Functional.h:180 > + if (!c) > + return R(); I don't understand this part. > Source/WTF/wtf/Functional.h:346 > +template<typename T, bool shouldValidate> struct ParamStorageTraits { Booooo. bool params are ugly. > Source/WTF/wtf/Functional.h:367 > +template<typename T> struct ParamStorageTraits<WeakPtr<T>, false> { See! Confusing bools! > Source/WTF/wtf/Functional.h:484 > + , m_p2(ParamStorageTraits<P2, false>::wrap(p2)) Oh the bools! > Source/WTF/wtf/WeakPtr.h:104 > + T* ptr = m_ref->get(); > + m_ref->clear(); > + // We create a new WeakReference so that future calls to createWeakPtr() create nonzero WeakPtrs. > + m_ref = Internal::WeakReference<T>::create(ptr); Can ptr go stale during this? >> Source/WTF/wtf/chromium/MainThreadChromium.cpp:60 >> + Function<void ()>* function = static_cast<Function<void ()>*>(context); > > Extra space before ( in function call [whitespace/parens] [4] I'm not sure the space helps readability and just seems to make the stylebot mad. :) We also need a global typedef for Function<void()>, it's very very common. There is currently one in HTMLBackgroundThread called Closure (like chromium).

The parser-side changes LGTM. I'm sure Anders would like to see the Functional bits go by. Maybe he has naming opinions on a "Function<void()>" (which we desperately need).

I agree that the boolean parameters are ugly - maybe we could add special member functions for wrapping and unwrapping/validating the first parameter instead?

(In reply to comment #5) > (From update of attachment 184172 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184172&action=review > > > Source/WTF/wtf/Functional.h:180 > > + if (!c) > > + return R(); > > I don't understand this part. That's the part that checks whether the WeakPtr has been cleared before the task runs. If "c" (which is the |this| pointer) is null, we don't call the member function. (Calling member functions on a null |this| pointer is undefined in C++ anyway.) > > Source/WTF/wtf/Functional.h:346 > > +template<typename T, bool shouldValidate> struct ParamStorageTraits { > > Booooo. bool params are ugly. I can try to use enums, but I'm not sure if that will freak out the template system. > > Source/WTF/wtf/WeakPtr.h:104 > > + T* ptr = m_ref->get(); > > + m_ref->clear(); > > + // We create a new WeakReference so that future calls to createWeakPtr() create nonzero WeakPtrs. > > + m_ref = Internal::WeakReference<T>::create(ptr); > > Can ptr go stale during this? Nope. Both get() and clear() ASSERT that they're called on a given thread (the "bound thread"). > >> Source/WTF/wtf/chromium/MainThreadChromium.cpp:60 > >> + Function<void ()>* function = static_cast<Function<void ()>*>(context); > > > > Extra space before ( in function call [whitespace/parens] [4] > > I'm not sure the space helps readability and just seems to make the stylebot mad. :) Ok. I can remove it. > We also need a global typedef for Function<void()>, it's very very common. There is currently one in HTMLBackgroundThread called Closure (like chromium). Perhaps "thunk"? :)

> I agree that the boolean parameters are ugly - maybe we could add special member functions for wrapping and unwrapping/validating the first parameter instead? The problem is that you want to use a different unwrapping function (with a different return type) when the argument is the |this| parameter to a member function than when the first argument to a non-member function.

Comment on attachment 184172 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184172&action=review >>> Source/WTF/wtf/WeakPtr.h:104 >>> + m_ref = Internal::WeakReference<T>::create(ptr); >> >> Can ptr go stale during this? > > Nope. Both get() and clear() ASSERT that they're called on a given thread (the "bound thread"). I was wondering more if the cleared WeakReference was the last ref to the object, would it now be deleted. :)

(In reply to comment #10) > (From update of attachment 184172 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184172&action=review > > >>> Source/WTF/wtf/WeakPtr.h:104 > >>> + m_ref = Internal::WeakReference<T>::create(ptr); > >> > >> Can ptr go stale during this? > > > > Nope. Both get() and clear() ASSERT that they're called on a given thread (the "bound thread"). > > I was wondering more if the cleared WeakReference was the last ref to the object, would it now be deleted. :) WeakReference does not prevent the object from being deleted.

Created attachment 184306 [details] Patch

Comment on attachment 184306 [details] Patch Looks great.

Created attachment 184308 [details] Patch

I'll land manually to avoid the sln issue.

Committed r140589: <http://trac.webkit.org/changeset/140589>