Bug 107637 – BackgroundHTMLParser::sendTokensToMainThread should use bind

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

BackgroundHTMLParser::sendTokensToMainThread should use bind

[Adam Barth]

Created attachment 184159 [details]
Patch

[WebKit Review Bot]

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.

[Adam Barth]

Created attachment 184172 [details]
Patch

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

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).

[Eric Seidel (no email)]

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).

[Anders Carlsson]

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

[Adam Barth]

(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"?  :)

[Adam Barth]

> 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.

[Eric Seidel (no email)]

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. :)

[Adam Barth]

(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.

[Adam Barth]

Created attachment 184306 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 184306 [details]
Patch

Looks great.

[Adam Barth]

Created attachment 184308 [details]
Patch

[Adam Barth]

I'll land manually to avoid the sln issue.

[Adam Barth]

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