Re: [w3c/ServiceWorker] (WIP) Navigation preload (#983)

jungkees requested changes on this pull request.

@jakearchibald, great! Please see my comments.

> +
+    <section algorithm>
+      <h4 id="navigation-preload-manager-disable"><dfn>{{NavigationPreloadManager/disable()}}</dfn></h4>
+
+      The {{NavigationPreloadManager/disable()}} method, when invoked, *must* return a new [=promise=] |promise| and run the following steps [=in parallel=]:
+
+        1. Let |registration| be the [=context object=]'s associated [=/service worker registration=].
+        1. If |registration|'s [=active worker=] is null, [=reject=] |promise| with an "{{InvalidStateError}}" exception, and abort these steps.
+        1. Unset |registration|'s [=navigation preload enabled flag=].
+        1. [=Resolve=] |promise|.
+    </section>
+
+    <section algorithm>
+      <h4 id="navigation-preload-manager-setheadervalue"><dfn>{{NavigationPreloadManager/setHeaderValue(value)}}</dfn></h4>
+
+      The <a method for="NavigationPreloadManager"><code>setHeaderValue(|value|)</code></a> method, when invoked, *must* return [=a new promise=] |promise| and run the following steps [=in parallel=]:

s/[=a new promise=]/a new [=promise=]/

> +
+    <section algorithm>
+      <h4 id="navigation-preload-manager-setheadervalue"><dfn>{{NavigationPreloadManager/setHeaderValue(value)}}</dfn></h4>
+
+      The <a method for="NavigationPreloadManager"><code>setHeaderValue(|value|)</code></a> method, when invoked, *must* return [=a new promise=] |promise| and run the following steps [=in parallel=]:
+
+        1. Let |registration| be the [=context object=]'s associated [=/service worker registration=].
+        1. If |registration|'s [=active worker=] is null, [=reject=] |promise| with an "{{InvalidStateError}}" exception, and abort these steps.
+        1. Set |registration|'s [=navigation preload header value=] to |value|.
+        1. [=Resolve=] |promise|.
+    </section>
+
+    <section algorithm>
+      <h4 id="navigation-preload-manager-getstate"><dfn>{{NavigationPreloadManager/getState()}}</dfn></h4>
+
+      The {{NavigationPreloadManager/getState()}} method, when invoked, *must* return [=a new promise=] |promise| and run the following steps [=in parallel=]:

s/[=a new promise=]/a new [=promise=]/

> @@ -2760,6 +2847,21 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
           1. Set |registration| to the result of running <a>Match Service Worker Registration</a> algorithm passing |request|'s [=request/url=] as the argument.
           1. If |registration| is null or |registration|'s <a>active worker</a> is null, return null.
           1. If |request|'s [=request/destination=] is not {{RequestDestination/"report"}}, set |reservedClient|'s <a>active service worker</a> to |registration|'s <a>active worker</a>.
+          1. If |request| is a [=navigation request=] and |registration|'s [=navigation preload enabled flag=] is set, and |request|'s [=request/method=] is "`GET`", then:

request's method is a byte sequence, so \\\`\<code>GET\</code>\\` would be better.

> @@ -2760,6 +2847,21 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
           1. Set |registration| to the result of running <a>Match Service Worker Registration</a> algorithm passing |request|'s [=request/url=] as the argument.
           1. If |registration| is null or |registration|'s <a>active worker</a> is null, return null.
           1. If |request|'s [=request/destination=] is not {{RequestDestination/"report"}}, set |reservedClient|'s <a>active service worker</a> to |registration|'s <a>active worker</a>.
+          1. If |request| is a [=navigation request=] and |registration|'s [=navigation preload enabled flag=] is set, and |request|'s [=request/method=] is "`GET`", then:
+              1. Let |preloadRequest| be the result of [=request/cloning=] the request |request|

A period is missing.

> @@ -2760,6 +2847,21 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
           1. Set |registration| to the result of running <a>Match Service Worker Registration</a> algorithm passing |request|'s [=request/url=] as the argument.
           1. If |registration| is null or |registration|'s <a>active worker</a> is null, return null.
           1. If |request|'s [=request/destination=] is not {{RequestDestination/"report"}}, set |reservedClient|'s <a>active service worker</a> to |registration|'s <a>active worker</a>.
+          1. If |request| is a [=navigation request=] and |registration|'s [=navigation preload enabled flag=] is set, and |request|'s [=request/method=] is "`GET`", then:
+              1. Let |preloadRequest| be the result of [=request/cloning=] the request |request|
+              1. Let |preloadRequestHeaders| be |preloadRequest|'s [=request/header list=]

A period is missing.

> @@ -2760,6 +2847,21 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
           1. Set |registration| to the result of running <a>Match Service Worker Registration</a> algorithm passing |request|'s [=request/url=] as the argument.
           1. If |registration| is null or |registration|'s <a>active worker</a> is null, return null.
           1. If |request|'s [=request/destination=] is not {{RequestDestination/"report"}}, set |reservedClient|'s <a>active service worker</a> to |registration|'s <a>active worker</a>.
+          1. If |request| is a [=navigation request=] and |registration|'s [=navigation preload enabled flag=] is set, and |request|'s [=request/method=] is "`GET`", then:
+              1. Let |preloadRequest| be the result of [=request/cloning=] the request |request|
+              1. Let |preloadRequestHeaders| be |preloadRequest|'s [=request/header list=]
+              1. Let |preloadResponseObject| be a new {{Response}} object and a new associated {{Headers}} object whose [=guard=] is "`immutable`"

Should be ".. new {{Response}} object associated with a new associated {{Headers}} object ..".

Please add missing periods in this algorithm.

> @@ -2760,6 +2847,21 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
           1. Set |registration| to the result of running <a>Match Service Worker Registration</a> algorithm passing |request|'s [=request/url=] as the argument.
           1. If |registration| is null or |registration|'s <a>active worker</a> is null, return null.
           1. If |request|'s [=request/destination=] is not {{RequestDestination/"report"}}, set |reservedClient|'s <a>active service worker</a> to |registration|'s <a>active worker</a>.
+          1. If |request| is a [=navigation request=] and |registration|'s [=navigation preload enabled flag=] is set, and |request|'s [=request/method=] is "`GET`", then:
+              1. Let |preloadRequest| be the result of [=request/cloning=] the request |request|
+              1. Let |preloadRequestHeaders| be |preloadRequest|'s [=request/header list=]
+              1. Let |preloadResponseObject| be a new {{Response}} object and a new associated {{Headers}} object whose [=guard=] is "`immutable`"
+              1. [=header list/Append=] to |preloadRequestHeaders| a new [=header=] whose [=header/name=] is "`Service-Worker-Navigation-Preload`" and [=header/value=] is |registration|'s [=navigation preload header value=]
+              1. Set |preloadRequest|'s [=skip-service-worker flag=]

It may need changes after https://github.com/w3c/ServiceWorker/pull/1025 and https://github.com/whatwg/fetch/pull/435.

> @@ -2760,6 +2847,21 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
           1. Set |registration| to the result of running <a>Match Service Worker Registration</a> algorithm passing |request|'s [=request/url=] as the argument.
           1. If |registration| is null or |registration|'s <a>active worker</a> is null, return null.
           1. If |request|'s [=request/destination=] is not {{RequestDestination/"report"}}, set |reservedClient|'s <a>active service worker</a> to |registration|'s <a>active worker</a>.
+          1. If |request| is a [=navigation request=] and |registration|'s [=navigation preload enabled flag=] is set, and |request|'s [=request/method=] is "`GET`", then:
+              1. Let |preloadRequest| be the result of [=request/cloning=] the request |request|
+              1. Let |preloadRequestHeaders| be |preloadRequest|'s [=request/header list=]
+              1. Let |preloadResponseObject| be a new {{Response}} object and a new associated {{Headers}} object whose [=guard=] is "`immutable`"
+              1. [=header list/Append=] to |preloadRequestHeaders| a new [=header=] whose [=header/name=] is "`Service-Worker-Navigation-Preload`" and [=header/value=] is |registration|'s [=navigation preload header value=]

A header's [name](https://fetch.spec.whatwg.org/#concept-header-name) and [value](https://fetch.spec.whatwg.org/#concept-header-value) are also byte sequences. So, using backticks without qutotation marks would be more correct.

> @@ -201,6 +201,12 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
 
     A [=/service worker registration=] has one or more <dfn export id="dfn-service-worker-registration-task-queue">task queues</dfn> that back up the <a>tasks</a> from its <a>active worker</a>'s <a>event loop</a>'s corresponding [=/task queues=]. (The target task sources for this back up operation are the <a>handle fetch task source</a> and the <a>handle functional event task source</a>.) The user agent dumps the <a>active worker</a>'s <a>tasks</a> to the [=/service worker registration=]'s [=service worker registration/task queues=] when the <a>active worker</a> is <a lt="terminate service worker">terminated</a> and <a lt="queue a task">re-queues those tasks</a> to the <a>active worker</a>'s <a>event loop</a>'s corresponding [=/task queues=] when the <a>active worker</a> spins off. Unlike the [=/task queues=] owned by <a>event loops</a>, the [=/service worker registration=]'s [=service worker registration/task queues=] are not processed by any <a>event loops</a> in and of it
 self.
 
+    A [=/service worker registration=] has an associated <dfn export>{{NavigationPreloadManager}}</dfn> object.
+
+    A [=/service worker registration=] has an associated <dfn export>navigation preload enabled flag</dfn>. It is initially unset.
+
+    A [=/service worker registration=] has an associated <dfn export>navigation preload header value</dfn>, which is a DOMString. It is initially set to "<code>true</code>".

I think the type should be a [byte sequence](https://infra.spec.whatwg.org/#byte-sequence) instead of a DOMString here. Also the initial value should be \\\`\<code>true\</code>\\`.

> @@ -724,6 +737,71 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
 </section>
 
 <section>
+    <h3 id="navigation-preload-manager">{{NavigationPreloadManager}}</h3>
+
+    <pre class="idl">
+      [SecureContext, Exposed=(Window,Worker)]
+      interface NavigationPreloadManager {
+        Promise&lt;void&gt; enable();
+        Promise&lt;void&gt; disable();
+        Promise&lt;void&gt; setHeaderValue(DOMString value);

The param type should be ByteString instead of DOMString.

-- 
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/983#pullrequestreview-13081791