Currently, a DOM error is thrown if the callback is omitted. Spec: http://notifications.spec.whatwg.org/#api Spec change discussion: https://github.com/whatwg/notifications/pull/1
As per the discussion, I agree with making requestPermssion optional. Providing patch to make the proposed changes.
Created attachment 189847 [details] Patch
Comment on attachment 189847 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189847&action=review > Source/WebCore/Modules/notifications/Notification.idl:54 > - [Custom] static void requestPermission(in NotificationPermissionCallback callback); > + [Custom] static void requestPermission(in [Optional, Callback] NotificationPermissionCallback callback); Why does it need to be [Custom]? It looks like it just needs [CallWith=ScriptExecutionContext].
Comment on attachment 189847 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189847&action=review > LayoutTests/fast/notifications/notifications-request-permission-optional.html:18 > + shouldThrow('window.Notification.requestPermission()', notEnoughArguments); Doesn't this show that the argument is not optional? /me is confused.
(In reply to comment #3) > Why does it need to be [Custom]? It looks like it just needs [CallWith=ScriptExecutionContext]. Yes, seems fine to me as well. I just kept the semantics as it is and added optional. But changing it as suggested would be better. Will do that. (In reply to comment #4) > > LayoutTests/fast/notifications/notifications-request-permission-optional.html:18 > > + shouldThrow('window.Notification.requestPermission()', notEnoughArguments); > > Doesn't this show that the argument is not optional? /me is confused. Basically it will throw error if the argument is not optional. When requestPermission() is called without any arguments it should work as it is. If the compiler detects the argument is missing, it will throw error. in this case, as the argument is optional, it wont. I hope my understanding is right. please correct me if wrong.
Comment on attachment 189847 [details] Patch I'm, sorry, but I don't understand what you are saying. Your patch doesn't actually change any behavior because this function has Custom bindings. I believe your test demonstrates that the argument is not optional.
Comment on attachment 189847 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189847&action=review > LayoutTests/fast/notifications/notifications-request-permission-optional.html:9 > +function runTests() I don't think this function is even called.
Created attachment 190156 [details] Patch
(In reply to comment #6) > (From update of attachment 189847 [details]) > I'm, sorry, but I don't understand what you are saying. Your patch doesn't actually change any behavior because this function has Custom bindings. I believe your test demonstrates that the argument is not optional. I removed the Custom bindings and updated the idl file with [CallWith=ScriptExecutionContext]. Also updated the test case to make a call. Can you review once?
Comment on attachment 190156 [details] Patch this looks much better
Comment on attachment 190156 [details] Patch Attachment 190156 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16756535
(In reply to comment #10) > (From update of attachment 190156 [details]) > this looks much better Thank you for the quick review. This is still not final patch. Seems there are few build breaks. I will fix those and put up patch again.
Comment on attachment 190156 [details] Patch Attachment 190156 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16744612
Comment on attachment 190156 [details] Patch Attachment 190156 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16743632
Comment on attachment 190156 [details] Patch Attachment 190156 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16746642
Comment on attachment 190156 [details] Patch Attachment 190156 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16780353 New failing tests: fast/notifications/notifications-rtl.html
Created attachment 190382 [details] Patch
Comment on attachment 190382 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190382&action=review The change looks good. A couple of comments on the test. > LayoutTests/fast/notifications/notifications-request-permission-optional.html:9 > +function log(message) > +{ > + document.getElementById("result").innerHTML += message + "<br>"; > +} Nit: Remove this. > LayoutTests/fast/notifications/notifications-request-permission-optional.html:14 > + if (window.testRunner) > + testRunner.dumpAsText(); > + Nit: This is not needed. > LayoutTests/fast/notifications/notifications-request-permission-optional.html:16 > + if (!window.webkitNotifications) > + log("FAIL: No webkitNotification interface!"); You can write: shouldNotBe('!window.webkitNotifications'); > LayoutTests/fast/notifications/notifications-request-permission-optional.html:25 > +runTests(); Nit: Now the test is just two lines. You can directly write the test here. > LayoutTests/fast/notifications/notifications-request-permission-optional.html:27 > +</body> Please add <script src="../js/resources/js-test-post.js"></script>
(In reply to comment #18) > (From update of attachment 190382 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190382&action=review > > The change looks good. A couple of comments on the test. Thanks :) I made those change. Will be landing patch soon.
Committed r144126: <http://trac.webkit.org/changeset/144126>
Reopening. This is causing crashes in fast/notifications/notifications-request-permission.html http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fnotifications%2Fnotifications-request-permission.html Expectations added in r144134.
Why on earth can't I reopen this bug?
Reopening
Hum... Seems to work for me...
Stephen, I just ran a script to give you canconfirm and editbugs. You probably already had them, however. :)
Layout test fast/notifications/notifications-request-permission-optional.html is also crashing flakily. Filed https://bugs.webkit.org/show_bug.cgi?id=110956 and marked as crashing in http://trac.webkit.org/changeset/144153
Just a nit, remember to fix the bindings generator tests that is failing after this patch.
I will try to fix these asap. Thank you.
I fixed the bindings generator test in http://trac.webkit.org/changeset/144176
Created attachment 190608 [details] Patch
Comment on attachment 190608 [details] Patch Attachment 190608 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16758269
Thats what i was trying to find the JS Custom bindings file for NotificationCenter. It was with different name. Adding new patch.
Created attachment 190622 [details] Patch
On Qt fast/notifications/notifications-request-permission-optional.html test causes other test to crash: 02:19:50.168 16160 worker/0 fast/notifications/notifications-request-permission-optional.html passed 02:19:50.294 16160 worker/0 fast/notifications/notifications-rtl.html crashed, (stderr lines): http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release/builds/48658/steps/layout-test/logs/stdio
If this patch is causing crashes, we should roll it out.
Created attachment 190786 [details] Patch
Merged both patches together. It required NotificationCenter class as well to have requestPermission callback as optional. Removed both custom bindings from Notification.idl & NotificationCenter.idl. Please review once.
What is the current status? The patch that caused the crashes was rolled out, and now you are going to reland the patch with proper fixes, right?
Yes that's correct. I have merged both patches together. Can you review?
Comment on attachment 190786 [details] Patch LGTM. Please keep watching the build tree when landing.
(In reply to comment #40) > (From update of attachment 190786 [details]) > LGTM. Please keep watching the build tree when landing. Sure I will keep eye on build tree. Thanks.
Comment on attachment 190786 [details] Patch Clearing flags on attachment: 190786 Committed r144376: <http://trac.webkit.org/changeset/144376>
All reviewed patches have been landed. Closing bug.
*** Bug 110956 has been marked as a duplicate of this bug. ***