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

@annevk commented on this pull request.



> +
+<p>To <dfn>clamp connection timing to fetch timing</dfn>, given <a for=/>connection timing info</a>
+<var>timingInfo</var> and <a typedef for=/>DOMHighResTimeStamp</a> <var>defaultStartTime</var>, run these
+steps:
+<ol>
+ <li><p>If <var>timingInfo</var>'s <a for="connection timing info">connection start time</a> is
+ greater than <var>defaultStartTime</var>, then return <var>timingInfo</var>.
+
+ <li><p>Otherwise, return a new <a for=/>connection timing info</a>, with
+ <a for="connection timing info">domain lookup start time</a> set to <var>defaultStartTime</var>,
+ <a for="connection timing info">domain lookup end time</a> set to <var>defaultStartTime</var>,
+ <a for="connection timing info">connection start time</a> set to <var>defaultStartTime</var>,
+ <a for="connection timing info">connection end time</a> set to <var>defaultStartTime</var>,
+ <a for="connection timing info">secure connection start time</a> set to
+ <var>defaultStartTime</var>, and <a for="connection timing info">alpn negotiated protocol</a>
+ set to <var>timingInfo</var>'s <a for="connection timing info">alpn negotiated protocol</a>.

This is missing a closing `</ol>` it seems.

> +  <dd>A <a typedef for=/>DOMHighResTimeStamp</a>
+  <dt><dfn export for="connection timing info">alpn negotiated protocol</dfn> (default empty string)
+  <dd>A string.
+</dl>
+
+<p class=note>Note that timestamps in this spec are usually unsafe, and are meant to be coarsened
+and normalized to a <a for=/>global object</a> prior to being exposed.
+
+<p>To <dfn>clamp connection timing to fetch timing</dfn>, given <a for=/>connection timing info</a>
+<var>timingInfo</var> and <a typedef for=/>DOMHighResTimeStamp</a> <var>defaultStartTime</var>, run these
+steps:
+<ol>
+ <li><p>If <var>timingInfo</var>'s <a for="connection timing info">connection start time</a> is
+ greater than <var>defaultStartTime</var>, then return <var>timingInfo</var>.
+
+ <li><p>Otherwise, return a new <a for=/>connection timing info</a>, with

No need for `Otherwise` if you do early returns.

> @@ -2213,6 +2281,90 @@ 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 export id="concept-record-connection-timing-info">recording connection

