[webkit-reviews] review denied: [Bug 63615] Allow notification origin permission request when no js callback is provided : [Attachment 108807] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Oct 17 12:08:34 PDT 2011
Darin Adler <darin at apple.com> has denied Alexandre Mazari <scaroo at gmail.com>'s
request for review:
Bug 63615: Allow notification origin permission request when no js callback is
provided
https://bugs.webkit.org/show_bug.cgi?id=63615
Attachment 108807: Patch
https://bugs.webkit.org/attachment.cgi?id=108807&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=108807&action=review
review- because of the lack of a test case
> Source/WebCore/bindings/js/JSDesktopNotificationsCustom.cpp:68
> + if (exec->argument(0).isObject()) {
> + JSDOMGlobalObject* globalObject =
toJSDOMGlobalObject(static_cast<Document*>(context), exec);
> + RefPtr<JSCustomVoidCallback> callback =
JSCustomVoidCallback::create(exec->argument(0).getObject(), globalObject);
>
> - PassRefPtr<JSCustomVoidCallback> callback =
JSCustomVoidCallback::create(exec->argument(0).getObject(),
toJSDOMGlobalObject(static_cast<Document*>(context), exec));
> + impl()->requestPermission(callback.release());
> + } else
> + impl()->requestPermission(0);
It was good that you fixed the mistake where there was an local variable of
type PassRefPtr.
But I’m not sure why you added a local variable for the global object. I
personally don’t think it enhances readability.
Also, if we declared the callback local variable outside of the if statement we
would not need an else.
More information about the webkit-reviews
mailing list