Closed Bug 1269385 Opened 9 years ago Closed 9 years ago

Need event to listen for newly created Push Subscriptions

Categories

(Core :: DOM: Notifications, defect)

47 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: janx, Assigned: lina)

References

Details

(Whiteboard: btpp-active)

Attachments

(1 file, 1 obsolete file)

For bug 1188981, about:debugging would like to know when a Service Worker's Push Subscription changes, so it can ask the platform about any new information to show. Bug 1266433 allowed interested observers to receive "push-message", "push-subscription-change" and "push-subscription-lost" events, allowing about:debugging to show up-to-date Push Subscription details for: - SWs that don't have a push subscription - SWs that already have a push subscription - SWs for which the push subscription is dropped However, for SWs that don't have a push subscription but then create new one, it seems we're missing an event. STR: - Open tab https://goroost.com/try-web-push (this registers a SW and subscribes to push) - Open tab about:preferences#content, Choose allowed sites, Remove goroost - Refresh goroost tab, and accept the Push Notifications prompt again - When receiving the "push-subscription-change" event, call `PushService.getSusbscription(scope, principal, callback)`. Expected: - `callback` is called with the new Push Subscription Actual: - `callback` is called with `null`, but if you poll `PushService.getSubscription()` for a little more than 3 seconds, you will eventually get the new Push Subscription Details: - "push-subscription-change" is obviously not the right event to detect the creation of a new Push Subscription object. Instead, the same event signals to the Service Worker that it can re-subscribe, but the latter needs a little over 3 seconds to do that. - Instead, we would need an event indicating "a new Push Subscription was successfully created". Kit, on IRC you suggested expanding the "push-subscription-lost" event to also notify about newly created Push Subscriptions. Would you be interested in addressing this?
Attached patch pushSubRefresh.patch (obsolete) (deleted) — Splinter Review
Attachment #8747792 - Flags: review?(dd.mozilla)
Attachment #8747792 - Flags: feedback?(janx)
This ended up being a nice patch for the train. :-) I renamed "push-subscription-lost" to "push-subscription-refresh," and made it fire whenever a subscription is created, dropped, and for every incoming push and quota change. So Dev Tools can listen for one event.
Status: NEW → ASSIGNED
Whiteboard: btpp-active
Comment on attachment 8747792 [details] [diff] [review] pushSubRefresh.patch Review of attachment 8747792 [details] [diff] [review]: ----------------------------------------------------------------- Works like a charm, thank you Kit!
Attachment #8747792 - Flags: feedback?(janx) → feedback+
Attached patch pushSubRefresh.patch (deleted) — Splinter Review
Added more test cases, and fixed an oversight where the event didn't fire after clearing permissions. https://treeherder.mozilla.org/#/jobs?repo=try&revision=baf49c7ace88
Attachment #8747792 - Attachment is obsolete: true
Attachment #8747792 - Flags: review?(dd.mozilla)
Attachment #8749077 - Flags: review?(dd.mozilla)
Comment on attachment 8749077 [details] [diff] [review] pushSubRefresh.patch Review of attachment 8749077 [details] [diff] [review]: ----------------------------------------------------------------- Ship it! sorry for a delay. ::: dom/interfaces/push/nsIPushNotifier.idl @@ +14,5 @@ > > // These constants are duplicated in `PushComponents.js`. > #define OBSERVER_TOPIC_PUSH "push-message" > #define OBSERVER_TOPIC_SUBSCRIPTION_CHANGE "push-subscription-change" > +#define OBSERVER_TOPIC_SUBSCRIPTION_REFRESH "push-subscription-refresh" At the first read the name is misleading, maybe because it can also be "subscription is dropped". subscription-change sounds better but we already have that one. So if you do not come up with a different name it is fine.
Attachment #8749077 - Flags: review?(dd.mozilla) → review+
Thanks for the review! (In reply to Dragana Damjanovic [:dragana] from comment #6) > Comment on attachment 8749077 [details] [diff] [review] > pushSubRefresh.patch > > At the first read the name is misleading, maybe because it can also be > "subscription is dropped". subscription-change sounds better but we already > have that one. I agree. In some ways, "subscription-change" is better for this event, and "subscription-lost" is a better name for what we're calling "subscription-change" now. Other than Sync, we don't have any chrome callers, so it might be OK to make this change. On the other hand, the service worker event is called "onpushsubscriptionchange", so it seems odd to have `notifySubscriptionLost` trigger "subscription-lost" observers and a "pushsubscriptionchange" event...and `notifySubscriptionChange` wouldn't fire an event at all. "push-subscription-state-change" seems wordy, and too similar to "push-subscription-change". "push-subscription-update" might be a good name.
"push-subscription-modified" could work, too.
(In reply to Kit Cambridge [:kitcambridge] from comment #8) > "push-subscription-modified" could work, too. I'll go with that.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: