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

@annevk commented on this pull request.



> @@ -194,8 +194,76 @@ lt="authentication entry">authentication entries</a> (for HTTP authentication).
 
  <dt><dfn for="fetch params">task destination</dfn> (default null)
  <dd>Null, a <a for=/>global object</a>, or a <a for=/>parallel queue</a>.
+ <dt><dfn for="fetch params">timing info</dfn>
+ <dd>A <a for=/>fetch timing info</a>.
+ <dt><dfn for="fetch params">redirect timing info list</dfn>(default an empty list)

Missing space, I'd also make this "default « »" (see Infra).

>  </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 export for="fetch timing info">fetch start time</dfn> (default zero)

Single-space indentation. Let's use 0 instead of zero (at least for new numeric things).

> +  <dt><dfn export for="fetch timing info">request start time</dfn> (default zero)
+  <dt><dfn export for="fetch timing info">response start time</dfn> (default zero)
+  <dt><dfn export for="fetch timing info">response end time</dfn> (default zero)
+  <dd>A <a for=/>DOMHighResTimeStamp</a>.
+  <dt><dfn export for="fetch timing info">encoded body size</dfn> (default zero)
+  <dt><dfn export for="fetch timing info">decoded body size</dfn> (default zero)
+  <dd>A number.
+  <dt><dfn export for="fetch timing info">connection timing info</dfn> (default null)
+  <dd>Null or a <a for=/>connection timing info</a>.
+</dl>
+
+<p>A <dfn export>redirect timing info</dfn> is a <a for=/>struct</a> used to maintain timing
+information about a redirect.
+<a for=struct>items</a>:
+<dl>
+  <dt><dfn export for="redirect timing info">redirect location URL</dfn>

Why do we need to record this in two places? And why is it needed to begin with?

> +information pertaining to the process of obtaining a connection. It has the following
+<a for=struct>items</a>:
+<dl>
+  <dt><dfn export for="connection timing info">domain lookup start time</dfn> (default zero)
+  <dt><dfn export for="connection timing info">domain lookup end time</dfn> (default zero)
+  <dt><dfn export for="connection timing info">connection start time</dfn> (default zero)
+  <dt><dfn export for="connection timing info">connection end time</dfn> (default zero)
+  <dt><dfn export for="connection timing info">secure connection start time</dfn> (default zero)
+  <dd>A <a for=/>DOMHighResTimeStamp</a>
+  <dt><dfn export for="connection timing info">alpn negotiated protocol</dfn> (default empty string)
+  <dd>A string.
+</dl>
+
+<p><dfn data-cite="hr-time-2#dom-domhighrestimestamp">DOMHighResTimeStamp</dfn> and
+<dfn data-cite="hr-time-2#dfn-unsafe-shared-current-time">unsafe shared current time</dfn> </code>
+are defined in [[HR-TIME]].

I don't understand why we need to mention these here. That's a style only HTML uses because it doesn't have Bikeshed.

> @@ -2219,6 +2291,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:
+<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

See Infra, we don't use uppercase conformance terms.

> + <li><p>Let <var>flushAlgorithm</var> be an action that calls
+ <a>generate and report timing info</a> with <var>fetchParams</var> and <var>response</var>.
+
+ <li><p>Let <var>monitoringStream</var> be the result of creating a <a for=/>transform stream</a>
+ with its <a>flushAlgorithm</a> set to <var>flushAlgorithm</var>.
+
+ <li><p>If <var>body</var> is not null, then set <var>stream</var> to <var>body</var>'s
+ <a for=body>stream</a>.
+
+ <li><p>If <var>stream</var> is null, <a for=ReadableStream>closed</a> or
+ <a for=ReadableStream>errored</a>, then call <var>flushAlgorithm</var>, and return.
+
+ <li><p>Set <var>body</var>'s <a for=body>stream</a> to <var>monitoringStream</var>'s
+ <a attribute for=TransformStream>readable</a>.
+
+ <li><p>Call <var>stream</var>'s <a data-for=ReadableStream>pipeTo(destination)</a> with

We cannot invoke public methods. They could be overridden.

> @@ -3779,6 +3940,79 @@ steps:
  <!-- This is really bad and needs to be handled differently at some point. -->
 </ol>
 
+<p>To <dfn>finalize timing info</dfn>, given <a for=/>fetch params</a> <var>fetchParams</var> and
+<a for=/>response</a> <var>response</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>body</var> be <var>response</var>'s <a for=response>body</a>.
+
+ <li><p>Let <var>stream</var> be null.
+
+ <li><p>Let <var>flushAlgorithm</var> be an action that calls
+ <a>generate and report timing info</a> with <var>fetchParams</var> and <var>response</var>.

I don't think this works. If you look at "fully read" or "incrementally read" those algorithms end up queuing a task (and there's also the processResponseEndOfBody callback which does a similar thing). This needs to be part of that task.

I think the way to get there is that we define fully read and incrementally read operations that take a response (rather than a response body) and those end up doing the extra thing of snapshotting the time and generating a report. That does mean we would have to store information on the response already, but it seems that by the time we create a response we should already know what information to hide based on TAO, right? And there's only a couple timing things still ongoing.

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