Re: [w3c/ServiceWorker] ServiceWorkerAutoPreload (PR #1756)

@youennf commented on this pull request.

Looks ok overall to me, some thoughts inline

> +              1. Set |preloadRequest|'s [=service-workers mode=] to "`none`".
+              1. Let |preloadFetchController| be null.
+              1. Run the following substeps [=in parallel=], but [=abort when=] |fetchController|'s [=fetch controller/state=] is "<code>terminated</code>" or "<code>aborted</code>":
+                  1. Set |preloadFetchController| to the result of [=Fetch|fetching=] |preloadRequest|.
+
+                      To [=fetch/processResponse=] for |navigationPreloadResponse|, run these substeps:
+
+                      1. If |navigationPreloadResponse|'s [=response/type=] is "`error`", reject |preloadResponse| with a `TypeError` and terminate these substeps.
+                      1. Associate |preloadResponseObject| with |navigationPreloadResponse|.
+                      1. Resolve |preloadResponse| with |preloadResponseObject|.
+              1. [=If aborted=], then:
+                  1. Let |deserializedError| be the result of [=deserialize a serialized abort reason=] given null and |workerRealm|.
+                  1. [=fetch controller/Abort=] |preloadFetchController| with |deserializedError|.
+          1. Else if |timingInfo|'s [=service worker timing info/worker matched router source=] is not "{{RouterSourceEnum/fetch-event}}", a user agent may run the following substeps:
+
+              Note: A user agent may speculatively dispatch a network request in parallel with creating a fetch event in order to minimize the bootstrap cost.

Is there sufficient description on when to do this speculative load?

> +
+                      1. If |navigationPreloadResponse|'s [=response/type=] is "`error`", reject |preloadResponse| with a `TypeError` and terminate these substeps.
+                      1. Associate |preloadResponseObject| with |navigationPreloadResponse|.
+                      1. Resolve |preloadResponse| with |preloadResponseObject|.
+              1. [=If aborted=], then:
+                  1. Let |deserializedError| be the result of [=deserialize a serialized abort reason=] given null and |workerRealm|.
+                  1. [=fetch controller/Abort=] |preloadFetchController| with |deserializedError|.
+          1. Else if |timingInfo|'s [=service worker timing info/worker matched router source=] is not "{{RouterSourceEnum/fetch-event}}", a user agent may run the following substeps:
+
+              Note: A user agent may speculatively dispatch a network request in parallel with creating a fetch event in order to minimize the bootstrap cost.
+
+              1. Let |autoPreloadFetchController| be null.
+              1. Set |responseForAutoPreload| to be a [=race response=] whose [=race response/value=] is "<code>pending</code>".
+              1. Run the following substeps [=in parallel=], but [=abort when=] |fetchController|'s [=fetch controller/state=] is "<code>terminated</code>" or "<code>aborted</code>":
+                  1. Set |autoPreloadFetchController| to the result of calling [=fetch=] given |request|, with [=fetch/processResponse=] set to the following steps given a [=/response=] |autoPreloadRequestResponse|:
+                  1. If |autoPreloadRequestResponse|'s [=response/status=] is [=ok status=], set |responseForAutoPreload|'s [=race response/value=] to |autoPreloadRequestResponse|.

Why only using the auto preload response if it is a 2XX response?
What happens if the auto preload response is not a 2XX (I haven't yet looked precisely at how the race response mechanism works)?

>        1. Else, resolve |preloadResponse| with undefined.
-      1. Let |fetchResult| be the result of [=Create Fetch Event and Dispatch=] with |request|, |registration|, |useHighResPerformanceTimers|, |timingInfo|, |workerRealm|, |reservedClient|, |preloadResponse|, and null.
+      1. Let |fetchResult| be the result of [=Create Fetch Event and Dispatch=] with |request|, |registration|, |useHighResPerformanceTimers|, |timingInfo|, |workerRealm|, |reservedClient|, |preloadResponse|, and |responseForAutoPreload|.

In the current WebKit implementation, if service worker does not call respondWith, the process doing networking is notified and it will reuse the responseForAutoPreload directly.
AIUI, the mechanism as described here will likely reuse the response in the renderer process, which might a bit slower. I am not sure whether this is observable so maybe the spec is fine as is.

> +              1. Let |preloadResponseObject| be a new {{Response}} object associated with a new {{Headers}} object whose [=guard=] is "`immutable`".
+              1. [=header list/Append=] to |preloadRequestHeaders| a new [=header=] whose [=header/name=] is \`<code>Service-Worker-Navigation-Preload</code>\` and [=header/value=] is |registration|'s [=navigation preload header value=].
+              1. Set |preloadRequest|'s [=service-workers mode=] to "`none`".
+              1. Let |preloadFetchController| be null.
+              1. Run the following substeps [=in parallel=], but [=abort when=] |fetchController|'s [=fetch controller/state=] is "<code>terminated</code>" or "<code>aborted</code>":
+                  1. Set |preloadFetchController| to the result of [=Fetch|fetching=] |preloadRequest|.
+
+                      To [=fetch/processResponse=] for |navigationPreloadResponse|, run these substeps:
+
+                      1. If |navigationPreloadResponse|'s [=response/type=] is "`error`", reject |preloadResponse| with a `TypeError` and terminate these substeps.
+                      1. Associate |preloadResponseObject| with |navigationPreloadResponse|.
+                      1. Resolve |preloadResponse| with |preloadResponseObject|.
+              1. [=If aborted=], then:
+                  1. Let |deserializedError| be the result of [=deserialize a serialized abort reason=] given null and |workerRealm|.
+                  1. [=fetch controller/Abort=] |preloadFetchController| with |deserializedError|.
+          1. Else if |timingInfo|'s [=service worker timing info/worker matched router source=] is not "{{RouterSourceEnum/fetch-event}}", a user agent may run the following substeps:

Why doing autopreload if the route is NOT `fetch-event`?
I would expect to only do autopreload if the route is actually `fetch-event`.

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

Message ID: <w3c/ServiceWorker/pull/1756/review/2938570052@github.com>