- From: Jake Archibald <notifications@github.com>
- Date: Fri, 21 Aug 2020 04:58:10 -0700
- To: w3c/ServiceWorker <ServiceWorker@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <w3c/ServiceWorker/pull/1526/review/472362670@github.com>
@jakearchibald requested changes on this pull request.
Mostly prose nits. The only real concern is the backwards compatibility break with `reg.scope`.
> @@ -232,12 +236,12 @@ 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=][this [=/service worker registration=]'s [=service worker registration/scope url=]] is not this [=/service worker registration=].
+ A [=/service worker registration=] is said to be <dfn export id="dfn-service-worker-registration-unregistered">unregistered</dfn> if [=registration map=][(|origin|,|id|)] is not this [=/service worker registration=], where |origin| and |id| are this [=service worker registration=]'s [=service worker registration/origin=] and [=service worker registration/id=] respectively.
```suggestion
A [=/service worker registration=] |registration| is said to be <dfn export id="dfn-service-worker-registration-unregistered">unregistered</dfn> if [=registration map=][(|registration|'s [=service worker registration/origin=], |registration|'s [=service worker registration/id=])] is not |registration|.
```
> @@ -208,9 +210,11 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
<section dfn-for="service worker registration">
<h3 id="service-worker-registration-concept">Service Worker Registration</h3>
- A <dfn export id="dfn-service-worker-registration" for="">service worker registration</dfn> is a tuple of a [=service worker registration/scope url=] and a set of [=/service workers=], an <a>installing worker</a>, a <a>waiting worker</a>, and an <a>active worker</a>. A user agent *may* enable many [=/service worker registrations=] at a single origin so long as the [=service worker registration/scope url=] of the [=/service worker registration=] differs. A [=/service worker registration=] of an identical [=service worker registration/scope url=] when one already exists in the user agent causes the existing [=/service worker registration=] to be replaced.
+ A <dfn export id="dfn-service-worker-registration" for="">service worker registration</dfn> is a tuple of an [=environment settings object/origin=], an [=service worker registration/id=], and a set of [=/service workers=]; an <a>installing worker</a>, a <a>waiting worker</a>, and an <a>active worker</a>. A user agent *may* enable many [=/service worker registrations=] at a single origin so long as the [=service worker registration/id=] of the [=/service worker registration=] differs.
What do you think about just removing this paragraph? It doesn't seem to be saying anything that isn't already said elsewhere.
> : Output
- :: A [=/service worker registration=]
+ :: A [=/service worker registration=] or null.
+
+ 1. Run the following steps atomically.
I guess we have this a few places in the spec, but I don't actually know what it means.
> + 1. Run the following steps atomically.
+ 1. Let |oldestWorker| be null.
+ 1. If |registration|'s <a>active worker</a> is not null, set |oldestWorker| to |registration|'s <a>active worker</a>.
+ 1. Else if |registration|'s <a>waiting worker</a> is not null, set |oldestWorker| to |registration|'s <a>waiting worker</a>.
+ 1. Else if |registration|'s <a>installing worker</a> is not null, set |oldestWorker| to |registration|'s <a>installing worker</a>.
+ 1. Return |oldestWorker|.
I wonder if this is easier to read:
```suggestion
1. Return the first of the following that is not null:
1. |registration|'s [=active worker=].
1. |registration|'s [=waiting worker=].
1. |registration|'s [=installing worker=].
Otherwise, return null.
```
> <section algorithm="service-worker-registration-scope">
<h4 id="service-worker-registration-scope">{{ServiceWorkerRegistration/scope}}</h4>
- The <dfn attribute for="ServiceWorkerRegistration"><code>scope</code></dfn> getter steps are to return the [=ServiceWorkerRegistration/service worker registration=]'s <a lt="URL serializer">serialized</a> [=service worker registration/scope url=].
+ The <dfn attribute for="ServiceWorkerRegistration"><code>scope</code></dfn> getter steps are:
+
+ 1, Let |oldestWorker| be the result of running [=Get Oldest Worker=] passing |registration| as the argument.
+ 1. If |oldestWorker| is null, then:
+ 1. Return null.
This is a backwards incompatible change. Unregistered service worker registrations will now return null when previously they would have returned a scope.
If this is something to worry about, a service worker registration could have a "last relevant scope", which is updated when `.scope` would change, but would also hang around after all the service workers become redundant.
> @@ -601,7 +617,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. If |newestWorker| is null, return [=a promise rejected with=] an "{{InvalidStateError}}" {{DOMException}} and abort these steps.
1. If [=this=]'s [=relevant global object=] |globalObject| is a {{ServiceWorkerGlobalScope}} object, and |globalObject|'s associated [=ServiceWorkerGlobalScope/service worker=]'s <a>state</a> is "`installing`", return [=a promise rejected with=] an "{{InvalidStateError}}" {{DOMException}} and abort these steps.
1. Let |promise| be a <a>promise</a>.
- 1. Let |job| be the result of running <a>Create Job</a> with *update*, |registration|'s [=service worker registration/scope url=], |newestWorker|'s [=service worker/script url=], |promise|, and [=this=]'s <a>relevant settings object</a>.
+ 1. Let |job| be the result of running <a>Create Job</a> with *update*, |registration|'s [=service worker registration/origin=], [=service worker registration/id=], null, |newestWorker|'s [=service worker/script url=], |promise|, and [=this=]'s <a>relevant settings object</a>.
It isn't clear from elsewhere in the spec that a job's scope URL can be null. Maybe make that explicit in Create Job and the Job definition?
You might be able to shortcut some checks by saying "This may only be null if job type is…"
> @@ -615,7 +631,9 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
The <dfn method for="ServiceWorkerRegistration"><code>unregister()</code></dfn> method steps are:
1. Let |promise| be [=a new promise=].
- 1. Let |job| be the result of running [=Create Job=] with *unregister*, the [=service worker registration/scope url=] of the [=ServiceWorkerRegistration/service worker registration=], null, |promise|, and [=this=]'s <a>relevant settings object</a>.
+ 1. Let |newestWorker| be the result of running <a>Get Newest Worker</a> algorithm passing |registration| as its argument.
+ 1. If |newestWorker| is null, return [=a promise rejected with=] an "{{InvalidStateError}}" {{DOMException}} and abort these steps.
When would this happen? If it's only in cases where the registration is already unregistered, we can just return a resolved promise, since the desired outcome happened.
> @@ -615,7 +631,9 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
The <dfn method for="ServiceWorkerRegistration"><code>unregister()</code></dfn> method steps are:
1. Let |promise| be [=a new promise=].
- 1. Let |job| be the result of running [=Create Job=] with *unregister*, the [=service worker registration/scope url=] of the [=ServiceWorkerRegistration/service worker registration=], null, |promise|, and [=this=]'s <a>relevant settings object</a>.
+ 1. Let |newestWorker| be the result of running <a>Get Newest Worker</a> algorithm passing |registration| as its argument.
+ 1. If |newestWorker| is null, return [=a promise rejected with=] an "{{InvalidStateError}}" {{DOMException}} and abort these steps.
+ 1. Let |job| be the result of running [=Create Job=] with *unregister*, the [=service worker registration/origin=], [=service worker registration/id=] of the [=ServiceWorkerRegistration/service worker registration=], null, null, |promise|, and [=this=]'s <a>relevant settings object</a>.
Huh, I see we already had prose that was setting scriptURL to null. Mind adding an "or null" to create job & the job definition for this too?
>
The <dfn method for="ServiceWorkerContainer"><code>register(|scriptURL|, |options|)</code></dfn> method steps are:
1. Let |p| be a <a>promise</a>.
1. Let |client| be [=this=]'s [=ServiceWorkerContainer/service worker client=].
1. Let |scriptURL| be the result of <a lt="URL parser">parsing</a> |scriptURL| with [=this=]'s <a>relevant settings object</a>'s <a>API base URL</a>.
1. Let |scopeURL| be null.
- 1. If |options|["{{RegistrationOptions/scope}}"] [=map/exists=], set |scopeURL| to the result of <a lt="URL parser">parsing</a> |options|["{{RegistrationOptions/scope}}"] with [=this=]'s <a>relevant settings object</a>'s <a>API base URL</a>.
- 1. Invoke [=Start Register=] with |scopeURL|, |scriptURL|, |p|, |client|, |client|'s <a>creation URL</a>, |options|["{{RegistrationOptions/type}}"], and |options|["{{RegistrationOptions/updateViaCache}}"].
+ 1. If |options|["{{RegistrationOptions/scope}}"] is <a>present</a>, set |scopeURL| to the result of <a lt="URL parser">parsing</a> |options|["{{RegistrationOptions/scope}}"] with the [=this=]'s <a>relevant settings object</a>'s <a>API base URL</a>.
```suggestion
1. If |options|["{{RegistrationOptions/scope}}"] [=map/exists=], set |scopeURL| to the result of [=URL parser|parsing=] |options|["{{RegistrationOptions/scope}}"] with [=this=]'s [=relevant settings object=]'s [=API base URL=].
```
> @@ -760,6 +782,20 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. Return |promise|.
</section>
+ <section algorithm="navigator-service-worker-getRegistrationById">
+ <h4 id="navigator-service-worker-getRegistrationById">{{ServiceWorkerContainer/getRegistrationById(id)}}</h4>
+
+ <dfn method for="ServiceWorkerContainer"><code>getRegistrationById(|id|)</code></dfn> method steps are:
+
+ 1. Let |origin| be [=current settings object=]'s [=environment settings object/origin=].
I'd usually get the relevant settings object for "this". That matters when coding across iframes, but maybe it doesn't matter in this case since iframes you can access synchronously must have the same origin (I think?)
Feels like it might be safer to go for the relevant settings object though.
> @@ -760,6 +782,20 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. Return |promise|.
</section>
+ <section algorithm="navigator-service-worker-getRegistrationById">
+ <h4 id="navigator-service-worker-getRegistrationById">{{ServiceWorkerContainer/getRegistrationById(id)}}</h4>
+
+ <dfn method for="ServiceWorkerContainer"><code>getRegistrationById(|id|)</code></dfn> method steps are:
+
+ 1. Let |origin| be [=current settings object=]'s [=environment settings object/origin=].
+ 1. Let |promise| be a new <a>promise</a>.
```suggestion
1. Let |promise| be [=a new promise=].
```
> @@ -760,6 +782,20 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. Return |promise|.
</section>
+ <section algorithm="navigator-service-worker-getRegistrationById">
+ <h4 id="navigator-service-worker-getRegistrationById">{{ServiceWorkerContainer/getRegistrationById(id)}}</h4>
+
+ <dfn method for="ServiceWorkerContainer"><code>getRegistrationById(|id|)</code></dfn> method steps are:
+
+ 1. Let |origin| be [=current settings object=]'s [=environment settings object/origin=].
+ 1. Let |promise| be a new <a>promise</a>.
+ 1. Run the following substeps <a>in parallel</a>
```suggestion
1. Run the following substeps [=in parallel=].
```
I know we're pretty inconsistent in this spec, but the bikeshed shortcuts seem much nicer.
> @@ -760,6 +782,20 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. Return |promise|.
</section>
+ <section algorithm="navigator-service-worker-getRegistrationById">
+ <h4 id="navigator-service-worker-getRegistrationById">{{ServiceWorkerContainer/getRegistrationById(id)}}</h4>
+
+ <dfn method for="ServiceWorkerContainer"><code>getRegistrationById(|id|)</code></dfn> method steps are:
+
+ 1. Let |origin| be [=current settings object=]'s [=environment settings object/origin=].
+ 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 the [=GetRegistration=] algorithm passing |origin| and |id| as the arguments.
```suggestion
1. Let |registration| be the result of running the [=Get Registration=] algorithm passing |origin| and |id| as the arguments.
```
Might be worth looking at bikeshed errors for other linkage errors.
> + A <a>job</a> has an <dfn id="dfn-job-id">origin</dfn>.
+
+ A <a>job</a> has an <dfn id="dfn-job-id">id</dfn>.
```suggestion
A <a>job</a> has an <dfn id="dfn-job-id">origin</dfn> (an [=/origin=]).
A <a>job</a> has an <dfn id="dfn-job-id">id</dfn> (a string).
```
> + 1. [=map/For each=] |key| → |value| of <a>registration map</a>:
+ 1. If |value|'s <a>active worker</a>'s [=service worker/scope url=] [=url/equals=] |scope|, then return |value|.
+ 1. If |value|'s <a>waiting worker</a>'s [=service worker/scope url=] [=url/equals=] |scope|, then return |value|.
+ 1. If |value|'s <a>installing worker</a>'s [=service worker/scope url=] [=url/equals=] |scope|, then return |value|.
```suggestion
1. [=map/For each=] |key| → |registration| of [=registration map=]:
1. [=list/For each=] |serviceWorker| of |registration|'s « [=active worker=], [=waiting worker=], [=installing worker=] »:
1. If |serviceWorker| is not null and |serviceWorker|'s [=service worker/scope url=] [=url/equals=] |scope|, then return |value|.
```
Adding null check, and removing some repetition.
> @@ -2530,7 +2578,8 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. If |scopeURL|'s [=url/scheme=] is not one of "<code>http</code>" and "<code>https</code>", reject |promise| with a <code>TypeError</code> and abort these steps.
1. If any of the strings in |scopeURL|'s [=url/path=] contains either <a>ASCII case-insensitive</a> "<code>%2f</code>" or <a>ASCII case-insensitive</a> "<code>%5c</code>", reject |promise| with a <code>TypeError</code> and abort these steps.
- 1. Let |job| be the result of running [=Create Job=] with *register*, |scopeURL|, |scriptURL|, |promise|, and |client|.
+ 1. If |id| is null, set |id| to |scopeURL|.
+ 1. Let |job| be the result of running [=Create Job=] with *register*, |client|' [=service worker client/origin=], |id|, |scopeURL|, |scriptURL|, |promise|, and |client|.
```suggestion
1. Let |job| be the result of running [=Create Job=] with *register*, |client|'s [=service worker client/origin=], |id|, |scopeURL|, |scriptURL|, |promise|, and |client|.
```
> + 1. Let |scopeURL| be |newest worker|'s [=service worker/scope url=].
+ 1. Else:
+ 1. Assert: |job|'s [=job/job type=] is *register*.
+ 1. Let |scopeURL| be |job|'s [=job/scope url=].
'Variables' in prose are block-scoped, so the definition of *scopeURL* needs to be moved into a parent scope.
> @@ -2797,6 +2862,10 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. If |registration|'s [=active worker=] is not null, then:
1. [=Terminate Service Worker|Terminate=] |registration|'s [=active worker=].
1. Run the [=Update Worker State=] algorithm passing |registration|'s [=active worker=] and "`redundant`" as the arguments.
+ 1. Let |oldUsingClients| be a [=list=] of [=/service worker clients=] who are <a>using</a> |registration|.
+
+ Note: We must get the list of clients prior to clearing the old <a>active worker</a> from |registration|. Otherwise the [=/service worker client=] will not be considered to be <a>using</a> |registration| any more since |registration| will no longer be the <a>containing service worker registration</a>.
Good catch!
> @@ -3314,7 +3387,12 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. Run the following steps atomically.
1. Let |clientURLString| be <a lt="URL serializer">serialized</a> |clientURL|.
1. Let |matchingScopeString| be the empty string.
- 1. Let |scopeStringSet| be the result of [=map/get the keys|getting the keys=] from <a>scope to registration map</a>.
+ 1. Let |scopeMap| be the empty map.
```suggestion
1. Let |scopeMap| be a new [=/ordered map=].
```
I'm never sure what the right prose is here, but using "the" makes me think it's a reference to something that already exists.
As in, the empty map === the empty map. In the same way that the empty string === the empty string. This is fine with strings, but seems weird with maps since they can be mutated.
> : Output
- :: A [=/service worker registration=]
+ :: A [=/service worker registration=] or null.
+
+ 1. Run the following steps atomically.
+ 1. [=map/Get=] (|origin|,|id|) from [=registration map=] and return the result.
```suggestion
1. Return [=registration map=][|origin|/|id|].
```
More readable?
> 1. Return null.
+
+ Note: Only one registration should be associated with a given scope at a time since we fail <a>Register</a> when there is a conflicting registration with the same scope.
```suggestion
Note: Only one registration should be associated with a given scope at a time since we fail [=Register=] when there is a conflicting registration with the same scope.
```
--
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/1526#pullrequestreview-472362670
Received on Friday, 21 August 2020 11:58:25 UTC