Merge replacing and displaying into showing #126
Conversation
notifications.bs
Outdated
| set, <a for=/>fetch</a> <var>notification</var>'s <a for=notification>sound URL</a> if it has | ||
| been set. | ||
| <p>If the notification platform supports sounds, and either <var>notification</var>'s | ||
| <a for=notification>old notification</a> is or <var>notification</var>'s |
There was a problem hiding this comment.
Remove "is", or change it to "is set"?
notifications.bs
Outdated
|
|
||
| <ol> | ||
| <li><p><a>Handle close events</a> with <var>notification</var>'s | ||
| <a for=notification>old notification</a>. |
There was a problem hiding this comment.
Since old notification is set immediately, but these steps are run in parallel, doesn't this now imply that the following snippet would result in the close event on n being fired twice?
const n = new Notification('foo', { tag: 'a' });
n.addEventListener('show', () => {
new Notification('bar', { tag: 'a' });
new Notification('baz', { tag: 'a' });
});I guess we can either add a closing flag that's set immediately, or add some logic to ensure that the close steps are not run twice for a particular notification?
There was a problem hiding this comment.
Good catch. I think that once #127 lands I could rebase and rewrite this bit to query the old notification during showing. That should remove this issue and leave less state on notifications.
The only thing that remains to fully remove all race conditions is to use a "parallel queue" to ensure that the "in parallel" steps don't race with each other. I need to file a follow-up on that.
This gives a more consistent story for event dispatching. The former setup was especially buggy if native replacement is not supported. This also reduces duplication (such as waiting for fetches). And it queues a task for the show and close events.
|
I rebased this on top of the sound removal and thereby addressed the race issue (I hope). I filed #128 as a follow-up to formalize even more. |
notifications.bs
Outdated
| <a>notification</a> and <var>notification</var>. | ||
| <li><p>Wait for any <a for=/ lt=fetch>fetches</a> to complete and <var>notification</var>'s | ||
| <a for=notification>image resource</a> <a for=notification>icon resource</a>, | ||
| <a for=notification>badge resource</a>, and <a for=notification>sound resource</a> to be set |
notifications.bs
Outdated
| @@ -70,7 +70,7 @@ run. | |||
|
|
|||
| <p>A <a>notification</a> has an associated | |||
| <dfn for=notification id=silent-preference-flag>silent preference flag</dfn> which is initially | |||
| unset. When set indicates that no sounds or vibrations should be made. | |||
There was a problem hiding this comment.
I kept this in because the user agent could still suppress any default sounds the platform's notification center may make, which feels appropriate for a property called silent.
This gives a more consistent story for event dispatching. The former setup was especially buggy if native replacement is not supported.
This also reduces duplication (such as waiting for fetches).
And it queues a task for the show and close events.
Preview | Diff