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

@annevk commented on this pull request.

There's still a lot of style issues left that I mostly tried to read over. I left some questions for things that are unclear. I checked some of the timings, but that could use another look as well. Is @yoavweiss also looking at those?

>  </dl>
 
+<p>A <dfn export>fetch timing info</dfn> is a <a for=/>struct</a> used to maintain timing information
+later required by the resource timing and navigation timing specs. It has the following
+<a for=struct>items</a>:
+<dl>
+  <dt><dfn for="fetch timing info">redirect start time</dfn> (default zero)
+  <dd>A <a for=/>DOMHighResTimeStamp</a>
+  <dt><dfn for="fetch timing info">redirect end time</dfn> (default zero)
+  <dd>A <a for=/>DOMHighResTimeStamp</a>
+  <dt><dfn for="fetch timing info">fetch start time</dfn> (default zero)
+  <dd>A <a for=/>DOMHighResTimeStamp</a>
+  <dt><dfn for="fetch timing info">request start time</dfn> (default zero)
+  <dd>A <a for=/>DOMHighResTimeStamp</a>
+  <dt><dfn for="fetch timing info">response start time</dfn> (default zero)
+  <dd>A <a for=/>DOMHighResTimeStamp</a>

If you have multiple `dt`s for which the `dd` is the same, you can group them and use a single `dd`. Also, there should be a dot at the end of the `dd` for consistency.

> @@ -4031,8 +4221,9 @@ these steps:
      <a for="filtered response">internal response</a> is <var>actualResponse</var>.
 
      <dt>"<code>follow</code>"
-     <dd><p>Set <var>response</var> to the result of running <a>HTTP-redirect fetch</a> given
-     <var>fetchParams</var> and <var>response</var>.
+     <dd><p>Set <var>timingInfo</var>'s <a for="fetch timing info">redirect start time</a> to

Wouldn't this keep updating the redirect start time?

> @@ -1855,6 +1923,9 @@ Unless stated otherwise, it is "<code>default</code>".
 <p class="note">This indicates that the request was intentionally aborted by the developer or
 end-user.
 
+<p>A <a for=/>response</a> can have an associated <a for=/>fetch timing info</a>
+<dfn export for=response id=concept-response-timing-info>timing info</dfn>, initially null.

This does not appear to be used, at least `for=response>timing` yields nothing.

>  </ol>
 
+<p>To <dfn>handle response end</dfn>, given <a for=/>fetch params</a> <var>fetchParams</var> and

This is only called for network responses. Does that mean that if a service worker handles a response there's no RT entry?

> +<p>To <dfn>handle response end</dfn>, given <a for=/>fetch params</a> <var>fetchParams</var> and
+<a for=/>response</a> <var>response</var>, perform the following steps:
+<ol>
+  <li>Set <var>fetchParams</var>'s <a for="fetch params">timing info</a>'s
+  <a for="fetch timing info">response end time</a> to the <a for=/>unsafe shared current time</a>.
+  <li>Let <var>request</var> be <var>fetchParams</var>'s <a for="fetch params">request</a>.
+  <li>Let <var>globalObject</var> be <a for="fetch params">request</a>'s
+  <a for=request>client</a>'s <a for="environment settings object">global object</a>.
+  <li>Let <var>timingInfo</var> be <var>fetchParams</var>'s <a for="fetch params">timing info</a>.
+  <li>Let <var>requestedURL</var> be <var>response</var>'s <a for=response>URL list</a>'s first
+  item.
+  <li>Return, if <var>request</var>'s <a for=request>destination</a> is one of the following:
+  <ul>
+    <li><code>"report"</code>
+    <li><code>"document"</code>
+    <li><code>"frame"</code>

This doesn't cover embed/object/iframe. That doesn't seem intentional.

>  </ol>
 
+<p>To <dfn>handle response end</dfn>, given <a for=/>fetch params</a> <var>fetchParams</var> and
+<a for=/>response</a> <var>response</var>, perform the following steps:
+<ol>
+  <li>Set <var>fetchParams</var>'s <a for="fetch params">timing info</a>'s
+  <a for="fetch timing info">response end time</a> to the <a for=/>unsafe shared current time</a>.
+  <li>Let <var>request</var> be <var>fetchParams</var>'s <a for="fetch params">request</a>.
+  <li>Let <var>globalObject</var> be <a for="fetch params">request</a>'s
+  <a for=request>client</a>'s <a for="environment settings object">global object</a>.
+  <li>Let <var>timingInfo</var> be <var>fetchParams</var>'s <a for="fetch params">timing info</a>.
+  <li>Let <var>requestedURL</var> be <var>response</var>'s <a for=response>URL list</a>'s first
+  item.
+  <li>Return, if <var>request</var>'s <a for=request>destination</a> is one of the following:

If .... then return reads better. Also each `<li>` needs a `<p>` generally.

> @@ -2215,6 +2288,86 @@ 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>When <dfn>recording connnection timing info</dfn> given a <var>connection</var>, perform the
+following steps:

As this cannot be implemented as a step-by-step algorithm I think this would work better as a section that describes the requirements. What do you think?

> @@ -3763,11 +3921,41 @@ steps:
 
  <li><p>Wait for either <var>response</var>'s <a for=response>body</a> to be null, or
  <var>response</var>'s <a for=response>body</a>'s <a for=body>stream</a> to be
- <a for=ReadableStream>closed</a> or <a for=ReadableStream>errored</a>, and then set
- <var>request</var>'s <a>done flag</a>.
- <!-- This is really bad and needs to be handled differently at some point. -->
+ <a for=ReadableStream>closed</a> or <a for=ReadableStream>errored</a>, and perform the following steps:
+ <ol>
+  <li><p>Set <var>request</var>'s <a>done flag</a>.
+   <!-- This is really bad and needs to be handled differently at some point. -->
+ </ol>

Please revert these changes.

> @@ -4874,6 +5081,8 @@ optional boolean <var>forceNewConnection</var> (default false), run these steps:
  <var>highWaterMark</var>, and <a for=ReadableStream/create><var>sizeAlgorithm</var></a> set to
  <var>sizeAlgorithm</var>.
 
+ <li><p>Let <var>timingInfo</var> be the <var>fetchParams</var>'s <a for="fetch params">timing info</a>.

It seems this is duplicating the variable.

> @@ -1088,8 +1134,10 @@ must be an algorithm accepting an exception.
     </ol>
 
    <dt><a for="read request">close steps</a>
-   <dd><ol><li><p><a>Queue a fetch task</a> given <var>processEndOfBody</var> and
-   <var>taskDestination</var>.</ol>
+   <dd><ol>
+    <li><p><a>Queue a fetch task</a> given <var>processEndOfBody</var> and
+      <var>taskDestination</var>.
+   </ol>

It looks to me this change can still be undone.

> @@ -4974,8 +5192,13 @@ optional boolean <var>forceNewConnection</var> (default false), run these steps:
       <p>If <var>aborted</var> is set, then:
 
       <ol>
+       <li><p>Call <a for=/>handle response end</a> with <var>fetchParams</var> and <var>response</var>.

It seems weird that this is invoked and then below response end time is set. And why is this only when the response is aborted and not when it errored? (Note that "if aborted" and "aborted" are different things.)

> @@ -4974,8 +5192,13 @@ optional boolean <var>forceNewConnection</var> (default false), run these steps:
       <p>If <var>aborted</var> is set, then:
 
       <ol>
+       <li><p>Call <a for=/>handle response end</a> with <var>fetchParams</var> and <var>response</var>.

Also, response end time isn't set for a successful 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/1185#pullrequestreview-607276200