-
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
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.
</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
A significant portion of this PR can be part of an editorial PR. I'll file a new one for them and merge it myself, for easier review here. |
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 don't see a blocker, just a few editorial things. I'm fine with dealing with them in separate issues.
[=push subscription=]. | ||
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.
(Might want to mention #401 in a note)
Section 4 about security consideration has this:
We probably want to change that. (Either remove that or make it non-normative? Not sure why it should be defined again in a security section.) Edit: Maybe the Subscription deactivation section should say MUST for deactivation after unregistration. |
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