Re: [whatwg/xhr] Editorial: adopt Fetch's new approach to callbacks (#311)

@domenic approved this pull request.

This looks reasonable, but the dance of having processResponse set a boolean and then waiting for the boolean seems like it'd be painful if it was repeated for every in-parallel fetch in the HTML spec. I'd kind of prefer having Fetch do that for us still...

> @@ -851,7 +836,8 @@ return <a>this</a>'s <a>cross-origin credentials</a>.
     </ol>
     <!-- upload complete flag can never be set here I hope -->
 
-    <p>To <a>process response</a> for <var>response</var>, run these steps:
+   <li>
+    <p>Let <var>processResponse</var>, given a <var>response</var>, be these steps:

Step 2 below has a typo "fo" instead of "for"

Also, most steps after step 1 seem to use "this's response", but step 2 uses "response".

> @@ -851,7 +836,8 @@ return <a>this</a>'s <a>cross-origin credentials</a>.
     </ol>
     <!-- upload complete flag can never be set here I hope -->
 
-    <p>To <a>process response</a> for <var>response</var>, run these steps:
+   <li>
+    <p>Let <var>processResponse</var>, given a <var>response</var>, be these steps:

Some steps use "terminate these steps" to bail, others use "return". Probably "abort these steps" is most common and best?

>     <li>
-    <p>Let <var>response</var> be the result of
-    <a for=/>fetching</a> <var>req</var>.
+    <p>Let <var>processResponse</var>, given a <var>fetchResponse</var>, be these steps:
+
+    <ol>
+     <li><p>Set <var>response</var> to <var>fetchResponse</var>.
+
+     <li><p>Set <var>doesNotBlockTask</var> to true.
+    </ol>
+
+   <li><p><a for=/>Fetch</a> <var>req</var> with <a for=fetch><i>processResponse</i></a>
+   set to <var>processResponse</var> and <a for=fetch><i>useParallelQueue</i></a> set to true.
+
+   <li><p>Wait until either <var>doesNotBlockTask</var> is true or <a>this</a>'s <a>timeout</a> is

This isn't so bad.

I wonder if it should use https://html.spec.whatwg.org/#pause ?

>     <li>
-    <p>Let <var>response</var> be the result of
-    <a for=/>fetching</a> <var>req</var>.
+    <p>Let <var>processResponse</var>, given a <var>fetchResponse</var>, be these steps:
+
+    <ol>
+     <li><p>Set <var>response</var> to <var>fetchResponse</var>.
+
+     <li><p>Set <var>doesNotBlockTask</var> to true.
+    </ol>
+
+   <li><p><a for=/>Fetch</a> <var>req</var> with <a for=fetch><i>processResponse</i></a>
+   set to <var>processResponse</var> and <a for=fetch><i>useParallelQueue</i></a> set to true.
+
+   <li><p>Wait until either <var>doesNotBlockTask</var> is true or <a>this</a>'s <a>timeout</a> is
+   not 0 and <a>this</a>'s <a>timeout</a> milliseconds have passed since now.

I'd prefer saving "now" in a previous step. Although the intention is clear, as written one could interpret this as never happening.

> @@ -930,13 +933,27 @@ return <a>this</a>'s <a>cross-origin credentials</a>.
    is not <a>allowed to use</a> the "<code><a>sync-xhr</a></code>" feature, then run
    <a>handle response end-of-body</a> for <a>this</a> and a <a>network error</a>, and then return.
 
+   <li><p>Let <var>response</var> be a <a for=/>network error</a>.
+
+   <li><p>Let <var>doesNotBlockTask</var> be false.

I find this variable name pretty confusing. Maybe _processedResponse_?

> @@ -825,7 +809,8 @@ return <a>this</a>'s <a>cross-origin credentials</a>.
 
     <p class=note>These steps are only invoked when new bytes are transmitted.
 
-    <p>To <a>process request end-of-body</a> for <var>request</var>, run these steps:
+   <li>
+    <p>Let <var>processRequestEndOfBody</var>, given a <var>request</var>, be these steps:
 
     <ol>
      <li><p>Set <a>this</a>'s <a>upload complete flag</a>.

I've been treating spec steps as "arrow functions" which close over "this". "this" is unique for the entire definition of the method, so this doesn't seem likely to cause confusion.

-- 
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/xhr/pull/311#pullrequestreview-588040137