Re: [w3c/ServiceWorker] How to create registration objects (#1445)

mattto approved this pull request.

lgtm! Just some nits (feel free to ignore if they don't sound right).

> @@ -477,7 +477,27 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
       };
     </pre>
 
-    A {{ServiceWorkerRegistration}} object represents a [=/service worker registration=]. Each {{ServiceWorkerRegistration}} object is associated with a <dfn for="ServiceWorkerRegistration">service worker registration</dfn> (a [=/service worker registration=]). Multiple separate objects implementing the {{ServiceWorkerRegistration}} interface across documents and workers can all be associated with the same [=/service worker registration=] simultaneously.
+    <section>
+      <h4 id="service-worker-registration-creation">Getting {{ServiceWorkerRegistration}} instances</h4>
+
+      An [=environment settings object=] has a <dfn for="environment settings object">service worker registration object map</dfn>, a [=/map=] where the [=map/keys=] are [=/service worker registrations=] and the [=map/values=] are {{ServiceWorkerRegistration}} objects.
+
+      <section algorithm="service-worker-registration-creation-algorithm">
+        To <dfn lt="get the service worker registration object|getting the service worker registration object">get the service worker registration object</dfn> representing |registration| (a [=/service worker registration=]) in |environment| (an [=environment settings object=]), run these steps:
+
+          1. Let |objectMap| be |environment|'s [=environment settings object/service worker registration object map=].
+          1. If |objectMap|[|registration|] does not [=map/exist=], then:
+            1. Let |registrationObject| be a new {{ServiceWorkerRegistration}} in |environment|'s [=environment settings object/Realm=].
+            1. Set |registrationObject|'s [=ServiceWorkerRegistration/service worker registration=] to |registration|.
+            1. Set |registrationObject|'s {{ServiceWorkerRegistration/installing}} attribute to null if |registration|'s [=service worker registration/installing worker=] is null, otherwise the result of [=getting the service worker object=] that represents |registration|'s [=service worker registration/installing worker=] in |environment|.

I think this is a run-on sentence. I was once told the spec convention is "to X, if Y, and Z otherwise".

> +      <section algorithm="service-worker-registration-creation-algorithm">
+        To <dfn lt="get the service worker registration object|getting the service worker registration object">get the service worker registration object</dfn> representing |registration| (a [=/service worker registration=]) in |environment| (an [=environment settings object=]), run these steps:
+
+          1. Let |objectMap| be |environment|'s [=environment settings object/service worker registration object map=].
+          1. If |objectMap|[|registration|] does not [=map/exist=], then:
+            1. Let |registrationObject| be a new {{ServiceWorkerRegistration}} in |environment|'s [=environment settings object/Realm=].
+            1. Set |registrationObject|'s [=ServiceWorkerRegistration/service worker registration=] to |registration|.
+            1. Set |registrationObject|'s {{ServiceWorkerRegistration/installing}} attribute to null if |registration|'s [=service worker registration/installing worker=] is null, otherwise the result of [=getting the service worker object=] that represents |registration|'s [=service worker registration/installing worker=] in |environment|.
+            1. Set |registrationObject|'s {{ServiceWorkerRegistration/waiting}} attribute to null if |registration|'s [=service worker registration/waiting worker=] is null, otherwise the result of [=getting the service worker object=] that represents |registration|'s [=service worker registration/waiting worker=] in |environment|.
+            1. Set |registrationObject|'s {{ServiceWorkerRegistration/active}} attribute to null if |registration|'s [=service worker registration/active worker=] is null, otherwise the result of [=getting the service worker object=] that represents |registration|'s [=service worker registration/active worker=] in |environment|.
+            1. Set |objectMap|[|registration|] to |registrationObject|.
+          1. Return |objectMap|[|registration|].
+      </section>
+    </section>
+
+    A {{ServiceWorkerRegistration}} has a <dfn for="ServiceWorkerRegistration">service worker registration</dfn> (a [=/service worker registration=]).

Could this sentence go above this section? Since the algorithm above refers to this definition.

> @@ -688,10 +708,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
         1. Let |promise| be a new <a>promise</a>.
         1. Run the following substeps <a>in parallel</a>:
             1. Let |registration| be the result of running <a>Match Service Worker Registration</a> algorithm with |clientURL| as its argument.
-            1. If |registration| is not null, then:
-                1. Resolve |promise| with the {{ServiceWorkerRegistration}} object which represents |registration|.
-            1. Else:
-                1. Resolve |promise| with undefined.
+            1. If |registration| is null, resolve |promise| with undefined, else resolve |promise| with the result of [=getting the service worker registration object=] that represents |registration| in |promise|'s [=relevant settings object=].

I'm not sure this if/else construction is conventional. How about splitting it to two statements or else doing the: "Resolve |promise| with undefined, if |registration| is null, and the result of getting the registration... otherwise."

-- 
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/1445#pullrequestreview-256194801