Closed Bug 1265828 Opened 4 years ago Closed 4 years ago

Dismissed persistent notifications should be removed from `serviceWorkerRegistration.getNotifications()`

Categories

(Core :: DOM: Push Notifications, defect)

defect
Not set

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)

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.
Attached patch test.patch (obsolete) — Splinter Review
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: kcambridge → nobody
Status: ASSIGNED → NEW
Depends on: 1265841
Whiteboard: btpp-fixlater
(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.
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.
(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: nobody → kcambridge
Blocks: 1265841
Status: NEW → ASSIGNED
No longer depends on: 1265841
Whiteboard: btpp-fixlater → btpp-active
Attachment #8742950 - Attachment is obsolete: true
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?
(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!
Attachment #8743623 - Flags: review?(wchen) → review+
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.
Blocks: 1264815
https://hg.mozilla.org/mozilla-central/rev/f1e282db5cae
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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?
Marking affected for 48 based on comment 12
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+
You need to log in before you can comment on or make changes to this bug.