Re: [w3c/ServiceWorker] Kill resurrection (#1415)

mattto approved this pull request.

Looks good! Just have nits.

> @@ -1435,18 +1445,10 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

 
       <dfn method for="FetchEvent"><code>respondWith(|r|)</code></dfn> method *must* run these steps:
 
+        1. Let |event| be the [=context object=].
         1. If the <a>dispatch flag</a> is unset, [=throw=] an "{{InvalidStateError}}" {{DOMException}}.
         1. If the [=FetchEvent/respond-with entered flag=] is set, [=throw=] an "{{InvalidStateError}}" {{DOMException}}.

Should these be "event's dispatch flag..." and "event's respond-with entered flag" instead of "the flag"?

> @@ -220,6 +218,8 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

 
     A [=/service worker registration=] has an associated <dfn export>navigation preload header value</dfn>, which is a [=byte sequence=]. It is initially set to \`<code>true</code>\`.
 
+    A [=/service worker registration=] is said to be <dfn export id="dfn-service-worker-registration-unregistered">unregistered</dfn> if [=scope to registration map=][[=service worker registration/scope url=]] is not this [=/service worker registration=].

nitty: is it required to say "this registration's scope url" to qualify "scope url"?

> @@ -518,12 +518,12 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

     <section algorithm="navigator-service-worker-unregister">
       <h4 id="navigator-service-worker-unregister">{{ServiceWorkerRegistration/unregister()}}</h4>
 
-      Note: The {{ServiceWorkerRegistration/unregister()}} method unregisters the [=/service worker registration=]. It is important to note that the currently <a>controlled</a> [=/service worker client=]'s <a>active service worker</a>'s <a>containing service worker registration</a> is effective until all the [=/service worker clients=] (including itself) using this [=/service worker registration=] unload. That is, the {{ServiceWorkerRegistration/unregister()}} method only affects subsequent <a lt="navigate">navigations</a>.
+      Note: The {{ServiceWorkerRegistration/unregister()}} method unregisters the [=/service worker registration=]. It is important to note that the currently [=controlled=] [=/service worker client=]'s [=active service worker=]'s [=containing service worker registration=] is effective until all the [=/service worker clients=] (including itself) using this [=/service worker registration=] unload. That is, the {{ServiceWorkerRegistration/unregister()}} method only affects subsequent [=navigate|navigations=].

Thanks I've been doing the same :)

>  
       <dfn method for="ExtendableEvent"><code>waitUntil(|f|)</code></dfn> method *must* run these steps:
 
-        1. If the {{Event/isTrusted}} attribute is false, [=throw=] an "{{InvalidStateError}}" {{DOMException}}.
+        1. Let |event| be the [=context object=].
+        1. [=ExtendableEvent/Add lifetime promise=] |f| to |event|.

Looks nicer.

>  
             Note: The [=ExtendableEvent/pending promises count=] is incremented even if the given promise has already been settled. The corresponding count decrement is done in the microtask queued by the reaction to the promise.
 
-        1. Upon [=upon fulfillment|fulfillment=] or [=upon rejection|rejection=] of |f|, [=queue a microtask=] to run these substeps:
-            1. Decrement the [=ExtendableEvent/pending promises count=] by one.
-            1. Let |registration| be the [=context object=]'s [=relevant global object=]'s associated [=ServiceWorkerGlobalScope/service worker=]'s [=containing service worker registration=].
-            1. If |registration|'s [=uninstalling flag=] is set, invoke [=Try Clear Registration=] with |registration|.
-            1. If |registration| is not null, invoke [=Try Activate=] with |registration|.
+        1. Upon [=upon fulfillment|fulfillment=] or [=upon rejection|rejection=] of |promise|, [=queue a microtask=] to run these substeps:
+            1. Decrement |event|'s [=ExtendableEvent/pending promises count=] by one.
+            1. If |event|'s [=ExtendableEvent/pending promises count=] is 0, then:

Agreed.

> @@ -2454,7 +2455,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

       :: none
 
       1. Let |registration| be the result of running the <a>Get Registration</a> algorithm passing |job|'s [=job/scope url=] as the argument.
-      1. If |registration| is null or |registration|'s <a>uninstalling flag</a> is set, then:
+      1. If |registration| is null or |registration| is [=service worker registration/unregistered=], then:

Can Get Registration return an unregistered registration?

> @@ -2605,7 +2606,10 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

       1. Run the <a>Update Worker State</a> algorithm passing |registration|'s <a>installing worker</a> and *installing* as the arguments.
       1. Assert: |job|'s [=job/job promise=] is not null.
       1. Invoke [=Resolve Job Promise=] with |job| and |registration|.
-      1. <a>Queue a task</a> to <a>fire an event</a> named <code>updatefound</code> at all the {{ServiceWorkerRegistration}} objects for all the [=/service worker clients=] whose <a>creation URL</a> <a lt="Match Service Worker Registration">matches</a> |registration|'s [=service worker registration/scope url=] and all the [=/service workers=] whose <a>containing service worker registration</a> is |registration|.
+      1. Let |settingsObjects| be all [=environment settings objects=] whose [=environment settings object/origin=] is |registration|'s [=service worker registration/scope url=]'s origin.
+      1. For each |settingsObject| of |settingsObjects|, [=queue a task=] on |settingsObject| to run the following steps:
+          1. Let |registrationObjects| be every {{ServiceWorkerRegistration}} object in |settingsObject|'s [=environment settings object/realm execution context=], whose [=ServiceWorkerRegistration/service worker registration=] is |registration|.
+          1. For each |registrationObject| of |registrationObjects|, [=fire an event=] on |registrationObject| named `updatefound`.

Is a |settingsObject| a task source? Other parts of the spec seem to use language like "relevant settings object ’s responsible event loop and the DOM manipulation task source."

> @@ -3177,7 +3178,6 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

           1. Set |matchingScope| to the result of <a lt="URL parser">parsing</a> |matchingScopeString|.
           1. Assert: |matchingScope|'s [=url/origin=] and |clientURL|'s [=url/origin=] are [=same origin=].
       1. Let |registration| be the result of running <a>Get Registration</a> algorithm passing |matchingScope| as the argument.
-      1. If |registration| is not null and |registration|'s <a>uninstalling flag</a> is set, return null.
       1. Return |registration|.

Should we collapse these two steps into just "Return the result of running..."

>        1. [=map/For each=] |key| → |value| of <a>scope to registration map</a>:
-          1. If |scopeString| matches |key|, set |registration| to |value|.
-      1. Return |registration|.
+          1. If |scopeString| matches |key|, then return |value|.
+      1. Return null.

Nice.

-- 
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/ServiceWorker/pull/1415#pullrequestreview-244879754