- From: Anne van Kesteren <notifications@github.com>
- Date: Thu, 03 Aug 2017 08:54:05 +0000 (UTC)
- To: whatwg/fetch <fetch@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <whatwg/fetch/pull/523/review/54024153@github.com>
annevk commented on this pull request.
Looks okay overall, couple of gotchas and many nits.
Do you plan to address the SW issues before landing this or do you want that as follow-up?
> @@ -1196,7 +1196,8 @@ or "<code>worker</code>".
<li>
<p>If <var>read</var> is fulfilled with an object whose <code>done</code> property is false
- and whose <code>value</code> property is a <code>Uint8Array</code> object, then run these steps:
+ and whose <code>value</code> property is a <code>Uint8Array</code> object, then run these steps
+ until fetch is <a for=fetch>terminated</a>:
The problem with this setup is that you don't reach the later "Otherwise" if the fetch is terminated because the initial "If" conditional is already met. So you probably need to create a new block after "then" where inside you first run steps until terminated and then handle the termination if terminated.
> <li>
- <p>Let <var>connection</var> be the result of establishing an HTTP connection to
- <var>origin</var>. [[!HTTP]] [[!HTTP-SEMANTICS]] [[!HTTP-COND]] [[!HTTP-CACHING]] [[!HTTP-AUTH]] [[!TLS]]
+ <p>Run these steps until fetch is <a for=fetch>terminated</a>:</p>
As discussed we should rephrase this here and elsewhere. My suggestion is "Run these steps once and stop if fetch is terminated" but I'm open to something better since it's not exactly great.
> </ol>
+ <li><p>If fetch is <a for=fetch>terminated</a>, providing a <var>reason</var>, return a
then return*
> + <a for=url>scheme</a> is "<code>http</code>"
+ <li><var>request</var>'s <a for=request>current url</a>'s
+ <a for=url>host</a> is a
+ <a for=/>domain</a>
+ <li>Matching <var>request</var>'s <a for=request>current url</a>'s
+ <a for=url>host</a> per
+ <a href=https://tools.ietf.org/html/rfc6797#section-8.2>Known HSTS Host Domain Name
+ Matching</a> results in either a superdomain match with an asserted
+ <code>includeSubDomains</code> directive or a congruent match (with or without an asserted
+ <code>includeSubDomains</code> directive) [[!HSTS]]
+ </ul>
+ <!-- Per Mike West HSTS happens "probably after" Referrer -->
+
+ </ol>
+
+ <li><p>If fetch is <a for=fetch>terminated</a>, providing a <var>reason</var>, return a
then return*
> </ol>
+ <li><p>If fetch is <a for=fetch>terminated</a>, providing a <var>reason</var>, return a
then return*
> @@ -3569,6 +3625,9 @@ Range Requests</cite>. [[HTTP-RANGE]] However, this is not widely supported by b
<li class=XXX><p>Needs testing: multiple `<code>Proxy-Authenticate</code>` headers, missing,
parsing issues.
+ <li><p>If fetch is <a for=fetch>terminated</a>, providing a <var>reason</var>, return a
+ <a>network error</a> with <a for=response>termination reason</a> set to <var>reason</var>.
then return*
But also, this seems to be in a strange place. Why do we have this for 407, but not 401? There's also no enclosing step saying these steps can be terminated.
>
- <p class="note no-backref">The exact determination here is up to user agents for the
- time being. User agents are strongly encouraged to only succeed HTTPS connections with
- strong security properties and return
- <a>network errors</a> otherwise. Using the
- "<code>deprecated</code>" state value ought to be a temporary and last resort kind
- of option.
+ <ol>
+ <li><p>If <var>connection</var> is an HTTP/2 connection, transmit an "<code>RST_STREAM</code>" to
then transmit*
>
- <p class=note>This is a fingerprinting vector.
+ <li><p>If fetch is <a for=fetch>terminated</a>, providing a <var>reason</var>, set
then set*
>
<li>
<p>Run these substeps <a>in parallel</a>:
<ol>
<li>
- <p>Whenever one or more bytes are transmitted from <var>response</var>'s message body, let
- <var>bytes</var> be the transmitted bytes and run these subsubsteps:
- <!-- XXX xref message body -->
+ <p>Loop the following steps, breaking immediately if fetch
+ <a for=fetch lt=terminated>terminates</a>, providing a <var>reason</var>:
"Loop the following steps" -> "While true".
I think we can also drop "immediately" since we always mean immediately (unless we state otherwise).
> </ol>
+
+ <li><p>Set <var>response</var>'s <a lt="termination reason" for=response>termination reason</a>
+ to <var>reason</var>.
Shouldn't this and subsequent steps be wrapped in a conditional of the fetch being terminated?
> @@ -4076,6 +4159,8 @@ Entries may be removed before that moment arrives.
<var>request</var> and <var>response</var>, run these steps:
<ol>
+ <li><p>If <var>response</var> is a <a>network error</a>, return failure.
There's no need for this as a network error is a response without headers so will always fail this check.
> @@ -4387,7 +4472,6 @@ method, when invoked, must run these steps:
<p>The <a>value pairs to iterate over</a> are the return value of running
<a for="header list">sort and combine</a> with the <a for=Headers>header list</a>.
-
Please don't remove this newline.
> @@ -4948,9 +5040,19 @@ constructor must run these steps:
to <var>method</var>.
</ol>
- <li><p>Let <var>r</var> be a new {{Request}} object associated with <var>request</var> and
+ <li><p>If <var>init</var>'s <code>signal</code> member is present, set <var>signal</var> to it.
then set*
> a new associated {{Headers}} object whose <a for=Headers>guard</a>
- is "<code>request</code>".
+ is "<code>request</code>", and a new {{AbortSignal}} object.
+
+ <li>
+ <p>If <var>signal</var> is not null, <a for=AbortSignal lt=add>add the following abort steps</a>
then add*
> @@ -5401,6 +5535,29 @@ method, must run these steps:
<li><p>Return <var>p</var>.
</ol>
+<p>To <dfn>abort fetch</dfn> with a <var>promise</var>, <var>request</var>, and
+<var>responseObject</var>, run these steps:
+
+<ol>
+ <li><p>Let <var>error</var> be an "<code><a exception>AbortError</a></code>" {{DOMException}}.
+
+ <li><p>Reject <var>promise</var> with <var>error</var>.
+
+ <li><p>If <var>request</var>'s <a for=request>body</a> is not null and is
+ <a for=ReadableStream>readable</a>, <a for=ReadableStream>cancel</a> <var>request</var>'s
+ <a for=request>body</a> with <var>error</var>.
+
+ <li><p>If <var>responseObject</var> is null, return.
then return*
> @@ -5401,6 +5535,29 @@ method, must run these steps:
<li><p>Return <var>p</var>.
</ol>
+<p>To <dfn>abort fetch</dfn> with a <var>promise</var>, <var>request</var>, and
+<var>responseObject</var>, run these steps:
+
+<ol>
+ <li><p>Let <var>error</var> be an "<code><a exception>AbortError</a></code>" {{DOMException}}.
+
+ <li><p>Reject <var>promise</var> with <var>error</var>.
+
+ <li><p>If <var>request</var>'s <a for=request>body</a> is not null and is
+ <a for=ReadableStream>readable</a>, <a for=ReadableStream>cancel</a> <var>request</var>'s
then cancel*
> +
+ <li><p>Reject <var>promise</var> with <var>error</var>.
+
+ <li><p>If <var>request</var>'s <a for=request>body</a> is not null and is
+ <a for=ReadableStream>readable</a>, <a for=ReadableStream>cancel</a> <var>request</var>'s
+ <a for=request>body</a> with <var>error</var>.
+
+ <li><p>If <var>responseObject</var> is null, return.
+
+ <li><p>Reject <var>responseObject</var>'s <a for=Response>trailer promise</a> with
+ <var>error</var>.
+
+ <li><p>Let <var>response</var> be <var>responseObject</var>'s <a for=Response>response</a>.
+
+ <li><p>If <var>response</var>'s <a for=response>body</a> is not null and is
+ <a for=ReadableStream>readable</a>, <a for=ReadableStream>cancel</a> <var>response</var>'s
then cancel*
> +
+ <li><p>If <var>request</var>'s <a for=request>body</a> is not null and is
+ <a for=ReadableStream>readable</a>, <a for=ReadableStream>cancel</a> <var>request</var>'s
+ <a for=request>body</a> with <var>error</var>.
+
+ <li><p>If <var>responseObject</var> is null, return.
+
+ <li><p>Reject <var>responseObject</var>'s <a for=Response>trailer promise</a> with
+ <var>error</var>.
+
+ <li><p>Let <var>response</var> be <var>responseObject</var>'s <a for=Response>response</a>.
+
+ <li><p>If <var>response</var>'s <a for=response>body</a> is not null and is
+ <a for=ReadableStream>readable</a>, <a for=ReadableStream>cancel</a> <var>response</var>'s
+ <a for=response>body</a> with <var>error</var>.
+</ol>
Needs newline afterwards. Two newlines before the `<h3>`.
> +
+ <ul class=brief>
+ <li><var>request</var>'s <a for=request>current url</a>'s
+ <a for=url>scheme</a> is "<code>http</code>"
+ <li><var>request</var>'s <a for=request>current url</a>'s
+ <a for=url>host</a> is a
+ <a for=/>domain</a>
+ <li>Matching <var>request</var>'s <a for=request>current url</a>'s
+ <a for=url>host</a> per
+ <a href=https://tools.ietf.org/html/rfc6797#section-8.2>Known HSTS Host Domain Name
+ Matching</a> results in either a superdomain match with an asserted
+ <code>includeSubDomains</code> directive or a congruent match (with or without an asserted
+ <code>includeSubDomains</code> directive) [[!HSTS]]
+ </ul>
+ <!-- Per Mike West HSTS happens "probably after" Referrer -->
+
This newline can be removed.
> @@ -2944,9 +2994,8 @@ optional <i>CORS flag</i> and <i>CORS-preflight flag</i>, run these steps:
<a for=response>url list</a> has more than one item.
</ul>
- <li><p>Execute
- <a href=https://w3c.github.io/webappsec-csp/#set-response-csp-list>set <var>response</var>'s CSP list</a>
- on <var>actualResponse</var>. [[!CSP]]
+ <li><p>Execute <a href=https://w3c.github.io/webappsec-csp/#set-response-csp-list>set
No newlines inside `<a>`.
> + <li><p>If <var>storedResponse</var>'s <a for=response>header list</a>
+ <a for="header list">contains</a> `<code>ETag</code>`, then <a for="header list">append</a>
+ `<code>If-None-Match</code>` with its value to <var>httpRequest</var>'s
+ <a for=request>header list</a>.
+
+ <li><p>If <var>storedResponse</var>'s <a for=response>header list</a>
+ <a for="header list">contains</a> `<code>Last-Modified</code>`, then
+ <a for="header list">append</a> `<code>If-Modified-Since</code>` with its value to
+ <var>httpRequest</var>'s <a for=request>header list</a>.
+ </ol>
+
+ <p class=note>See also the
+ "<a href=https://tools.ietf.org/html/rfc7234#section-4.3.4>Sending a Validation Request</a>"
+ chapter of <cite>HTTP Caching</cite> [[!HTTP-CACHING]].
+
+ <li><p>Otherwise, if the <var>revalidatingFlag</var> is unset, then set <var>response</var> to
This `<li>` needs one less indentation.
>
- <li><p>If <var>connection</var> is not an HTTP/2 connection, <var>request</var>'s
- <a for=request>body</a> is non-null, and <var>request</var>'s <a for=request>body</a>'s
- <a for=body>source</a> is null, then <a for="header list">append</a>
- `<code>Transfer-Encoding</code>`/`<code>chunked</code>` to <var>request</var>'s
- <a for=request>header list</a>.
+ <li>
It seems this is indented one too few? (And then it gets worse.)
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/fetch/pull/523#pullrequestreview-54024153
Received on Thursday, 3 August 2017 08:54:31 UTC