Bug 63615

Summary: Allow notification origin permission request when no js callback is provided
Product: WebKit Reporter: Alexandre Mazari <scaroo>
Component: WebCore JavaScriptAssignee: Jon Lee <jonlee>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, eric, jonlee, laszlo.gombos, mrobinson, pnormand, scaroo, sra, webkit-bug-importer, xan.lopez
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 61140    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Alexandre Mazari 2011-06-29 02:51:14 PDT
Allow notification origin permission request when no js callback is provided
Comment 1 Alexandre Mazari 2011-06-29 02:52:17 PDT
Created attachment 99061 [details]
Patch
Comment 2 Alexandre Mazari 2011-07-01 01:16:05 PDT
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.
Comment 3 Alexandre Mazari 2011-07-01 02:55:18 PDT
Created attachment 99448 [details]
Patch

Added ChangeLog entry
Comment 4 Eric Seidel (no email) 2011-09-12 15:58:20 PDT
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.
Comment 5 Philippe Normand 2011-09-23 06:13:58 PDT
Ping Alex?
Comment 6 Alexandre Mazari 2011-09-27 01:06:31 PDT
Created attachment 108807 [details]
Patch
Comment 7 Darin Adler 2011-10-17 12:06:10 PDT
Comment on attachment 108807 [details]
Patch

Why no test case?
Comment 8 Darin Adler 2011-10-17 12:08:34 PDT
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.
Comment 9 Alexandre Mazari 2011-10-25 02:53:38 PDT
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 !
Comment 10 Alexandre Mazari 2011-10-27 07:54:01 PDT
Created attachment 112681 [details]
Patch
Comment 11 Darin Adler 2011-10-27 10:06:57 PDT
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.
Comment 12 Darin Adler 2011-10-27 10:07:57 PDT
(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.
Comment 13 Alexandre Mazari 2011-11-10 06:47:48 PST
(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.
Comment 14 Jon Lee 2011-12-15 15:00:45 PST
*** Bug 74433 has been marked as a duplicate of this bug. ***
Comment 15 Jon Lee 2011-12-15 15:01:59 PST
(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.
Comment 16 Jon Lee 2012-03-15 21:51:56 PDT
<rdar://problem/11059590>
Comment 17 Radar WebKit Bug Importer 2012-03-15 21:52:23 PDT
<rdar://problem/11061334>
Comment 18 Jon Lee 2012-08-03 10:23:45 PDT
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.
Comment 19 Darin Adler 2012-08-14 12:49:42 PDT
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.
Comment 20 Jon Lee 2012-09-20 11:01:37 PDT
Created attachment 164947 [details]
Patch
Comment 21 Jon Lee 2012-10-08 15:53:50 PDT
ping? still needs review.
Comment 22 Jon Lee 2012-10-14 16:50:25 PDT
Comment on attachment 164947 [details]
Patch

Clearing flags on attachment: 164947

Committed r131280: <http://trac.webkit.org/changeset/131280>
Comment 23 Jon Lee 2012-10-14 16:50:28 PDT
All reviewed patches have been landed.  Closing bug.