Re: [w3c/push-api] Expose pushManager on Window (PR #393)

@saschanaz commented on this pull request.

My first round of review. Looks mostly fine but with some questions.

> @@ -192,12 +202,9 @@ <h2>
           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 |registration|, a {{PushSubscription}}
-          instance representing the <a>push subscription</a> having the old keys as
-          |oldSubscription| and a {{PushSubscription}} instance representing the <a>push
-          subscription</a> having the new keys as |newSubscription|.
+          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=].

(Reviewer note: this change is an editorial refactoring to reuse the term "refresh")

>            </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.
+            subscription</a> whose [=associated service worker registration=] is non-null at any
+            time, for example because it has reached a certain age.

Does this mean a subscription without an SWR is never supposed to be refreshed? I think the Firefox push server will keep its logic to auto-clear subscriptions (we do that when an inactive subscription is being spammed and gets a huge amount of message queue). cc @jrconlin

>          [SecureContext]
-        partial interface ServiceWorkerRegistration {
+        interface mixin PushManagerAttribute {

Sorta weird name when it's a mixin and not an attribute. Bikeshedding: PushManagerBase (kinda following WebGL), PushManagerCommon (kinda following TextDecoder/Encoder), PushManageable (kinda following Animatable), PushManagerSource (kinda following FontFaceSource), PushManagerMixin (kinda following ARIAMixin and PaintTimingMixin), etc etc...

Not strongly against the current name though, especially it's just a spec-only name.

> @@ -578,7 +609,23 @@ <h3>
         </li>
         <li>Let |global| be [=this=]' [=relevant global object=].
         </li>
-        <li>Return |promise| and continue [=in parallel=].
+        <li>Let |scope| be null.
+        </li>
+        <li>Let |registration| be null.
+        </li>
+        <li>If [=this=] is a {{Window}} object, then set |scope| to the result of running the
+        [=basic URL parser=] given "`/`" and |global|'s <a>associated <code>Document</code></a>'s
+        [=Document/URL=].

This would fail for `about:blank`, but I guess that's fine as this is restricted to SecureContext.

>            <ol>
-            <li>If |options|'s {{PushSubscriptionOptionsInit/applicationServerKey}} is a
-            {{DOMString}}, set its value to an {{ArrayBuffer}} containing the sequence of octets
-            that result from decoding |options|'s
-            {{PushSubscriptionOptionsInit/applicationServerKey}} using the base64url encoding
-            [[RFC7515]].
+            <li>If the |options| argument has a {{PushSubscriptionOptionsInit/userVisibleOnly}}

(Reviewer note: nothing is changed here except indentation and rewrapping, following the "Run these steps in parallel" change and until the scope check below. It would be super nice if there was a smarter diff viewer for human language for this kind of changes.)

>              </li>
-            <li>When the request has been completed, [=queue a global task=] on the [=networking
-            task source=] using |global| to [=resolve=] |promise| with |subscription| and terminate
-            these steps.
+            <li>If |permission| is {{PermissionState/"denied"}}, [=queue a global task=] on the
+            [=user interaction task source=] using |global| to [=reject=] |promise| with a
+            {{"NotAllowedError"}} {{DOMException}} and terminate these steps.
+            </li>
+            <li>If |subscription| is non-null:
+              <ol>
+                <li>If there is an error with |subscription|, then [=queue a global task=] on the

This sentence now sounds like there's an error field on subscriptions, but given the previous sentence was "Let subscription be the result of obtaining registration's push subscription. If there is an error, ..." it was more about an error while obtaining the subscription.

I think either we should throw earlier in the subscription obtaining steps or just remove this step, as the obtaining steps are now clearer and infallible (except the existing null check)?

> +                </li>
+                <li>Compare the |options| argument with the `options` attribute of |subscription|.
+                The contents of {{BufferSource}} values are compared for equality rather than
+                [=ECMAScript/reference record|reference=].
+                </li>
+                <li>If any attribute on |options| contains a different value to that stored for
+                |subscription|, then [=queue a global task=] on the [=networking task source=]
+                using |global| to [=reject=] |promise| with an {{"InvalidStateError"}}
+                {{DOMException}} and terminate these steps.
+                </li>
+                <li>[=Queue a global task=] on the [=networking task source=] using |global| to
+                [=resolve=] |promise| with |subscription| and terminate these steps.
+                </li>
+              </ol>
+            </li>
+            <li>[=/Assert=]: |subscription| is null and |scope| is a [=/URL=].

(The null check assertion here is a bit silly, we're right below the null check above... 😛)

> +                |windowScope|, then set |subscription| to that [=push subscription=].
+                </li>
+              </ol>
+            </li>
+            <li>Otherwise:
+              <ol>
+                <li>[=/Assert=]: |registration| is non-null.
+                </li>
+                <li>If |registration|'s [=associated push subscription=] is non-null, then set
+                |subscription| to |registration|'s [=associated push subscription=].
+                </li>
+              </ol>
+            </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}}

(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.)

> @@ -288,13 +319,13 @@ <h2>
             delivered.
           </p>
           <p>
-            A <a>push subscription</a> is <a>deactivated</a> when its associated <a>service worker
-            registration</a> is unregistered, though a <a>push subscription</a> MAY be
-            <a>deactivated</a> earlier.
+            A <a>push subscription</a> without a [=push subscription/window-accessible scope=] is
+            <a>deactivated</a> when its associated <a>service worker registration</a> is
+            unregistered, though a <a>push subscription</a> MAY be <a>deactivated</a> earlier.

Should we provide a way to migrate to window-accessible scope to make an existing non-root scoped subscription be independent from service worker, probably as a followup?

> -            <li>Compare the |options| argument with the `options` attribute of |subscription|. The
-            contents of {{BufferSource}} values are compared for equality rather than
-            [=ECMAScript/reference record|reference=].
+            <li>Otherwise:
+              <ol>
+                <li>[=/Assert=]: |registration| is non-null.
+                </li>
+                <li>If |registration|'s [=service worker registration/active worker=] is null,
+                [=queue a global task=] on the [=networking task source=] using |global| to
+                [=reject=] |promise| with an {{"InvalidStateError"}} {{DOMException}} and terminate
+                these steps.
+                </li>
+                <li>If |registration|'s [=associated push subscription=] is non-null, then set
+                |subscription| to |registration|'s [=associated push subscription=].
+                </li>
+                <li>Set |scope| to |registration|'s [=service worker registration/scope URL=].

If the scope URL is the root path then this is also accessible from window, right? I assume that's the intention.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/w3c/push-api/pull/393#pullrequestreview-2605725427
You are receiving this because you are subscribed to this thread.

Message ID: <w3c/push-api/pull/393/review/2605725427@github.com>