-
Notifications
You must be signed in to change notification settings - Fork 43
Expose pushManager on Window #393
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: gh-pages
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This introduces a new feature whereby push messages conforming to a certain JSON format directly create an end user notification and show it (possibly preceded by an enhanced push event). In addition to showing a notification, the app badge can be updated as well. This builds on whatwg/notifications#213 which adds URL members to notifications. Exposing PushManager outside of service workers is handled by #393.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first round of review. Looks mostly fine but with some questions.
subscription</a> having the new keys as |newSubscription|. | ||
If the <a>user agent</a> has to change the keys of a [=push subscription=] for any reason | ||
and the [=push subscription=]'s [=associated service worker registration=] is non-null, | ||
it MUST [=refresh=] the [=push subscription=]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Reviewer note: this change is an editorial refactoring to reuse the term "refresh")
</h2> | ||
<p> | ||
A <a>user agent</a> or <a>push service</a> MAY choose to <dfn>refresh</dfn> a <a>push | ||
subscription</a> at any time, for example because it has reached a certain age. | ||
subscription</a> whose [=associated service worker registration=] is non-null at any | ||
time, for example because it has reached a certain age. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean a subscription without an SWR is never supposed to be refreshed? I think the Firefox push server will keep its logic to auto-clear subscriptions (we do that when an inactive subscription is being spammed and gets a huge amount of message queue). cc @jrconlin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clearing seems fine, but refreshing indeed doesn't work. See #401.
</li> | ||
<li>If |subscription| is non-null: | ||
<ol> | ||
<li>If there is an error with |subscription|, then [=queue a global task=] on the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence now sounds like there's an error field on subscriptions, but given the previous sentence was "Let subscription be the result of obtaining registration's push subscription. If there is an error, ..." it was more about an error while obtaining the subscription.
I think either we should throw earlier in the subscription obtaining steps or just remove this step, as the obtaining steps are now clearer and infallible (except the existing null check)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep this as this is not something I was aiming to change in this PR. I'm not really sure how to do it earlier though without getting very verbose.
</li> | ||
<li>If |subscription| is null, then resolve |promise| with null. | ||
</li> | ||
<li>If there is an error with |subscription|, reject |promise| with a {{DOMException}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Same about error, might be nicer if we throw in the obtaining steps or just skip this. It's unclear what a caller should do if this function throws, so maybe implementations should just pretend that no subscription exists if it's corrupted or something. If there's a good reason to throw error, it would be nice to have a note about that.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just trying to preserve existing text.
This comment was marked as resolved.
This comment was marked as resolved.
This introduces a new feature whereby push messages conforming to a certain JSON format directly create an end user notification and show it (possibly preceded by an enhanced push event). In addition to showing a notification, the app badge can be updated as well. This builds on whatwg/notifications#213 which adds URL members to notifications. Exposing PushManager outside of service workers is handled by #393.
This introduces a new feature whereby push messages conforming to a certain JSON format directly create an end user notification and show it (possibly preceded by an enhanced push event). In addition to showing a notification, the app badge can be updated as well. This builds on whatwg/notifications#213 which adds URL members to notifications. Exposing PushManager outside of service workers is handled by #393.
This is part of the Declarative Web Push initiative (see #360). This makes window.pushManager work by making push subscriptions tied to a scope rather than a service worker registration. Most often push subscriptions remain 1:1 with service worker registrations, but the scope whose serialized path is "/" is treated specially from now on and can exist independently.
6e463a7
to
895dd36
Compare
This is part of the Declarative Web Push initiative (see #360) and mainly makes sense when that is supported, although could be independently supported in theory.
This makes window.pushManager work by making push subscriptions tied to a scope rather than a service worker registration. Most often push subscriptions remain 1:1 with service worker registrations, but the scope whose serialized path is "/" is treated specially from now on and can exist independently.
This obsoletes #368.
The following tasks have been completed:
Implementer support:
Preview | Diff