Bug 57760 – Factoring the creation of 'FunctionOnly' callbacks in V8

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Leandro Graciá Gil]

This patch makes a template of the existing functions to create callbacks from arguments in V8GeolocationCustom.cpp and places it in V8Utilites. This method is going to be used in common by Geolocation, the Media Stream API and possibly more.

[Leandro Graciá Gil]

Created attachment 88069 [details]
Patch

[Steve Block]

Comment on attachment 88069 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88069&action=review

Do you have a bug to do the same for JSC?

> Source/WebCore/bindings/v8/V8Utilities.h:58
> +    void throwTypeMismatchException();

Now that the call-sites have been unified, there's probably no need for this to be a function - you can just call throwError() directly.

> Source/WebCore/bindings/v8/V8Utilities.h:61
> +    PassRefPtr<V8CallbackType> createFunctionOnlyCallback(v8::Local<v8::Value> value, bool& succeeded, bool allowUndefined, bool allowNull)

It's usual to have the out param last. Also, an enum would be much easier to read than a pair of bools for allowUndefined and allowNull.

> Source/WebCore/bindings/v8/custom/V8GeolocationCustom.cpp:-68
> -    // Argument is optional (hence undefined is allowed), and null is allowed.

Would be good to preserve this comment at the call-site of createFunctionOnlyCallback().

[Leandro Graciá Gil]

Created attachment 88086 [details]
Patch

[Leandro Graciá Gil]

Comment on attachment 88069 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88069&action=review

>> Source/WebCore/bindings/v8/V8Utilities.h:58
>> +    void throwTypeMismatchException();
> 
> Now that the call-sites have been unified, there's probably no need for this to be a function - you can just call throwError() directly.

Including V8Proxy.h in the header generates a dependency cycle. Keeping it in the cpp file avoids this.

>> Source/WebCore/bindings/v8/V8Utilities.h:61
>> +    PassRefPtr<V8CallbackType> createFunctionOnlyCallback(v8::Local<v8::Value> value, bool& succeeded, bool allowUndefined, bool allowNull)
> 
> It's usual to have the out param last. Also, an enum would be much easier to read than a pair of bools for allowUndefined and allowNull.

Fixed. Using bitmasks and a typedef to unsigned as argument type, mimicking UpdateLayerPositionsFlag in RenderLayer.h.

>> Source/WebCore/bindings/v8/custom/V8GeolocationCustom.cpp:-68
>> -    // Argument is optional (hence undefined is allowed), and null is allowed.
> 
> Would be good to preserve this comment at the call-site of createFunctionOnlyCallback().

Fixed.

[Leandro Graciá Gil]

(In reply to comment #2)
> (From update of attachment 88069 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88069&action=review
> 
> Do you have a bug to do the same for JSC?
> 

Yes. Here it is: https://bugs.webkit.org/show_bug.cgi?id=57770

[Steve Block]

Comment on attachment 88086 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88086&action=review

> Source/WebCore/bindings/v8/V8Utilities.h:61
> +        CallbackRequireFunction = 1,

Would 'CallbackAllowFunction' be a better name, to keep all three consistent, and to make clear what 'CallbackRequireFunction | CallbackAllowUndefined' means?

> Source/WebCore/bindings/v8/V8Utilities.h:77
> +        // 'FunctionOnly' is assumed for the created callback.

This comment seems a little odd. Maybe move it to above the method - 'Creates a callback of the templated type from a V8 Value which represents a function object. Values of null and undefined may be allowed, in which case the method succeeds but the callback is null.'

[Leandro Graciá Gil]

Created attachment 88210 [details]
Patch

[Leandro Graciá Gil]

Comment on attachment 88086 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88086&action=review

>> Source/WebCore/bindings/v8/V8Utilities.h:61
>> +        CallbackRequireFunction = 1,
> 
> Would 'CallbackAllowFunction' be a better name, to keep all three consistent, and to make clear what 'CallbackRequireFunction | CallbackAllowUndefined' means?

Fixed.

>> Source/WebCore/bindings/v8/V8Utilities.h:77
>> +        // 'FunctionOnly' is assumed for the created callback.
> 
> This comment seems a little odd. Maybe move it to above the method - 'Creates a callback of the templated type from a V8 Value which represents a function object. Values of null and undefined may be allowed, in which case the method succeeds but the callback is null.'

Fixed.

[Steve Block]

Comment on attachment 88210 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 88210 [details]
Patch

Clearing flags on attachment: 88210

Committed r82940: <http://trac.webkit.org/changeset/82940>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.