[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]

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.