Navigation preload #983

jakearchibald · 2016-10-05T16:10:18Z

Needs review.

jakearchibald · 2016-10-05T16:10:59Z

docs/index.bs

+        text: append to header list; url: concept-header-list-append
        text: basic filtered response; url: concept-filtered-response-basic
        text: cancel a ReadableStream; url: concept-cancel-readablestream
+        text: cloning the request; url: concept-request-clone


I get the feeling this is the wrong way to do this, but I'm not sure what the right way is

It is. You want <a for=request>clone</a> and don't add this link at all. This whole list of terms seems basically wrong...

jakearchibald · 2016-10-05T16:13:06Z

docs/index.bs

+                    <p><a>Fetch</a> <var>preloadRequest</var>.</p>
+                    <p>To <a>process response</a> for <var>preloadResponse</var>, run these substeps:</p>
+                    <ol>
+                      <li><p>If <var>preloadResponse</var>'s <a>type</a> is "<code>error</code>", reject <var>navigationPreload</var> with a <code>TypeError</code> and terminate these substeps.</li>


type isn't linking up properly, but I'm not sure what the right way to do this is.

jakearchibald · 2016-10-05T16:16:51Z

docs/index.bs

          <li>Set <var>registration</var> to the result of running <a href="#scope-match-algorithm">Match Service Worker Registration</a> algorithm passing <var>request</var>'s <a for="request">url</a> as the argument.</li>
          <li>If <var>registration</var> is null or <var>registration</var>'s <a href="#dfn-active-worker">active worker</a> is null, return null.</li>
          <li>Set <var>client</var>'s <a href="#dfn-service-worker-client-active-worker">active worker</a> to <var>registration</var>'s <a href="#dfn-active-worker">active worker</a>.</li>
+          <li>If <var>request</var> is a <a>navigation request</a> and <var>registration</var>'s <a>navigation preload enabled flag</a> is set, and <var>request</var>'s <a>method</a> is `<code>GET</code>`, then:


@annevk this is the section I'm least confident in - it's fetch for the preload. I don't think redirects are a problem since the mode will be manual. When the redirect is sent back to the navigate algorithm it'll be associated with the original request, which won't have the header.

Having said that, that may be unexpected for same-origin redirects.

It's likely not a problem. Should we prefix the header so that it cannot be set through script?

mfalken · 2016-10-07T04:38:35Z

docs/index.bs

