- From: Martin Thomson <notifications@github.com>
- Date: Mon, 19 Dec 2016 19:39:43 -0800
- To: w3c/push-api <push-api@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <w3c/push-api/pull/234/review/13687269@github.com>
martinthomson approved this pull request.
This is definitely an improvement.
@kitcambridge, does this make sense to you?
> + A <a>push subscription</a> has internal slots for a P-256 <a>ECDH</a> key pair and an
+ authentication secret in accordance with [[!WEBPUSH-ENCRYPTION]]. These slots MUST be
+ populated when creating the <a>push subscription</a>.
+ </p>
+ <p>
+ If the <a>user agent</a> has to change the keys for any reason,
+ it MUST <a>fire the pushsubscriptionchange event</a> with the
+ <a>service 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> having the old keys as <var>oldSubscription</var> and a
+ <a>PushSubscription</a> instance representing the <a>push subscription</a> having the new
+ keys as <var>newSubscription</var>.
+ </p>
+ <section>
+ <h2>
+ Subscription refreshes
nit:
title case for titles (throughout)
> + <h2>
+ Subscription refreshes
+ </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.
+ </p>
+ <p>
+ When this happens, the <a>user agent</a> MUST create a new <a>push subscription</a>
+ with the <a>push service</a> on behalf of the application, using the
+ <a>PushSubscriptionOptions</a> that were provided for creating the current
+ <a>push subscription</a>. The new <a>push subscription</a> MUST have a key pair that's
+ 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.
> + <a>PushSubscriptionOptions</a> that were provided for creating the current
+ <a>push subscription</a>. The new <a>push subscription</a> MUST have a key pair that's
+ different from the original subscription.
+ </p>
+ <p>
+ When successful, <a>user agent</a> then MUST <a>fire the pushsubscriptionchange event</a>
+ with the <a>service worker registration</a> associated with the <a>push subscription</a>
+ as <var>registration</var>, a <a>PushSubscription</a> instance representing the initial
+ <a>push subscription</a> as <var>oldSubscription</var> and a <a>PushSubscription</a>
+ instance representing the new <a>push subscription</a> as <var>newSubscription</var>.
+ </p>
+ <p>
+ <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>.`
> @@ -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
+ <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
> @@ -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
+ <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
+ <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
> </li>
- <li>Invoke the <a>Handle Functional Event</a> algorithm with a <a>service worker
- registration</a> of <var>registration</var> and <var>callbackSteps</var> set to the
- following steps:
+ <li>Set the <code>oldSubscription</code> attribute of <var>event</var> to
+ <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.
> + </pre>
+ <p>
+ The <dfn>newSubscription</dfn> attribute contains the details of the
+ <a>push subscription</a> that is valid per invocation of the <a>pushsubscriptionchange</a>
+ event. The value will be <code>null</code> when no new <a>push subscription</a> could be
+ established, for example because the <a>webapp</a> has lost <a>express permission</a>.
+ </p>
+ <p>
+ The <dfn>oldSubscription</dfn> attribute contains the details of the
+ <a>push subscription</a> that SHOULD NOT be used anymore. The value will be
+ <code>null</code> when the <a>user agent</a> is not able to provide the full set of
+ details, for example because of partial database corruption.
+ </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.
> + with the <a>service worker registration</a> associated with the <a>push subscription</a>
+ as <var>registration</var>, a <a>PushSubscription</a> instance representing the initial
+ <a>push subscription</a> as <var>oldSubscription</var> and a <a>PushSubscription</a>
+ instance representing the new <a>push subscription</a> as <var>newSubscription</var>.
+ </p>
+ <p>
+ <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.
+ </p>
+
+ <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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/push-api/pull/234#pullrequestreview-13687269
Received on Tuesday, 20 December 2016 03:40:37 UTC