Properly define the pushsubscriptionchange event #234

beverloo · 2016-12-20T01:59:34Z

This is a first attempt at #228 and related issues. An online preview is available.

I added a few inline issues because iteration will be necessary - more things will have to be split out in algorithms. I think that coincidentally improves clarity, so I plan to model the push event after pushsubscriptionchange, but will do that in a separate PR.

WDYT?

Closes #228, #193, #132, #120, #61.

martinthomson

This is definitely an improvement.

@kitcambridge, does this make sense to you?

martinthomson · 2016-12-20T03:22:54Z

index.html

+        </p>
+        <section>
+          <h2>
+            Subscription refreshes


nit:
title case for titles (throughout)

martinthomson · 2016-12-20T03:24:25Z

index.html

+            different from the original subscription.
+          </p>
+          <p>
+            When successful, <a>user agent</a> then MUST <a>fire the pushsubscriptionchange event</a>


What about when the refresh is not successful? I think that we could recommend/suggest that the user agent either retain the old subscription, though it may treat the subscription as broken at its discretion.

Should we require the user agent to retry if the refresh is unsuccessful? (Or fire pushsubscriptionchange with newSubscription set to null, as a last resort?)

I added the following paragraph:

If the user agent is not able to refresh the push subscription, it should periodically retry the refresh. When the push subscription can no longer be used, for example because it has expired, the user agent must fire the pushsubscriptionchange event with the service worker registration associated with the push subscription as registration, a PushSubscription instance representing the deactivating push subscription as oldSubscription and null as the newSubscription.

I like Kit's suggestion of setting newSubscription to null since it gives the developer something.

martinthomson · 2016-12-20T03:25:58Z

index.html

+            <a>User agents</a> MAY continue to accept messages for the old <a>push subscription</a>
+            for a brief amount of time, but MUST stop doing so once the first message for the
+            refreshed <a>push subscription</a> has been received, as this indicates that the
+            <a>application server</a> has propagated the subscription change.


That creates an interesting externality: if the application is required to update two data sources with updated subscription details, it has to ensure that one source isn't used before the other is emplaced. I think that's a reasonable global trade-off though.

I would restructure this to more directly link the MAY to the reason:

To allow for time to propagate changes to <a>application servers</a>, a <a>user agent</a> MAY continue to accept messages for an old <a>push subscription</a> for a brief time after a refresh. Once messages have been received for a refreshed <a>subscription</a>, any old <a>subscription</a> MUST be <a>deactivated</a>.

Thanks! Adopted.

martinthomson · 2016-12-20T03:28:38Z

index.html

-        MUST be <a>deactivated</a>.
+        When a permission is revoked, the <a>user agent</a> MAY
+        <a>fire the pushsubscriptionchange event</a> for subscriptions created with that permission,
+        with the <a>servce worker registration</a> associated with the <a>push subscription</a> as


typo: servce

martinthomson · 2016-12-20T03:29:22Z

index.html

+        with the <a>servce worker registration</a> associated with the <a>push subscription</a> as
+        <var>registration</var>, a <a>PushSubscription</a> instance representing the
+        <a>push subscription</a> as <var>oldSubscription</var>, and <code>null</code> as
+        <var>newSubscription</var>. The <a>user agent</a> MUST, in parallel, <a>deactivate</a>


the user agent must deactivate the affected subscriptions in parallel

martinthomson · 2016-12-20T03:32:04Z

index.html

+          <var>oldSubscription</var>.
+          </li>
+          <li>Invoke the <a>Handle Functional Event</a> algorithm with <var>event</var>,
+          <var>registration</var> and <var>callbackSteps</var> set to the following steps:


The list construction implies that all three inputs are set to the following steps. I don't know how to improve that with some more restructuring though.

Slightly rephrased.

martinthomson · 2016-12-20T03:34:27Z

index.html

+          </p>
+          <p class="issue">
+            Should we include an attribute that indicates whether the <code>oldSubscription</code>
+            has been decisively invalidated? How would developers use this information?


I think that the answer here is no. The intent of having the old subscription respond to push messages is to avoid races. We should not encourage use of subscriptions once they are considered old.

I agree. Allowing the old subscription to briefly receive messages is helpful, but I don't think we want folks depending on it.

Agreed. Thanks - removed.

martinthomson · 2016-12-20T03:38:57Z

index.html

+          <p class="issue">
+            The steps for creating a subscription should be separated from the
+            <a lt="PushManager.subscribe">subscribe</a> method into an algorithm of its own so that
+            it can be referenced here. Similarly, the <a>PushSubscriptionOptions</a> provided to the


I would raise the issue on github.

#235. Removed here.

ghost · 2016-12-29T23:09:20Z

index.html

@@ -423,8 +473,13 @@
        acquiring permission or determining the permission status.
      </p>
      <p>
-        When a permission is revoked, all <a>push subscriptions</a> created with that permission
-        MUST be <a>deactivated</a>.
+        When a permission is revoked, the <a>user agent</a> MAY


What about if permission is granted again, after being revoked? @beverloo, I think you mentioned Chrome won't fire a pushsubscriptionchange event in that case, right?

I actually think we found a case where we need to do this too. Let me file a more elaborate issue on it in a bit.

ghost · 2016-12-30T00:40:20Z

Sorry for the delay. Modulo a couple of questions, this LGTM!

beverloo

Thank you for the feedback. Let's add this and build upon it.

beverloo · 2017-02-17T15:53:24Z

index.html

+        </p>
+        <section>
+          <h2>
+            Subscription refreshes


beverloo · 2017-02-17T16:10:44Z

index.html