+    <pre class="idl" id="navigation-preload-state">
+      dictionary NavigationPreloadState {
+        boolean enabled = false;
+        DOMString headerValue;


Should this be ByteString? The Fetch and XHR specs are using ByteString for HTTP header values.

Yea, it should be

annevk · 2016-10-31T10:10:06Z

docs/index.bs

+                <ol>
+                  <li>
+                    <p><a>Fetch</a> <var>preloadRequest</var>.</p>
+                    <p>To <a>process response</a> for <var>navigationPreloadResponse</var>, run these substeps:</p>


This variable seems to come out of nowhere...

I was using https://fetch.spec.whatwg.org/#fetch-method has a guide, and response seems to come out of nowhere there too. What's the right thing to do here, and why is fetch right to do it this way?

Ah right, I guess this is fine. I should refactor this at some point to make it more clear...

annevk · 2016-10-31T10:11:49Z

docs/index.bs

      [Constructor(DOMString type, FetchEventInit eventInitDict), Exposed=ServiceWorker]
      interface FetchEvent : ExtendableEvent {
        [SameObject] readonly attribute Request request;
+        readonly attribute Promise&lt;Response?&gt; preloadResponse;


This is wrong if you want to resolve it with undefined.

I'm trying to show that preloadResponse is a promise that resolves with a response or undefined. What's the right IDL in this case? "any" seems less informative.

any is the only correct option here. I recommend complaining in the IDL repository. Perhaps someone can up priority of fixing that.

annevk · 2016-10-31T10:12:01Z

docs/index.bs

    <pre class="idl" id="fetch-event-init-dictionary">
      dictionary FetchEventInit : ExtendableEventInit {
        required Request request;
+        required Promise&lt;Response?&gt; preloadResponse;


Why would you allow null as a value?

Again the intent is for this to be a promise that resolves with a response or undefined.

jakearchibald · 2016-12-14T11:03:10Z

@jungkees I'm happy with this now. Could you review?

jungkees

@jakearchibald, great! Please see my comments.

jungkees · 2016-12-15T08:40:00Z

docs/index.bs

+
+    <section algorithm>
+      <h4 id="navigation-preload-manager-setheadervalue"><dfn>{{NavigationPreloadManager/setHeaderValue(value)}}</dfn></h4>
+


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

I thought the same, but it's "a new promise".

Ah.. That's right. I got it.

jungkees · 2016-12-15T08:46:03Z

docs/index.bs

+
+    <section algorithm>
+      <h4 id="navigation-preload-manager-getstate"><dfn>{{NavigationPreloadManager/getState()}}</dfn></h4>
+


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

jungkees · 2016-12-15T10:17:40Z

docs/index.bs

          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.

jungkees · 2016-12-15T10:20:36Z

docs/index.bs

          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.

jungkees · 2016-12-15T10:20:42Z

docs/index.bs

          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.

jungkees · 2016-12-15T10:30:59Z

docs/index.bs

+          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.

I think this one hasn't been addressed yet? I missed that the value of the guard is just a string, so it seems it should be "<code>immutable</code>".

@jakearchibald, I was confused with the markdown syntax here. If "`immutable`" works fine, that'd be good as-is.

jungkees · 2016-12-15T10:33:47Z

docs/index.bs

+              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 #1025 and whatwg/fetch#435.

jungkees · 2016-12-15T11:10:23Z

docs/index.bs

+              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 and value are also byte sequences. So, using backticks without qutotation marks would be more correct.

jungkees · 2016-12-15T11:12:18Z

docs/index.bs

+
+    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 instead of a DOMString here. Also the initial value should be \`<code>true</code>\`.

jungkees · 2016-12-15T11:13:36Z

docs/index.bs

+      [SecureContext, Exposed=(Window,Worker)]
+      interface NavigationPreloadManager {
+        Promise&lt;void&gt; enable();
+        Promise&lt;void&gt; disable();


The param type should be ByteString instead of DOMString.

Damnit it @mattto mentioned this too - sorry for forgetting

jungkees · 2016-12-16T01:24:26Z

docs/index.bs

+
+        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 enabled flag=].


This should resolve with undefined I guess.

jakearchibald · 2016-12-16T11:12:34Z

@jungkees excellent review, thank you. I've updated the branch.

jungkees · 2016-12-16T14:01:48Z

@jakearchibald, LGTM with the following points!

https://github.com/w3c/ServiceWorker/pull/983/files#diff-27b79860afe28f01aed4f1f6228367faR2853
- It seems the comment for this line hasn't been addressed yet
https://github.com/w3c/ServiceWorker/pull/983/files/1e58712f505366a5c260213e297d1ead43f5874a..245b62e45dc0cef0d2f30e6fb1fdec9b1fbc7c4b#diff-27b79860afe28f01aed4f1f6228367faR2855
- Maybe you can land this with Changing skip-service-worker flag to use-service-workers enum whatwg/fetch#435 (to reference service-workers mode.) Or just keep the current text (skip-service-worker flag) for this PR and change this line in Opt cache.add/addAll and importScripts out of a local service worker #1025.

Please include the corresponding html file when you merge it. Thank you!

jakearchibald · 2016-12-16T14:15:34Z

@jungkees the guard setting is correct I think. "`foo`" is markdown shorthand for "<code>foo</code>".

I'll revert the "service-workers mode" change before I merge.

jungkees · 2016-12-16T14:34:37Z

@jakearchibald, with the first bullet point in the previous comment, I tried to say the following sentence might have to be changed,
From:
"Let preloadResponseObject be a new Response object and a new associated Headers object whose guard is "immutable"."
To:
"Let preloadResponseObject be a new Response object associated with a new Headers object whose guard is "immutable"."

jungkees · 2016-12-16T15:21:10Z

@jakearchibald, sorry I found some places that need editorial changes just now. Noticed them through reviewing the html output. Addressed them with this follow-up commit: a6f6a32.

jakearchibald commented Oct 5, 2016

View reviewed changes

mfalken reviewed Oct 7, 2016

View reviewed changes

jakearchibald mentioned this pull request Oct 28, 2016

Making a concurrent request for navigations #920

Closed

annevk reviewed Oct 31, 2016

View reviewed changes

jakearchibald force-pushed the navigation-preload branch from e185219 to adc38d8 Compare December 2, 2016 12:10

jakearchibald requested a review from jungkees December 14, 2016 11:02

jungkees requested changes Dec 15, 2016

View reviewed changes

jungkees reviewed Dec 16, 2016

View reviewed changes

jungkees approved these changes Dec 16, 2016

View reviewed changes

jakearchibald added 6 commits December 16, 2016 14:18

Rebasing on new, cleaner spec.

8fd436f

Fixing linking & formatting bugs

4b34e0a

Invalid state error is more consistent with the rest of the spec

f8c4eab

Following review feeback

a2c97de

Service workers mode

dba2f0a

Reverting skip-sw-flag change

d284f4c

jakearchibald force-pushed the navigation-preload branch from 4194b32 to d284f4c Compare December 16, 2016 14:20

jakearchibald merged commit fbc0faa into master Dec 16, 2016

jakearchibald deleted the navigation-preload branch December 16, 2016 14:22

jungkees changed the title ~~(WIP) Navigation preload~~ Navigation preload Dec 16, 2016

KenjiBaheux mentioned this pull request Apr 7, 2017

Navigation Preload for Service Worker w3ctag/design-reviews#166

Closed

3 tasks


		<section algorithm>
		<h4 id="navigation-preload-manager-setheadervalue"><dfn>{{NavigationPreloadManager/setHeaderValue(value)}}</dfn></h4>


		<section algorithm>
		<h4 id="navigation-preload-manager-getstate"><dfn>{{NavigationPreloadManager/getState()}}</dfn></h4>


		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>".

Navigation preload #983

Navigation preload #983

Uh oh!

Conversation

jakearchibald commented Oct 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakearchibald commented Dec 14, 2016

Uh oh!

jungkees left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakearchibald Dec 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakearchibald commented Dec 16, 2016

Uh oh!

jungkees commented Dec 16, 2016

Uh oh!

jakearchibald commented Dec 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

jakearchibald Dec 16, 2016 •

edited

Loading

jakearchibald commented Dec 16, 2016 •

edited

Loading

jungkees commented Dec 16, 2016 •

edited

Loading