Bug 108009 – The notification constructor needs to check permissions synchronously

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

The notification constructor needs to check permissions synchronously

Created attachment 184877 [details]
Patch

Created attachment 184878 [details]
Patch

[Build Bot]

Comment on attachment 184878 [details]
Patch

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

[Build Bot]

Comment on attachment 184878 [details]
Patch

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

[Adam Barth]

Comment on attachment 184878 [details]
Patch

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

Looks like you've got a problem with mac.

> Source/WebCore/Modules/notifications/Notification.cpp:113
> +    ASSERT(static_cast<Document*>(context)->page());

I don't think this ASSERT is valid.  Create a popup window, grab a reference to its Notification constructor, and then close the popup.  Now, if you call the Notification constructor, the page will be 0.

[Adam Barth]

http://www.w3.org/TR/notifications/#dom-notification doesn't say anything about the constructor throwing an exception...

Created attachment 184882 [details]
Patch

You're right. I moved the check from the timer to show()

I also changed the assert into just exiting gracefully if the page is gone.

[Adam Barth]

Comment on attachment 184882 [details]
Patch

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

> LayoutTests/fast/notifications/notifications-constructor-with-permission.html:25
> +            setTimeout(function() { testRunner.notifyDone(); }, 100);

This isn't flaky because the timer gets scheduled first, right?

Created attachment 184908 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 184908 [details]
Patch for landing

Clearing flags on attachment: 184908

Committed r140927: <http://trac.webkit.org/changeset/140927>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jon Lee]

Why was this change necessary? With the legacy API you end up checking permissions twice-- once at construction (which throws an error) and once at show() (which calls onerror).

For the standard API show() is called on a 0-delay timer when the notification is created, so this change is a no-op, essentially. But it seems odd to have to check the permission twice for the legacy case.

(In reply to comment #13)
> Why was this change necessary? With the legacy API you end up checking permissions twice-- once at construction (which throws an error) and once at show() (which calls onerror).
> 
> For the standard API show() is called on a 0-delay timer when the notification is created, so this change is a no-op, essentially. But it seems odd to have to check the permission twice for the legacy case.

Because new Notification("blah").show() would show a notification

[Jake Archibald]

Btw, the show() method is no longer part of the spec. Notifications show when they're constructed http://notifications.spec.whatwg.org/

[Jake Archibald]

(disregard my comment, I didn't see the 'legacy' part of the show() discussion)