+            different from the original subscription.
+          </p>
+          <p>
+            When successful, <a>user agent</a> then MUST <a>fire the pushsubscriptionchange event</a>


I added the following paragraph:

If the user agent is not able to refresh the push subscription, it should periodically retry the refresh. When the push subscription can no longer be used, for example because it has expired, the user agent must fire the pushsubscriptionchange event with the service worker registration associated with the push subscription as registration, a PushSubscription instance representing the deactivating push subscription as oldSubscription and null as the newSubscription.

I like Kit's suggestion of setting newSubscription to null since it gives the developer something.

beverloo · 2017-02-17T16:12:18Z

index.html

+            <a>User agents</a> MAY continue to accept messages for the old <a>push subscription</a>
+            for a brief amount of time, but MUST stop doing so once the first message for the
+            refreshed <a>push subscription</a> has been received, as this indicates that the
+            <a>application server</a> has propagated the subscription change.


Thanks! Adopted.

beverloo · 2017-02-17T16:13:39Z

index.html

+          <p class="issue">
+            The steps for creating a subscription should be separated from the
+            <a lt="PushManager.subscribe">subscribe</a> method into an algorithm of its own so that
+            it can be referenced here. Similarly, the <a>PushSubscriptionOptions</a> provided to the


#235. Removed here.

beverloo · 2017-02-17T16:14:33Z

index.html

@@ -423,8 +473,13 @@
        acquiring permission or determining the permission status.
      </p>
      <p>
-        When a permission is revoked, all <a>push subscriptions</a> created with that permission
-        MUST be <a>deactivated</a>.
+        When a permission is revoked, the <a>user agent</a> MAY


I actually think we found a case where we need to do this too. Let me file a more elaborate issue on it in a bit.

beverloo · 2017-02-17T16:14:52Z

index.html

-        MUST be <a>deactivated</a>.
+        When a permission is revoked, the <a>user agent</a> MAY
+        <a>fire the pushsubscriptionchange event</a> for subscriptions created with that permission,
+        with the <a>servce worker registration</a> associated with the <a>push subscription</a> as


beverloo · 2017-02-17T16:15:14Z

index.html

+        with the <a>servce worker registration</a> associated with the <a>push subscription</a> as
+        <var>registration</var>, a <a>PushSubscription</a> instance representing the
+        <a>push subscription</a> as <var>oldSubscription</var>, and <code>null</code> as
+        <var>newSubscription</var>. The <a>user agent</a> MUST, in parallel, <a>deactivate</a>


beverloo · 2017-02-17T16:19:49Z

index.html

+          <var>oldSubscription</var>.
+          </li>
+          <li>Invoke the <a>Handle Functional Event</a> algorithm with <var>event</var>,
+          <var>registration</var> and <var>callbackSteps</var> set to the following steps:


Slightly rephrased.

beverloo · 2017-02-17T16:20:11Z

index.html

+          </p>
+          <p class="issue">
+            Should we include an attribute that indicates whether the <code>oldSubscription</code>
+            has been decisively invalidated? How would developers use this information?


Agreed. Thanks - removed.

beverloo added 6 commits December 19, 2016 23:49

Move the ServiceWorkerGlobalScope extensions to their own section

f2f99fa

Redefine pushsubscriptionchange

9f859b0

Minor textual updates

1ee1d48

Fire subscriptionchange when changing the encryption keys

90ad882

Fire pushsubscriptionchange when refreshing a subscription

c84869a

Maybe fire pushsubscriptionchange on revoked permission

109d4a9

martinthomson approved these changes Dec 20, 2016

View reviewed changes

ghost reviewed Dec 29, 2016

View reviewed changes

Address the review comments.

30cca42

beverloo commented Feb 17, 2017

View reviewed changes

beverloo merged commit 60708ea into w3c:gh-pages Feb 17, 2017

gauntface mentioned this pull request Feb 21, 2017

Future Changes gauntface/web-push-book#48

Open

linabutler mentioned this pull request May 8, 2020

pushsubscriptionchange event doesn't have oldSubscription in it. #325

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly define the pushsubscriptionchange event #234

Properly define the pushsubscriptionchange event #234

beverloo commented Dec 20, 2016 •

edited by martinthomson

martinthomson left a comment

martinthomson Dec 20, 2016

beverloo Feb 17, 2017

martinthomson Dec 20, 2016

ghost Dec 29, 2016

beverloo Feb 17, 2017

martinthomson Dec 20, 2016

beverloo Feb 17, 2017

martinthomson Dec 20, 2016

beverloo Feb 17, 2017

martinthomson Dec 20, 2016

beverloo Feb 17, 2017

martinthomson Dec 20, 2016

beverloo Feb 17, 2017

martinthomson Dec 20, 2016

ghost Dec 30, 2016

beverloo Feb 17, 2017

martinthomson Dec 20, 2016

beverloo Feb 17, 2017

ghost Dec 29, 2016

beverloo Feb 17, 2017

ghost commented Dec 30, 2016

beverloo left a comment

beverloo Feb 17, 2017

beverloo Feb 17, 2017

beverloo Feb 17, 2017

beverloo Feb 17, 2017

beverloo Feb 17, 2017

beverloo Feb 17, 2017

beverloo Feb 17, 2017

beverloo Feb 17, 2017

beverloo Feb 17, 2017

Properly define the pushsubscriptionchange event #234

Properly define the pushsubscriptionchange event #234

Conversation

beverloo commented Dec 20, 2016 • edited by martinthomson

martinthomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Dec 30, 2016

beverloo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beverloo commented Dec 20, 2016 •

edited by martinthomson