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

@yoavweiss commented on this pull request.

Looks good! A few questions/comments, mostly on the SW/cache flow and the size attributes

>     <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 the result of calling
+     <a for="header list">extract a length</a> from <var>response</var>'s
+     <a for=response>header list</a>.

Encoded and decoded body size should map to the actual bytes transferred on the wire, not the Content-Length header (e.g. maybe no Content-Length was sent, maybe it's wrong, etc). Also Content-Length would give you one of those sizes, but not both.

In HTTP-Network fetch you increment them in step 18.1.1.1.3.

If we can't do the same for the SW flow, it'd be better to handwave those parts for now than to set them based on content length.

Otherwise, could we hang those variables on the Response, and then use those in case the SW is reusing a Response to `respondWith()`? 

/cc @wanderview @mfalken

> @@ -3942,6 +4157,9 @@ these steps:
     </ol>
   </ol>
 
+  <p>Otheriwse, set <var>fetchParams</var>'s <a for="fetch params">timing info</a>'s

s/Otheriwse/Otherwise/

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

@wanderview - Can you verify that this matches what's currently implemented?

> @@ -4445,6 +4688,14 @@ steps. They return a <a for=/>response</a>.
 
       <!-- cache hit -->
       <ol>
+       <li><p>Set <var>timingInfo</var>'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 the result of calling
+       <a for="header list">extract a length</a> from <var>storedResponse</var>'s
+       <a for=response>header list</a>.

Same comment as above, we shouldn't tie these to the Content-Length header.



>     <li>
     <p>Set <var>response</var> to the result of making an HTTP request over <var>connection</var>
     using <var>request</var> with the following caveats:
 
     <ul>
      <li><p>Follow the relevant requirements from HTTP. [[!HTTP]] [[!HTTP-SEMANTICS]] [[!HTTP-COND]] [[!HTTP-CACHING]] [[!HTTP-AUTH]]
 
+     <li><p>Set <var>timingInfo</var>'s <a for="fetch timing info">response start time</a> to
+     the <a for=/>unsafe shared current time</a>immediately after the user agent's HTTP parser
+     receives the first byte of the response (e.g. frame header bytes for HTTP/2, or response
+     status line for HTTP/1.x).
+
      <li><p>Wait until all the <a for=/>headers</a> are transmitted.
 
      <li>

Comment on line 5034: What should we do with 103s? Do we want to report them as responseStart?

Might be best to add a TOOO here, open an issue on Resource Timing, or both..
https://tools.ietf.org/html/rfc8297

> @@ -2213,6 +2283,89 @@ 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 <a for=/>connection</a>
+<var>connection</var> and its <a for=/>connection timing info</a> <var>timingInfo</var>, are as
+follows:
+<ul>
+ <li><p><var>timingInfo</var>'s <a for="connection timing info">domain lookup start time</a>
+ should be the <a for=/>unsafe shared current time</a> immediately before starting the domain
+ lookup, or beginning retrieval of the information from cache.
+
+ <li><p><var>timingInfo</var>'s <a for="connection timing info">domain lookup end time</a> should
+ be the <a for=/>unsafe shared current time</a> immediately after finishing the domain lookup, or
+ retrieving the information from cache.
+
+ <li><p><var>timingInfo</var>'s <a for="connection timing info">connection start time</a> should
+ be the <a for=/>unsafe shared current time</a> immediately before establishing the connection to
+ the server or proxy.

Oh, right. That makes sense. Maybe add a note indicating that here?

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