Re: [whatwg/fetch] Initial resource timing integration. (#1185)

@annevk commented on this pull request.

Although this is largely a bug fix, it's a fairly major one so could you restore the pull request template? I think we'll want to at least point to some existing testing coverage or add some and also file browser bugs if they disagree about certain aspects. (Implementer support seems self-evident.)

> @@ -1150,6 +1209,72 @@ null), run these steps. <var>processBody</var> must be an algorithm accepting a
  <var>reader</var>.
 </ol>
 
+<p>To <dfn export for=response>incrementally read</dfn> a <a for=/>response</a> <var>response</var>,
+given an algorithm <var>processBodyChunk</var>, an algorithm <var>processEndOfBody</var>,
+an algorithm <var>processBodyError</var>, and an optional null,
+<a for=/>parallel queue</a>, or <a for=/>global object</a> <var>taskDestination</var> (default
+null), run these steps.
+<var>processBodyChunk</var> must be an algorithm accepting a <a for=/>byte sequence</a>.
+<var>processEndOfBody</var> must be an algorithm accepting no arguments. <var>processBodyError</var>
+must be an algorithm accepting an exception.
+<ol>
+ <li><p>Let <var>processAndReportEndOfBody</var> be an action the performs the following steps:

```suggestion
 <li>
  <p>Let <var>processAndReportEndOfBody</var> be the following steps:
```
(and similar below)

I don't think we need "an action" anymore for identifying algorithms.

> @@ -1150,6 +1209,72 @@ null), run these steps. <var>processBody</var> must be an algorithm accepting a
  <var>reader</var>.
 </ol>
 
+<p>To <dfn export for=response>incrementally read</dfn> a <a for=/>response</a> <var>response</var>,
+given an algorithm <var>processBodyChunk</var>, an algorithm <var>processEndOfBody</var>,
+an algorithm <var>processBodyError</var>, and an optional null,
+<a for=/>parallel queue</a>, or <a for=/>global object</a> <var>taskDestination</var> (default
+null), run these steps.
+<var>processBodyChunk</var> must be an algorithm accepting a <a for=/>byte sequence</a>.
+<var>processEndOfBody</var> must be an algorithm accepting no arguments. <var>processBodyError</var>
+must be an algorithm accepting an exception.

xref exception here too as you did below?

> +<ol>
+ <li><p>Let <var>processAndReportBody</var> be an action that performs the following steps given
+ <a for=/>byte sequence</a> <var>data</var>:
+ <ol>
+  <li><p><a for=response>Update timing info</a> for <var>response</var>.
+  <li><p>Call <var>processBody</var> with <var>data</var>.
+ </ol>
+
+ <li><p>Let <var>processAndReportBodyError</var> be an action the performs the following steps:
+ <ol>
+  <li><p><a for=response>Update timing info</a> for <var>response</var>.
+  <li><p>Call <var>processBodyError</var>.
+ </ol>
+
+ <li><p>Perform the <a for=body>fully read</a> algorithm given <var>response</var>'s
+ <a for=response>body</a>, <var>taskDestination</var>, <var>processBodyChunk</var>,

No processBodyChunk here, right?

> +<ol>
+ <li><p>Let <var>processAndReportEndOfBody</var> be an action the performs the following steps:
+ <ol>
+  <li><p><a for=response>Update timing info</a> for <var>response</var>.
+  <li><p>Call <var>processEndOfBody</var>.
+ </ol>
+
+ <li><p>Let <var>processAndReportBodyError</var> be an action that performs the following steps
+ given <a for=/>exception</a> <var>exception</var>:
+ <ol>
+  <li><p><a for=response>Update timing info</a> for <var>response</var>.
+  <li><p>Call <var>processBodyError</var> with <var>exception</var>.
+ </ol>
+
+ <li><p>Perform the <a for=body>incrementally read</a> algorithm given <var>response</var>'s
+ <a for=response>body</a>, <var>taskDestination</var>, <var>processBodyChunk</var>,

taskDestination is not the second argument. Haven't double checked the others.

> +  <li><p><a for=response>Update timing info</a> for <var>response</var>.
+  <li><p>Call <var>processBody</var> with <var>data</var>.
+ </ol>
+
+ <li><p>Let <var>processAndReportBodyError</var> be an action the performs the following steps:
+ <ol>
+  <li><p><a for=response>Update timing info</a> for <var>response</var>.
+  <li><p>Call <var>processBodyError</var>.
+ </ol>
+
+ <li><p>Perform the <a for=body>fully read</a> algorithm given <var>response</var>'s
+ <a for=response>body</a>, <var>taskDestination</var>, <var>processBodyChunk</var>,
+ <var>processAndReportBody</var>, and <var>processAndReportBodyError</var>.
+</ol>
+
+<p>To <dfn export lt="fully reading response as promise">fully read response body as promise</dfn>,

I was wondering if perhaps we didn't need to export this and I started looking at callers of "full read body as promise" and now I'm unsure how we would update `fetch()`. I guess we basically have to special case response body reads on the `Body` mixin, unless `fetch()` RT objects are created earlier?

Defining when RT objects are created for `fetch()` (and maybe `XMLHttpRequest`) seems like something we should tackle for the MVP of this.

