Allow notification origin permission request when no js callback is provided
Created attachment 99061 [details] Patch
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.
Created attachment 99448 [details] Patch Added ChangeLog entry
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.
Ping Alex?
Created attachment 108807 [details] Patch
Comment on attachment 108807 [details] Patch Why no test case?
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.
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 !
Created attachment 112681 [details] Patch
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.
(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.
(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.
*** Bug 74433 has been marked as a duplicate of this bug. ***
(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.
<rdar://problem/11059590>
<rdar://problem/11061334>
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 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.
Created attachment 164947 [details] Patch
ping? still needs review.
Comment on attachment 164947 [details] Patch Clearing flags on attachment: 164947 Committed r131280: <http://trac.webkit.org/changeset/131280>
All reviewed patches have been landed. Closing bug.