Re: [whatwg/fetch] Attach timing info and URL to network errors, and report for fetch API (#1311)

@annevk commented on this pull request.

I like the idea of using a task to synchronize. Nice. Couple of remaining nits and it would be really great if we could secure at least one other reviewer.

> @@ -4124,6 +4130,27 @@ steps:
 <a for=/>response</a> <var>response</var>, run these steps:
 
 <ol>
+ <li>
+  <p>If <var>response</var> is a <a>network error</a>, then:
+
+  <ol>
+   <li>
+    <p>Set <var>response</var>'s <a for=response>URL list</a> to « <var>fetchParams</var>'s
+    <a for="fetch params">request</a>'s <a for=request>URL list</a>[0]</p>.

This lacks a ».

>   <li><p>Set <var>fetchParams</var>'s <a for="fetch params">request</a>'s
  <a for=request>done flag</a>.
 
  <li><p>If <var>fetchParams</var>'s <a for="fetch params">process response done</a> is not null,
- then <a>queue a fetch task</a> to run <var>fetchParams</var>'s
- <a for="fetch params">process response done</a> given <var>response</var>,
- with <var>fetchParams</var>'s <a for="fetch params">task destination</a>.
+ then run <var>fetchParams</var>'s <a for="fetch params">process response done</a> with
+ <var>response</var>.
+</ol>
+
+<p>To <dfn>finalize network response</dfn> given a <a for=/>fetch params</a> <var>fetchParams</var>
+and a <a for=/>response</a> <var>response</var>, <a>queue a fetch task</a>
+with to run the following steps with <var>fetchParams</var>'s
+<a for="fetch params">task destination</a>:</p>
+
+<ol>
+  <li><p>Set <var>fetchParams</var>'s <a for=response>network read complete flag</a>.</p></li>

Nit: indentation.

>  </ol>
 
 <p>To <dfn>finalize response</dfn> given a <a for=/>fetch params</a> <var>fetchParams</var> and a
-<a for=/>response</a> <var>response</var>, run these steps:
+<a for=/>response</a> <var>response</var>, run these steps:</p>

Nit: remove closing tag.

>  
 <ol>
+ <li><p>Assert: these steps are running as part of <var>fetchParams</var>'s
+ <a for="fetch params">task destination</a>.</p></li>

I'm not sure this makes sense. I guess let's remove it for now?

> @@ -2035,6 +2035,12 @@ description of the attack.
 <dfn for=response id=concept-response-timing-allow-passed>timing allow passed flag</dfn>, which is
 initially unset.
 
+<p>A <a for=/>response</a> has an associated
+<dfn for=response>network read complete flag</dfn>, which is initially unset.
+
+<p>A <a for=/>response</a> has an associated
+<dfn for=response>ready for clients flag</dfn>, which is initially unset.

Why store these on response and not fetch params?

>   <li><p>Set <var>fetchParams</var>'s <a for="fetch params">request</a>'s
  <a for=request>done flag</a>.
 
  <li><p>If <var>fetchParams</var>'s <a for="fetch params">process response done</a> is not null,
- then <a>queue a fetch task</a> to run <var>fetchParams</var>'s
- <a for="fetch params">process response done</a> given <var>response</var>,
- with <var>fetchParams</var>'s <a for="fetch params">task destination</a>.
+ then run <var>fetchParams</var>'s <a for="fetch params">process response done</a> with
+ <var>response</var>.
+</ol>
+
+<p>To <dfn>finalize network response</dfn> given a <a for=/>fetch params</a> <var>fetchParams</var>

Let's call this "fetch network finale" and put it before "finalize response".

-- 
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/1311#pullrequestreview-778495695