-
Notifications
You must be signed in to change notification settings - Fork 49
Editorial: add "has showNotification() been successfully invoked" #227
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: main
Are you sure you want to change the base?
Conversation
This is a concept needed for Declarative Web Push: see w3c/push-api#360 for context.
@@ -1098,6 +1104,10 @@ method steps are: | |||
|
|||
<li><p>Run the <a for=/>notification show steps</a> for <var>notification</var>. | |||
|
|||
<li><p>Set <var>notification</var>'s <a for=notification>service worker registration</a>'s | |||
<a for="service worker registration">has <code>showNotification()</code> been successfully invoked</a> | |||
to true. |
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.
Nothing sets it back to false, perhaps the push event handler should set it false in w3c/push-api#385?
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 also wonder what would happen if another push event is fired before the previous one finishes, because then this value would conflict. Perhaps we could have PushEvent.showNotification() (where the default arg is the DWP parse result) to make it per-event? 🤔
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.
Right, I was waiting to see if this approach (which also means you have to process pushes sequentially) works for everyone, but then nobody gave feedback for what felt like an eternity…
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.
@saschanaz it seems good to resolve this as well.
The idea here is indeed for the Push API PR to set this to false and for the Push API specification to enforce incoming messages are processed sequentially. (Perhaps scoped to a site, but I don't think we have to go into that much detail for now.)
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 think Gecko waits for service worker until processing the next event, the PushService notifies the service worker and moves on for the next push message. There should be something that ties a PushEvent and showNotification.
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 WebKit makes sure it's sequential btw? queueTaskToFireDeclarativePushEvent looks like the queued task would end after adding a callback to the extended lifetime promise, is there an internal test for 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.
I think the idea is to not queue another task until the callback is called. But I'll double check. (I don't think you can tie them together without API changes and additional complexity for web developers which doesn't really seem warranted and rather undesirable at this point. I investigated this pretty extensively a year ago or so.)
This is a concept needed for Declarative Web Push: see w3c/push-api#360 for context.
Preview | Diff