Bug 57760 - Factoring the creation of 'FunctionOnly' callbacks in V8
Summary: Factoring the creation of 'FunctionOnly' callbacks in V8
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Leandro Graciá Gil
URL:
Keywords:
Depends on:
Blocks: 56586 57963
  Show dependency treegraph
 
Reported: 2011-04-04 09:37 PDT by Leandro Graciá Gil
Modified: 2012-09-19 07:17 PDT (History)
2 users (show)

See Also:


Attachments
Patch (6.18 KB, patch)
2011-04-04 09:46 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (6.69 KB, patch)
2011-04-04 11:39 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff
Patch (6.67 KB, patch)
2011-04-05 05:50 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Leandro Graciá Gil 2011-04-04 09:37:36 PDT
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.
Comment 1 Leandro Graciá Gil 2011-04-04 09:46:07 PDT
Created attachment 88069 [details]
Patch
Comment 2 Steve Block 2011-04-04 09:54:58 PDT
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().
Comment 3 Leandro Graciá Gil 2011-04-04 11:39:42 PDT
Created attachment 88086 [details]
Patch
Comment 4 Leandro Graciá Gil 2011-04-04 11:40:03 PDT
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.
Comment 5 Leandro Graciá Gil 2011-04-04 11:40:33 PDT
(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
Comment 6 Steve Block 2011-04-05 02:57:53 PDT
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.'
Comment 7 Leandro Graciá Gil 2011-04-05 05:50:58 PDT
Created attachment 88210 [details]
Patch
Comment 8 Leandro Graciá Gil 2011-04-05 05:51:26 PDT
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.
Comment 9 Steve Block 2011-04-05 05:54:32 PDT
Comment on attachment 88210 [details]
Patch

r=me
Comment 10 WebKit Commit Bot 2011-04-05 08:10:35 PDT
Comment on attachment 88210 [details]
Patch

Clearing flags on attachment: 88210

Committed r82940: <http://trac.webkit.org/changeset/82940>
Comment 11 WebKit Commit Bot 2011-04-05 08:10:41 PDT
All reviewed patches have been landed.  Closing bug.