Closed
Bug 1265828
Opened 8 years ago
Closed 8 years ago
Dismissed persistent notifications should be removed from `serviceWorkerRegistration.getNotifications()`
Categories
(Core :: DOM: Notifications, defect)
Core
DOM: Notifications
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox47 | --- | unaffected |
firefox48 | --- | fixed |
firefox49 | --- | fixed |
People
(Reporter: lina, Assigned: lina)
References
Details
(Whiteboard: btpp-active)
Attachments
(1 file, 1 obsolete file)
58 bytes,
text/x-review-board-request
|
wchen
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details |
https://dxr.mozilla.org/mozilla-central/rev/67ac40fb8f680ea5e03805552187ba1b5e8392a1/dom/notification/Notification.cpp#1626-1657 should listen for `alertfinished` and call `Notification::UnpersistNotification`. Otherwise, `serviceWorkerRegistration.getNotifications()` will always return the stale notification, even if it's dismissed by other means (for example, clicking the "x" in Notification Center). XUL notifications are also auto-dismissed, even for "persistent" notifications.
Assignee | ||
Comment 1•8 years ago
|
||
I went to write a test for this, then realized we don't implement the `notificationclose` event yet. We can fake it using a timer, but I think it makes more sense to do that first, then follow up with this patch.
Assignee | ||
Updated•8 years ago
|
Assignee: kcambridge → nobody
Status: ASSIGNED → NEW
Assignee | ||
Updated•8 years ago
|
Whiteboard: btpp-fixlater
Comment 2•8 years ago
|
||
(In reply to Kit Cambridge [:kitcambridge] from comment #1) > We can fake it using a timer, but I think it makes more sense to do that > first, then follow up with this patch. Is this something that we could uplift? The technique described in https://developers.google.com/web/updates/2015/05/notifying-you-of-changes-to-notifications uses `getNotifications` and looks like something that a lot of developers would want to use.
Assignee | ||
Comment 3•8 years ago
|
||
I think the refactoring in bug 1265841 should take care of this. I don't know about uplifting (it's unlikely to make it in 48), and I'd prefer to let the change bake for a while. Another alternative would be to move the refactoring into this patch, and add `notificationclose` later. Let me think about it.
Comment 4•8 years ago
|
||
(In reply to Kit Cambridge [:kitcambridge] from comment #3) > I think the refactoring in bug 1265841 should take care of this. I don't > know about uplifting (it's unlikely to make it in 48), and I'd prefer to let > the change bake for a while. > > Another alternative would be to move the refactoring into this patch, and > add `notificationclose` later. Let me think about it. The `notificationclose` event looks less important to me, as it's only triggered for persistent notifications.
Assignee | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/47947/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47947/
Attachment #8743623 -
Flags: review?(wchen)
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Attachment #8742950 -
Attachment is obsolete: true
Assignee | ||
Comment 6•8 years ago
|
||
https://reviewboard.mozilla.org/r/47947/#review44677 ::: dom/notification/Notification.cpp:1731 (Diff revision 1) > // The observer is wholly owned by the NotificationObserver passed to the alert service. > - observer = new ServiceWorkerNotificationObserver(mScope, GetPrincipal(), mID); > + nsAutoString behavior; > + if (NS_WARN_IF(!mBehavior.ToJSON(behavior))) { > + behavior.Truncate(); > + } > + observer = new ServiceWorkerNotificationObserver(mScope, Same question here as bug 1265841, comment 2. ::: dom/workers/test/serviceworkers/test_notification_get.html:83 (Diff revision 1) > + is(notifications[0].tag, "dismiss", "Tag should match"); > + > + // Simulate dismissing the notification by using the alerts service > + // directly, instead of `Notification#close`. > + var principal = SpecialPowers.wrap(document).nodePrincipal; > + var id = principal.origin + "#tag:dismiss"; That reminds me, should it possible for a non-persistent notification to replace a persistent one if they have the same tag?
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to Marco Castelluccio [:marco] from comment #4) > The `notificationclose` event looks less important to me, as it's only > triggered for persistent notifications. Cool; thanks, Marco!
Updated•8 years ago
|
Attachment #8743623 -
Flags: review?(wchen) → review+
Comment 8•8 years ago
|
||
Comment on attachment 8743623 [details] MozReview Request: Bug 1265828 - Remove persistent notifications from storage. r?wchen https://reviewboard.mozilla.org/r/47947/#review46205 ::: dom/notification/Notification.cpp:1731 (Diff revision 1) > // The observer is wholly owned by the NotificationObserver passed to the alert service. > - observer = new ServiceWorkerNotificationObserver(mScope, GetPrincipal(), mID); > + nsAutoString behavior; > + if (NS_WARN_IF(!mBehavior.ToJSON(behavior))) { > + behavior.Truncate(); > + } > + observer = new ServiceWorkerNotificationObserver(mScope, Persistent notifications are different in that they dispatch the event on ServiceWorkerGlobalScope instead of the Notification object. This is why ServiceWorkerNotificationObserver doesn't take ownership of the Notification while the other observers do. From script, persistent notifications are not accessible from multiple threads, the reasons why we create them as they are needed is because that's the way it's specified. ::: dom/workers/test/serviceworkers/test_notification_get.html:83 (Diff revision 1) > + is(notifications[0].tag, "dismiss", "Tag should match"); > + > + // Simulate dismissing the notification by using the alerts service > + // directly, instead of `Notification#close`. > + var principal = SpecialPowers.wrap(document).nodePrincipal; > + var id = principal.origin + "#tag:dismiss"; I believe the answer is yes because the spec doesn't say otherwise. It doesn't seem to me that persistent notification tags should be different.
Assignee | ||
Comment 10•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=75f33e5fc03c
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f1e282db5cae
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Comment 12•8 years ago
|
||
Comment on attachment 8743623 [details] MozReview Request: Bug 1265828 - Remove persistent notifications from storage. r?wchen Approval Request Comment [Feature/regressing bug #]: Service worker notifications. [User impact if declined]: Possible web compat issue with Chrome. The article that Marco linked to in comment 2 suggests using `serviceWorkerRegistration.getNotifications()` to collapse multiple notifications. That technique won't work in 48 and older. [Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=75f33e5fc03c [Risks and why]: Low risk. This patch has baked on Nightly for a week, and persistent notification behavior is covered by existing tests. [String/UUID change made/needed]: None.
Attachment #8743623 -
Flags: approval-mozilla-aurora?
Comment 13•8 years ago
|
||
Marking affected for 48 based on comment 12
status-firefox47:
--- → unaffected
status-firefox48:
--- → affected
Comment 14•8 years ago
|
||
Comment on attachment 8743623 [details] MozReview Request: Bug 1265828 - Remove persistent notifications from storage. r?wchen Includes tests, fixes web compat/spec issue in 48
Attachment #8743623 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 15•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/eb1e2a6b09e3
You need to log in
before you can comment on or make changes to this bug.
Description
•