Bug 57760

Summary: Factoring the creation of 'FunctionOnly' callbacks in V8
Product: WebKit Reporter: Leandro Graciá Gil <leandrogracia>
Component: WebCore Misc.Assignee: Leandro Graciá Gil <leandrogracia>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, steveblock
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 56586, 57963    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Leandro Graciá Gil
Reported 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.
Attachments
Patch (6.18 KB, patch)
2011-04-04 09:46 PDT, Leandro Graciá Gil 		no flags
DetailsFormatted DiffDiff
Patch (6.69 KB, patch)
2011-04-04 11:39 PDT, Leandro Graciá Gil 		no flags
DetailsFormatted DiffDiff
Patch (6.67 KB, patch)
2011-04-05 05:50 PDT, Leandro Graciá Gil 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Leandro Graciá Gil
Comment 1 2011-04-04 09:46:07 PDT
Created attachment 88069 [details] Patch
Steve Block
Comment 2 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().
Leandro Graciá Gil
Comment 3 2011-04-04 11:39:42 PDT
Created attachment 88086 [details] Patch
Leandro Graciá Gil
Comment 4 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.
Leandro Graciá Gil
Comment 5 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
Steve Block
Comment 6 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.'
Leandro Graciá Gil
Comment 7 2011-04-05 05:50:58 PDT
Created attachment 88210 [details] Patch
Leandro Graciá Gil
Comment 8 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.
Steve Block
Comment 9 2011-04-05 05:54:32 PDT
Comment on attachment 88210 [details] Patch r=me
WebKit Commit Bot
Comment 10 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>
WebKit Commit Bot
Comment 11 2011-04-05 08:10:41 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.