| Summary: | The notification constructor needs to check permissions synchronously | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | jochen | ||||||||||
| Component: | New Bugs | Assignee: | jochen | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | abarth, buildbot, jaffathecake, jonlee, ojan.autocc, rniwa, webkit.review.bot | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
jochen
2013-01-26 11:38:04 PST
Created attachment 184877 [details]
Patch
Created attachment 184878 [details]
Patch
Comment on attachment 184878 [details] Patch Attachment 184878 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16122899 Comment on attachment 184878 [details] Patch Attachment 184878 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16114962 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. 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. 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
Comment on attachment 184908 [details] Patch for landing Clearing flags on attachment: 184908 Committed r140927: <http://trac.webkit.org/changeset/140927> All reviewed patches have been landed. Closing bug. 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 Btw, the show() method is no longer part of the spec. Notifications show when they're constructed http://notifications.spec.whatwg.org/ (disregard my comment, I didn't see the 'legacy' part of the show() discussion) |