- From: Marijn Kruisselbrink <notifications@github.com>
- Date: Tue, 20 Feb 2024 16:09:21 -0800
- To: w3c/ServiceWorker <ServiceWorker@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <w3c/ServiceWorker/pull/1701/review/1891776594@github.com>
@mkruisselbrink commented on this pull request.
just a couple of hopefully minor points/questions, otherwise this looks good to me.
> @@ -1077,7 +1080,9 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/
};
</pre>
- A {{ServiceWorkerGlobalScope}} object represents the global execution context of a [=/service worker=]. A {{ServiceWorkerGlobalScope}} object has an associated <dfn export for="ServiceWorkerGlobalScope">service worker</dfn> (a [=/service worker=]). A {{ServiceWorkerGlobalScope}} object has an associated <dfn for="ServiceWorkerGlobalScope">force bypass cache for import scripts flag</dfn>. It is initially unset.
+ A {{ServiceWorkerGlobalScope}} object represents the global execution context of a [=/service worker=]. A {{ServiceWorkerGlobalScope}} object has an associated <dfn export for="ServiceWorkerGlobalScope">service worker</dfn> (a [=/service worker=]). A {{ServiceWorkerGlobalScope}} object has an associated <dfn for="ServiceWorkerGlobalScope">force bypass cache for import scripts flag</dfn>. A {{ServiceWorkerGlobalScope}} object has an associated <dfn for="ServiceWorkerGlobalScope">race response map</dfn> which is an [=ordered map=] where the [=map/keys=] are [=/requests=] and the [=map/values=] are [=race response=]. It is initially unset.
nit: could you re-format this so every "A {{ServiceWorkerGlobalScope}} object has" sentence starts on a new line? And maybe even have blank lines between them so they also start on new lines/paragraphs in the generated HTML. I think that might be more readable.
> + DOMString cacheName;
+ };
+
+ enum RunningStatus { "running", "not-running" };
+ enum RouterSourceEnum {
+ "cache",
+ "fetch-event",
+ "network",
+ "race-network-and-fetch-handler"
+ };
+ </pre>
+
+ <section>
+ <h4 id="register-router-method">{{InstallEvent/addRoutes(rules)|event.addRoutes(rules)}}</h4>
+
+ {{InstallEvent/addRoutes(rules)}} registers this service worker the rules to offload simple tasks that the fetch handler does.
This sentence doesn't quite sound right to me. What about:
```suggestion
Note: {{InstallEvent/addRoutes(rules)}} registers rules for this service worker to offload simple tasks that the fetch event handler ordinarily does.
```
> + 1. If |source| is {{RouterSourceEnum/"network"}}:
+ 1. If |shouldSoftUpdate| is true, then [=in parallel=] run the [=Soft Update=] algorithm with |registration|.
+ 1. Return null.
+ 1. Else if |source| is {{RouterSourceEnum/"cache"}}, or |source|["{{RouterSourceDict/cacheName}}"] [=map/exists=], then:
+ 1. If |shouldSoftUpdate| is true, then [=in parallel=] run the [=Soft Update=] algorithm with |registration|.
+ 1. [=map/For each=] |cacheName| → |cache| of the |registration|'s [=service worker registration/storage key=]'s [=name to cache map=].
+ 1. If |source|["{{RouterSourceDict/cacheName}}"] [=map/exists=] and |source|["{{RouterSourceDict/cacheName}}"] [=string/is=] not |cacheName|, [=continue=].
+ 1. Let |requestResponses| be the result of running [=Query Cache=] with |request|, a new {{CacheQueryOptions}}, and |cache|.
+ 1. If |requestResponses| is an empty [=list=], return null.
+ 1. Else:
+ 1. Let |requestResponse| be the first element of |requestResponses|.
+ 1. Let |response| be |requestResponse|'s response.
+ 1. If |activeWorker|'s [=service worker/global object=] is null:
+ 1. If the result of running the [=Setup ServiceWorkerGlobalScope=] algorithm with |activeWorker| is false, then return null.
+
+ Note: If this step succeeds, then |activeWorker|'s [=relevant settings object=] is now ready to use.
My hope was that this wouldn't be modifying |activeWorker| at all. I.e. we'd have something like:
```
1. Let |global object| be |activeWorker|'s global object.
2. If |global object| is null:
1. Set |global object| to the result of running [=Setup ServiceWorkerGlobalScope=]
with |activeWorker|.
3. If |global object| is null:
1. Return null.
// Now use |global object|
```
Perhaps with a "Note: This only creates a ServiceWorkerGlobalScope because CORS checks require that. It is not expected that implementations will actually create a ServiceWorkerGlobalScope here."
> + 1. Else:
+ 1. Let |requestResponse| be the first element of |requestResponses|.
+ 1. Let |response| be |requestResponse|'s response.
+ 1. If |activeWorker|'s [=service worker/global object=] is null:
+ 1. If the result of running the [=Setup ServiceWorkerGlobalScope=] algorithm with |activeWorker| is false, then return null.
+
+ Note: If this step succeeds, then |activeWorker|'s [=relevant settings object=] is now ready to use.
+
+ 1. Let |settingsObject| be |activeWorker|'s [=relevant settings object=].
+ 1. If |response|'s [=response/type=] is "`opaque`", and [=cross-origin resource policy check=] with |settingsObject|'s [=environment settings object/origin=], |settingsObject|, "", and |response|'s [=filtered response/internal response=] returns <b>blocked</b>, then return null.
+ 1. Return |response|.
+ 1. Return null.
+ 1. Else if |source| is {{RouterSourceEnum/"race-network-and-fetch-handler"}}, and |request|'s [=request/method=] is \`<code>GET</code>\` then:
+ 1. Let |queue| be an empty [=queue=] of [=/response=].
+ 1. Let |raceFetchController| be null.
+ 1. Set |raceResponse|'s [=race response/value=] to "<code>pending</code>".
Where did |raceResponse| come from? Should this be `Let |raceResponse| be a [=race response=] whose [=race response/value=] is "<code>pending</code>".`?
> + 1. Let |requestResponse| be the first element of |requestResponses|.
+ 1. Let |response| be |requestResponse|'s response.
+ 1. If |activeWorker|'s [=service worker/global object=] is null:
+ 1. If the result of running the [=Setup ServiceWorkerGlobalScope=] algorithm with |activeWorker| is false, then return null.
+
+ Note: If this step succeeds, then |activeWorker|'s [=relevant settings object=] is now ready to use.
+
+ 1. Let |settingsObject| be |activeWorker|'s [=relevant settings object=].
+ 1. If |response|'s [=response/type=] is "`opaque`", and [=cross-origin resource policy check=] with |settingsObject|'s [=environment settings object/origin=], |settingsObject|, "", and |response|'s [=filtered response/internal response=] returns <b>blocked</b>, then return null.
+ 1. Return |response|.
+ 1. Return null.
+ 1. Else if |source| is {{RouterSourceEnum/"race-network-and-fetch-handler"}}, and |request|'s [=request/method=] is \`<code>GET</code>\` then:
+ 1. Let |queue| be an empty [=queue=] of [=/response=].
+ 1. Let |raceFetchController| be null.
+ 1. Set |raceResponse|'s [=race response/value=] to "<code>pending</code>".
+ 1. Run the following substeps [=in parallel=], but [=abort when=] |controller|'s [=fetch controller/state=] is "<code>terminated</code>" or "<code>aborted</code>":
(mostly an FYI, if https://github.com/w3c/ServiceWorker/pull/1705 lands first, |controller| will need to be updated to |fetchController|. If this PR lands first, I'll need to update the other PR to also change this here).
> + 1. Set |raceFetchController| to the result of calling [=fetch=] given |request|, with [=fetch/processResponse=] set to the following steps given a [=/response=] |raceNetworkRequestResponse|:
+ 1. If |raceNetworkRequestResponse|'s [=response/status=] is [=ok status=], then:
+ 1. Set |raceResponse|'s [=race response/value=] to |raceNetworkRequestResponse|.
+ 1. [=queue/Enqueue=] |raceNetworkRequestResponse| to |queue|.
+ 1. Otherwise, set |raceResponse|'s [=race response/value=] to a [=network error=].
+ 1. [=If aborted=] and |raceFetchController| is not null, then:
+ 1. [=fetch controller/Abort=] |raceFetchController|.
+ 1. Set |raceResponse| to a [=race response=] whose [=race response/value=] is null.
+ 1. Resolve |preloadResponse| with undefined.
+ 1. Run the following substeps [=in parallel=]:
+ 1. Let |fetchHandlerResponse| be the result of [=Create Fetch Event and Dispatch=] with |request|, |registration|, |useHighResPerformanceTimers|, |timingInfo|, |workerRealm|, |reservedClient|, |preloadResponse|, and |raceResponse|.
+ 1. If |fetchHandlerResponse| is not null and not a [=network error=], and |raceFetchController| is not null, [=fetch controller/abort=] |raceFetchController|.
+ 1. [=queue/Enqueue=] |fetchHandlerResponse| to |queue|.
+ 1. Wait until |queue| is not empty.
+ 1. Return the result of [=dequeue=] |queue|.
+ 1. Assert: |source| is "{{RouterSourceEnum/fetch-event}}"
I think this should be un-indented one extra level (to line up with the following steps, which assume that this assertion does indeed hold)
> + :: |preloadResponse|, a [=promise=]
+ :: |raceResponse|, a [=race response=]
+ : Output
+ :: a [=/response=]
+
+ 1. Let |response| be null.
+ 1. Let |eventCanceled| be false.
+ 1. Let |client| be |request|'s [=request/client=].
+ 1. Let |activeWorker| be |registration|'s <a>active worker</a>.
+ 1. Let |eventHandled| be null.
+ 1. Let |handleFetchFailed| be false.
+ 1. Let |respondWithEntered| be false.
+ 1. Let |shouldSoftUpdate| be true if any of the following are true, and false otherwise:
+ * |request| is a [=non-subresource request=].
+ * |request| is a [=subresource request=] and |registration| is [=stale=].
+ 1. Let |raceResponseMap| be |activeWorker|'s [=service worker/global object=]'s [=race response map=].
this should move till after the run Service Worker step below as well to make sure global object has been initialized.
--
Reply to this email directly or view it on GitHub:
https://github.com/w3c/ServiceWorker/pull/1701#pullrequestreview-1891776594
You are receiving this because you are subscribed to this thread.
Message ID: <w3c/ServiceWorker/pull/1701/review/1891776594@github.com>
Received on Wednesday, 21 February 2024 00:09:26 UTC