(Once we figure that out I think it's still worth considering not exporting this (and the body counterpart) until we have an identified consumer.)

> + </ol>
+
+ <li><p>Perform the <a for=body>fully read</a> algorithm given <var>response</var>'s
+ <a for=response>body</a>, <var>taskDestination</var>, <var>processBodyChunk</var>,
+ <var>processAndReportBody</var>, and <var>processAndReportBodyError</var>.
+</ol>
+
+<p>To <dfn export lt="fully reading response as promise">fully read response body as promise</dfn>,
+given a <a for=/>response</a> <var>response</var>, run these steps:
+
+<ol>
+ <li><p>Let <var>promise</var> be the result of
+ <a for=/>fully reading response as promise</a> with <var>response</var>'s
+ <a for=response>body</a>.
+
+ <li><p><a for=/>Upon fulfillment</a> of <var>promise</var>, <a for=response>update timing info</a>

I think this should be react as we also want to update these upon rejection, no?

> @@ -2173,8 +2305,9 @@ false), and an optional boolean <dfn export for="obtain a connection"><var>dedic
   <ol>
    <li>
     <p>Set <var>connection</var> to the result of establishing an HTTP connection to
-    <var>origin</var>. [[!HTTP]] [[!HTTP-SEMANTICS]] [[!HTTP-COND]] [[!HTTP-CACHING]] [[!HTTP-AUTH]]
-    [[!TLS]]
+    <var>origin</var>, following the requirements for
+    <a>recording <var>connection</var> timing info</a>.

```suggestion
    <a>recording connection timing info</a> given <var>connection</var>.
```

> @@ -2213,6 +2346,88 @@ clearly stipulates that <a>connections</a> are keyed on
 <!-- See https://github.com/whatwg/fetch/issues/114#issuecomment-143500095 for when we make
      WebSocket saner -->
 
+<p>The requirements for <dfn>recording connection timing info</dfn> given a <var>connection</var>

```suggestion
<p>The requirements for <dfn>recording connection timing info</dfn> given a <a for=/>connection</a> <var>connection</var>
```
+ rewrapping

> @@ -2213,6 +2346,88 @@ clearly stipulates that <a>connections</a> are keyed on
 <!-- See https://github.com/whatwg/fetch/issues/114#issuecomment-143500095 for when we make
      WebSocket saner -->
 
+<p>The requirements for <dfn>recording connection timing info</dfn> given a <var>connection</var>
+and its <a for=/>connection timing info</a> <var>timingInfo</var>, are as follows:

I find this a bit confusing as it's not an argument, but I don't really have a good suggestion for how to address it either. 😟

> @@ -3775,6 +3998,55 @@ steps:
  <!-- This is really bad and needs to be handled differently at some point. -->
 </ol>
 
+<p>To <dfn for=response>Initialize timing info</dfn>, given <a for=/>response</a>
+<var>response</var> and <a for=/>fetch params</a> <var>fetchParams</var>, perform the following
+steps:
+<ol>
+ <li><p>Let <var>timingInfo</var> be <var>fetchParams</var>'s <a for="fetch params">timing info</a>.
+
+ <li><p>Let <var>redirectTimingList</var> be <var>fetchParams</var>'s
+ <a for="fetch params">redirect timing info list</a>.
+
+ <li><p>Let <var>body</var> be <var>response</var>'s <a for=response>body</a>.
+
+ <li><p>Let <var>stream</var> be null.
+
+ <li><p>If <var>body</var> is null, then set <var>stream</var> to <var>body</var>'s
+ <a for=body>stream</a>.

If body is null it cannot have a member, right?

> @@ -3898,20 +4170,32 @@ these steps:
 
  <li><p>Let <var>actualResponse</var> be null.
 
+ <li><p>Let <var>timingInfo</var> be <var>fetchParams</var>'s <a for="fetch params">timing info</a>

```suggestion
 <li><p>Let <var>timingInfo</var> be <var>fetchParams</var>'s <a for="fetch params">timing info</a>.
```

>     <li><p>Set <var>response</var> to the result of invoking <a for=/>handle fetch</a> for
    <var>requestForServiceWorker</var>. [[!HTML]] [[!SW]]
 
    <li>
     <p>If <var>response</var> is not null, then:
 
     <ol>
+     <li><p>Set <var>fetchParams</var>'s <a for="fetch params">timing info</a>'s
+     <a for="fetch timing info">response start time</a> to the <a for=/>unsafe shared current time</a>.
+
+     <li><p>Set <var>timingInfo</var>'s <a for="fetch timing info">encoded body size</a> and
+     <a for="fetch timing info">decoded body size</a> to <var>response</var>'s
+     `<code>Content-Length</code>` <a for=/>header</a>.

You want https://fetch.spec.whatwg.org/#header-list-extract-a-length here, right? (Similar elsewhere.)

Should we also set this field for service worker responses? Some tests would be good here.

> +<ol>
+ <li><p>Let <var>processAndReportBody</var> be an action that performs the following steps given
+ <a for=/>byte sequence</a> <var>data</var>:
+ <ol>
+  <li><p><a for=response>Update timing info</a> for <var>response</var>.
+  <li><p>Call <var>processBody</var> with <var>data</var>.
+ </ol>
+
+ <li><p>Let <var>processAndReportBodyError</var> be an action the performs the following steps:
+ <ol>
+  <li><p><a for=response>Update timing info</a> for <var>response</var>.
+  <li><p>Call <var>processBodyError</var>.
+ </ol>
+
+ <li><p>Perform the <a for=body>fully read</a> algorithm given <var>response</var>'s
+ <a for=response>body</a>, <var>taskDestination</var>, <var>processBodyChunk</var>,

Also, taskDestination is in the wrong place.

-- 
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/1185#pullrequestreview-613108338