This shouldn't be exported. Also doesn't need an ID (it'll get one assigned automatically).

> @@ -3775,6 +3932,75 @@ steps:
  <!-- This is really bad and needs to be handled differently at some point. -->
 </ol>
 
+<p>To <dfn for=response>Attach timing info</dfn>, given <a for=/>response</a>

Lowercase attach. Can probably leave out `for=response` here given it's an internal algorithm.

> @@ -3775,6 +3932,75 @@ steps:
  <!-- This is really bad and needs to be handled differently at some point. -->
 </ol>
 
+<p>To <dfn for=response>Attach 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>.

This doesn't appear to be needed?

> +<ol>
+ <li><p>Let <var>timingInfo</var> be <var>response</var>'s <a for=response>timing info</a>.
+
+ <li><p>If <var>timingInfo</var> is null, return null.
+
+ <li><p>Let <var>startTime</var> be <var>timingInfo</var>'s
+ <a for="fetch timing info">fetch start time</a>.
+
+ <li><p>If <var>timingInfo</var>'s <a for="fetch timing info">redirect start time</a> is not zero,
+ then set <var>startTime</var> to <var>timingInfo</var>'s
+ <a for="fetch timing info">redirect start time</a>.
+
+ <li><p>If <var>response</var>'s <a for=response>timing allow passed flag</a> is not set, then
+ set <var>timingInfo</var> to a new <a for=/>fetch timing info</a>, with its
+ <a for="fetch timing info">start time</a>
+ <a for="fetch timing info">fetch start time</a> set to <var>startTime</var>.

Does this mean to set two fields?

> + <li><p>If <var>timingInfo</var> is null, return null.
+
+ <li><p>Let <var>startTime</var> be <var>timingInfo</var>'s
+ <a for="fetch timing info">fetch start time</a>.
+
+ <li><p>If <var>timingInfo</var>'s <a for="fetch timing info">redirect start time</a> is not zero,
+ then set <var>startTime</var> to <var>timingInfo</var>'s
+ <a for="fetch timing info">redirect start time</a>.
+
+ <li><p>If <var>response</var>'s <a for=response>timing allow passed flag</a> is not set, then
+ set <var>timingInfo</var> to a new <a for=/>fetch timing info</a>, with its
+ <a for="fetch timing info">start time</a>
+ <a for="fetch timing info">fetch start time</a> set to <var>startTime</var>.
+
+ <li><p>Otherwise, <var>timingInfo</var>'s <a for="fetch timing info">start time</a> to
+ <var>startTime</var>.

I think this is missing "set".

> + <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>Set <var>response</var>'s <a for=response>timing info</a> to <var>fetchParams</var>'s
+ <a for="fetch params">timing info</a>.
+
+ <li><p>If <var>redirectTimingList</var> is not empty, set <var>timingInfo</var>'s
+ <a for="fetch timing info">redirect start time</a> to <var>redirectTimingList</var>'s first item's
+ <a for="fetch timing info">fetch start time</a> and
+ <a for="fetch timing info">redirect end time</a> to <var>redirectTimingList</var>'s last item's
+ <a for="fetch timing info">fetch start time</a>.
+</ol>
+
+<p>To <dfn export for=response>retrieve timing info</dfn>, given <a for=/>response</a> <var>response</var>,

It seems weird for something called retrieve to also manipulate.

> + <li><p>If <var>timingInfo</var>'s <a for="fetch timing info">redirect start time</a> is not zero,
+ then set <var>startTime</var> to <var>timingInfo</var>'s
+ <a for="fetch timing info">redirect start time</a>.
+
+ <li><p>If <var>response</var>'s <a for=response>timing allow passed flag</a> is not set, then
+ set <var>timingInfo</var> to a new <a for=/>fetch timing info</a>, with its
+ <a for="fetch timing info">start time</a>
+ <a for="fetch timing info">fetch start time</a> set to <var>startTime</var>.
+
+ <li><p>Otherwise, <var>timingInfo</var>'s <a for="fetch timing info">start time</a> to
+ <var>startTime</var>.
+
+ <li><p>Return <var>timingInfo</var>.
+</ol>
+
+<p>To <dfn export for=response>finalize and report timing</dfn> given <a for=/>response</a>

Probably no need for `for=response` here either.

>     <li><p><a for=/>Resolve</a> <var>p</var> with <var>responseObject</var>.
   </ol>
 
+  <p><a>If aborted</a>, then <a for=response>finalize and report timing</a> for <var>response</var>,

This is not how that Infra construct works. It seems this needs integration with "abort fetch" as well somehow.

> @@ -5926,6 +6219,9 @@ the associated steps:
  <li><p>Let <var>steps</var> be to return the result of <a>package data</a> with the first argument
  given, <var>type</var>, and <var>object</var>'s <a for=Body>MIME type</a>.
 
+ <li><p><a>Upon fulfillment</a> of <var>promise</var>, call <var>object</var>'s
+ <a for=Body>on body consumed</a>.

One thing this setup means is that unless you consume you don't get a RT object. Is that the case? Also, it's a bit weird that above we do care about errors, but here we don't.

> @@ -5788,6 +6077,10 @@ due course.
 <dfn id=concept-body-mime-type for=Body>MIME type</dfn> algorithm which takes no arguments and
 returns failure or a <a for=/>MIME type</a>.
 
+<p>Objects including the {{Body}} interface mixin have an associated
+<dfn id=concept-body-on-fully-read for=Body>on body consumed</dfn> (an algorithm which takes no

Let's call this fully read, like the other one. No need for ID. (Or is Bikeshed case-insensitive? In that case, maybe "once read". I'd rather not name it similarly to event handlers.)

> @@ -6915,9 +7213,16 @@ method steps are:
    <li><p>Set <var>responseObject</var> to the result of <a for=Response>creating</a> a {{Response}}
    object, given <var>response</var>, "<code>immutable</code>", and <var>relevantRealm</var>.
 
+   <li><p>Set <var>responseObject</var>'s <a for=Body>on body consumed</a> to calling
+   <a for=response>finalize and report timing</a> for <var>response</var>,
+   <var>globalObject</var> and "<code>fetch</code>".

Here and elsewhere, note that we use Oxford commas.

> @@ -3775,6 +3932,75 @@ steps:
  <!-- This is really bad and needs to be handled differently at some point. -->
 </ol>
 
+<p>To <dfn for=response>Attach timing info</dfn>, given <a for=/>response</a>

Also, if we only do this once, I think it would be okay to inline this.

> + <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>Set <var>response</var>'s <a for=response>timing info</a> to <var>fetchParams</var>'s
+ <a for="fetch params">timing info</a>.
+
+ <li><p>If <var>redirectTimingList</var> is not empty, set <var>timingInfo</var>'s
+ <a for="fetch timing info">redirect start time</a> to <var>redirectTimingList</var>'s first item's
+ <a for="fetch timing info">fetch start time</a> and
+ <a for="fetch timing info">redirect end time</a> to <var>redirectTimingList</var>'s last item's
+ <a for="fetch timing info">fetch start time</a>.
+</ol>
+
+<p>To <dfn export for=response>retrieve timing info</dfn>, given <a for=/>response</a> <var>response</var>,

Also, it seems there is only one caller. Maybe inline this?

-- 
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-615089606