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.
Created attachment 88069 [details] Patch
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().
Created attachment 88086 [details] Patch
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.
(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 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.'
Created attachment 88210 [details] Patch
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 on attachment 88210 [details] Patch r=me
Comment on attachment 88210 [details] Patch Clearing flags on attachment: 88210 Committed r82940: <http://trac.webkit.org/changeset/82940>
All reviewed patches have been landed. Closing bug.