Bug 63615 – Allow notification origin permission request when no js callback is provided

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Alexandre Mazari]

Allow notification origin permission request when no js callback is provided

[Alexandre Mazari]

Created attachment 99061 [details]
Patch

[Alexandre Mazari]

The JavasCript NotificationCenter::requestPermission can * optionally* take a callback as argument. If one is provided, it should be called in the case the request was taken into account.

The current implementation requires such an argument in the JSC wrapper for NotificationCenter where the V8 wrapper makes it optional.

This patch implements the same behaviour for JSC.

[Alexandre Mazari]

Created attachment 99448 [details]
Patch

Added ChangeLog entry

[Eric Seidel (no email)]

Comment on attachment 99448 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99448&action=review

We need a test.

> Source/WebCore/bindings/js/JSDesktopNotificationsCustom.cpp:63
> +        RefPtr<JSCustomVoidCallback> callback = JSCustomVoidCallback::create(exec->argument(0).getObject(), toJSDOMGlobalObject(static_cast<Document*>(context), exec));

I mgiht have used a local object for the global object to make this line more readable, but this is also OK.

> Source/WebCore/bindings/js/JSDesktopNotificationsCustom.cpp:66
> +      impl()->requestPermission(0);

Indent.  Sad that check-webkit-style doesn't notice.

[Philippe Normand]

Ping Alex?

[Alexandre Mazari]

Created attachment 108807 [details]
Patch

[Darin Adler]

Comment on attachment 108807 [details]
Patch

Why no test case?

[Darin Adler]

Comment on attachment 108807 [details]
Patch

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.

[Alexandre Mazari]

Eric, Darin, Phillipe,

thanks for your reviews and pardon my late answer, I was dragged out by other stuffs.

I did not include a specific test for that patch because checkPermission is already called without argument within the existing 'fast/notifications/notifications-document-close-crash.html' test.
Is a specific test case required, then ?


eric: "I mgiht have used a local object for the global object to make this line more readable, but this is also OK."
darin: "But I’m not sure why you added a local variable for the global object. I personally don’t think it enhances readability."

Little puzzled here :)


darin: "Also, if we declared the callback local variable outside of the if statement we would not need an else."

I remember trying that, but compiler complained of a re-affectation of the RefPtr. Would something like the following match your comment:

JSCustomVoidCallback* callback = 0;
if (exec->argument(0).isObject()) {
   callback = JSCustomVoidCallback::create(exec->argument(0).getObject(),
			toJSDOMGlobalObject(static_cast<Document*>(context), exec)).release();
}
impl()->requestPermission(callback);

Again, thanks for having a look at it !

[Alexandre Mazari]

Created attachment 112681 [details]
Patch

[Darin Adler]

Comment on attachment 112681 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112681&action=review

Where is the test case for this?

> Source/WebCore/bindings/js/JSDesktopNotificationsCustom.cpp:62
> +    // if a callback function is provided as first argument, convert the object

The word if should be capitalized here.

[Darin Adler]

(In reply to comment #9)
> I did not include a specific test for that patch because checkPermission is already called without argument within the existing 'fast/notifications/notifications-document-close-crash.html' test.
> Is a specific test case required, then ?

Does the test case result change? If so, then the patch needs the new expected results. If not, then we need a new test case that shows the old bad behavior being replaced with new good behavior.

[Alexandre Mazari]

(In reply to comment #12)
> (In reply to comment #9)
> > I did not include a specific test for that patch because checkPermission is already called without argument within the existing 'fast/notifications/notifications-document-close-crash.html' test.
> > Is a specific test case required, then ?
> 
> Does the test case result change? 
Yep for the GTK port after applying #61140, that add html5 notifications support to the port, and #66477, that introduce the testing facilities.

> If so, then the patch needs the new expected results.

The effect of this patch is only perciptible after apply the two mentionned patches. Should I fold this one with them ?
I isolated it because it is not port specific, so I am unsure about the eventuality.

[Jon Lee]

*** Bug 74433 has been marked as a duplicate of this bug. ***

[Jon Lee]

(In reply to comment #14)
> *** Bug 74433 has been marked as a duplicate of this bug. ***

Could we also add the Optional parameter to IDL? Since this is custom, I assume adding that attribute does no harm, and it also makes it clearer what the expectation for that parameter is.

I think we should also have a different test, to make sure that an exception isn't thrown when we make the request with no callback.

[Jon Lee]

<rdar://problem/11059590>

[Radar WebKit Bug Importer]

<rdar://problem/11061334>

[Jon Lee]

This bug apparently affects gmail.com's settings. When you go to the settings panel and click on the blue link "Click here to enable desktop notifications" it throws an exception because it makes this kind of call.

Oddly, when it auto-detects that desktop notifications are available, and show you a yellow banner on the main gmail screen, that link uses the currently supported form.

[Darin Adler]

Comment on attachment 112681 [details]
Patch

Code change looks good. We also need a regression test case to go with it. Or an explanation of why a regression test is not possible.

[Jon Lee]

Created attachment 164947 [details]
Patch

[Jon Lee]

ping? still needs review.

[Jon Lee]

Comment on attachment 164947 [details]
Patch

Clearing flags on attachment: 164947

Committed r131280: <http://trac.webkit.org/changeset/131280>

[Jon Lee]

All reviewed patches have been landed.  Closing bug.