Bug 108196 – Notification.requestPermission callback should be optional

• Component: DOM
• Status: RESOLVED FIXED

[Jake Archibald]

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

[Kaustubh Atrawalkar]

As per the discussion, I agree with making requestPermssion optional. Providing patch to make the proposed changes.

[Kaustubh Atrawalkar]

Created attachment 189847 [details]
Patch

[Adam Barth]

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

[Adam Barth]

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.

[Kaustubh Atrawalkar]

(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.

[Adam Barth]

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.

[Adam Barth]

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.

[Kaustubh Atrawalkar]

Created attachment 190156 [details]
Patch

[Kaustubh Atrawalkar]

(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?

[Adam Barth]

Comment on attachment 190156 [details]
Patch

this looks much better

[Early Warning System Bot]

Comment on attachment 190156 [details]
Patch

Attachment 190156 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16756535

[Kaustubh Atrawalkar]

(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.

[Early Warning System Bot]

Comment on attachment 190156 [details]
Patch

Attachment 190156 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16744612

[Build Bot]

Comment on attachment 190156 [details]
Patch

Attachment 190156 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16743632

[Build Bot]

Comment on attachment 190156 [details]
Patch

Attachment 190156 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16746642

[WebKit Review Bot]

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

[Kaustubh Atrawalkar]

Created attachment 190382 [details]
Patch

[Kentaro Hara]

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>

[Kaustubh Atrawalkar]

(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.

[Kaustubh Atrawalkar]

Committed r144126: <http://trac.webkit.org/changeset/144126>

[Stephen Chenney]

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.

[Stephen Chenney]

Why on earth can't I reopen this bug?

[Adam Barth]

Reopening

[Adam Barth]

Hum...   Seems to work for me...

[Adam Barth]

Stephen, I just ran a script to give you canconfirm and editbugs.  You probably already had them, however.  :)

[Vsevolod Vlasov]

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

[Thiago Marcos P. Santos]

Just a nit, remember to fix the bindings generator tests that is failing after this patch.

[Kaustubh Atrawalkar]

I will try to fix these asap. Thank you.

[Stephen Chenney]

I fixed the bindings generator test in http://trac.webkit.org/changeset/144176

[Kaustubh Atrawalkar]

Created attachment 190608 [details]
Patch

[Early Warning System Bot]

Comment on attachment 190608 [details]
Patch

Attachment 190608 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16758269

[Kaustubh Atrawalkar]

Thats what i was trying to find the JS Custom bindings file for NotificationCenter. It was with different name. Adding new patch.

[Kaustubh Atrawalkar]

Created attachment 190622 [details]
Patch

[Zoltan Arvai]

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

[Adam Barth]

If this patch is causing crashes, we should roll it out.

[Kaustubh Atrawalkar]

Created attachment 190786 [details]
Patch

[Kaustubh Atrawalkar]

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.

[Kentaro Hara]

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?

[Kaustubh Atrawalkar]

Yes that's correct. I have merged both patches together. Can you review?

[Kentaro Hara]

Comment on attachment 190786 [details]
Patch

LGTM. Please keep watching the build tree when landing.

[Kaustubh Atrawalkar]

(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.

[WebKit Review Bot]

Comment on attachment 190786 [details]
Patch

Clearing flags on attachment: 190786

Committed r144376: <http://trac.webkit.org/changeset/144376>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kaustubh Atrawalkar]

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