- From: Anne van Kesteren <notifications@github.com>
- Date: Thu, 04 Mar 2021 03:13:00 -0800
- To: whatwg/fetch <fetch@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <whatwg/fetch/pull/1185/review/603943537@github.com>
@annevk commented on this pull request.
It would help a lot if you checked the final diff and read through it at least once to make sure there are no obvious mistakes there. At least I personally tend to spot a lot of mistakes when doing that in my own writing.
Overall I think this approach looks good. I'm a little worried about it also relying on the "done flag" badness. Perhaps the solution there is to accept that implementations are not full duplex and so once the response stream is closed it's really all over.
One thing I haven't done in detail is ensuring that the point of capture matches RT and matches implementations. For that it would be helpful to have some pointers to tests. As I also noted I would prefer to not define fields we're unsure of or that can be inferred from other fields.
(I also do not see transfer size, was that omitted intentionally?)
> </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>
I thought these were integers without a type and only once you have a global you can cast them into this type?
> + <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>
+ <dt><dfn for="fetch timing info">response end time</dfn> (default zero)
+ <dd>A <a for=/>DOMHighResTimeStamp</a>
+ <dt><dfn for="fetch timing info">worker start time</dfn> (default zero)
+ <dd>A <a for=/>DOMHighResTimeStamp</a>
+ <dt><dfn for="fetch timing info">encoded body size</dfn> (default zero)
+ <dd>A number.
+ <dt><dfn for="fetch timing info">decoded body size</dfn> (default zero)
+ <dd>A number.
+ <dt><dfn for="fetch timing info">connection timing info</dfn> (default null)
+ <dd>A <a for=/>connection timing info</a>.
Null or 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>
+ <dt><dfn for="fetch timing info">response end time</dfn> (default zero)
+ <dd>A <a for=/>DOMHighResTimeStamp</a>
+ <dt><dfn for="fetch timing info">worker start time</dfn> (default zero)
+ <dd>A <a for=/>DOMHighResTimeStamp</a>
+ <dt><dfn for="fetch timing info">encoded body size</dfn> (default zero)
This is redundant with Content-Length, no?
> + <dt><dfn for="fetch timing info">connection timing info</dfn> (default null)
+ <dd>A <a for=/>connection timing info</a>.
+</dl>
+
+<p>A <dfn>connection timing info</dfn> is a <a for=/>struct</a> used to maintain timing information
+pertaining to the process of obtaining a connection. It has the following <a for=struct>items</a>:
+<dl>
+ <dt><dfn for="connection timing info">domain lookup start time</dfn> (default zero)
+ <dd>A <a for=/>DOMHighResTimeStamp</a>
+ <dt><dfn for="connection timing info">domain lookup end time</dfn> (default zero)
+ <dd>A <a for=/>DOMHighResTimeStamp</a>
+ <dt><dfn for="connection timing info">connection start time</dfn> (default zero)
+ <dd>A <a for=/>DOMHighResTimeStamp</a>
+ <dt><dfn for="connection timing info">connection end time</dfn> (default zero)
+ <dd>A <a for=/>DOMHighResTimeStamp</a>
+ <dt><dfn for="connection timing info">secure connection start time</dfn> (default zero)
This seems redundant with connection start time. The caller can figure out this value from the connection start time coupled with the scheme, as far as I can tell.
> +
+<p>A <dfn>connection timing info</dfn> is a <a for=/>struct</a> used to maintain timing information
+pertaining to the process of obtaining a connection. It has the following <a for=struct>items</a>:
+<dl>
+ <dt><dfn for="connection timing info">domain lookup start time</dfn> (default zero)
+ <dd>A <a for=/>DOMHighResTimeStamp</a>
+ <dt><dfn for="connection timing info">domain lookup end time</dfn> (default zero)
+ <dd>A <a for=/>DOMHighResTimeStamp</a>
+ <dt><dfn for="connection timing info">connection start time</dfn> (default zero)
+ <dd>A <a for=/>DOMHighResTimeStamp</a>
+ <dt><dfn for="connection timing info">connection end time</dfn> (default zero)
+ <dd>A <a for=/>DOMHighResTimeStamp</a>
+ <dt><dfn for="connection timing info">secure connection start time</dfn> (default zero)
+ <dd>A <a for=/>DOMHighResTimeStamp</a>
+ <dt><dfn for="connection timing info">alpn negotiated protocol</dfn> (default empty string)
+ <dd>A string.
If there is a finite enumeration I'd rather enumerate.
> +<dl>
+ <dt><dfn for="connection timing info">domain lookup start time</dfn> (default zero)
+ <dd>A <a for=/>DOMHighResTimeStamp</a>
+ <dt><dfn for="connection timing info">domain lookup end time</dfn> (default zero)
+ <dd>A <a for=/>DOMHighResTimeStamp</a>
+ <dt><dfn for="connection timing info">connection start time</dfn> (default zero)
+ <dd>A <a for=/>DOMHighResTimeStamp</a>
+ <dt><dfn for="connection timing info">connection end time</dfn> (default zero)
+ <dd>A <a for=/>DOMHighResTimeStamp</a>
+ <dt><dfn for="connection timing info">secure connection start time</dfn> (default zero)
+ <dd>A <a for=/>DOMHighResTimeStamp</a>
+ <dt><dfn 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
This can be removed.
> @@ -1037,8 +1084,8 @@ must be an algorithm accepting an exception.
<p class=note>This operation will not throw an exception.
<li><p>Perform the <a>incrementally-read loop</a> given <var>reader</var>,
- <var>taskDestination</var>, <var>processBodyChunk</var>, <var>processEndOfBody</var>, and
- <var>processBodyError</var>.
+ <var>taskDestination</var>, <var>processBodyChunk</var>, <var>processEndOfBody</var>,
+ <var>processBodyError</var> and <var>timingInfo</var>.
This seems like a leftover change?
> @@ -1055,7 +1102,6 @@ must be an algorithm accepting an exception.
<dd>
<ol>
<li><p>Let <var>continueAlgorithm</var> be null.
-
Same?
> @@ -1078,8 +1124,8 @@ must be an algorithm accepting an exception.
<li><p>Run <var>processBodyChunk</var> given <var>bytes</var>.
<li><p>Perform the <a>incrementally-read loop</a> given <var>reader</var>,
- <var>taskDestination</var>, <var>processBodyChunk</var>, <var>processEndOfBody</var>, and
- <var>processBodyError</var>.
And again?
> @@ -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>
Here as well?
> @@ -1902,7 +1953,6 @@ initially unset.
allowed on the resource fetched by looking at the flag of the response returned. Because the flag on
the response of a redirect has to be set if it was set for previous responses in the redirect chain,
this is also tracked internally using the request's <a for=request>timing allow failed flag</a>.
-
Please restore this.
> @@ -1828,6 +1876,9 @@ Unless stated otherwise, it is "<code>default</code>".
<p>A <a for=/>response</a> can have an associated
<dfn export for=response id=concept-response-aborted>aborted flag</dfn>, which is initially unset.
+<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>, which is initially unset.
You added this in the wrong place as the note below is about the aborted flag.
It also doesn't make sense for a thing like this to be unset (that's only for deprecated flags). You want it to be null or something.
> @@ -2120,6 +2170,9 @@ unset or <a for=request>keepalive</a> is false, <a lt=terminated for=fetch>termi
identified by a <b>key</b> (a <a>network partition key</a>), an <b>origin</b> (an
<a for=/>origin</a>), and <b>credentials</b> (a boolean).
+<p>Each <a>connection</a> has an associated <a for=/>connection timing info</a> <dfn for=connection>timingInfo</dfn>,
100 columns.
Also `<a for=connection>timing info</a>` seems cleaner.
> @@ -2129,15 +2182,61 @@ identified by a <b>key</b> (a <a>network partition key</a>), an <b>origin</b> (a
<var>credentials</var>, then return that <a>connection</a>.
<li><p>Let <var>connection</var> be null.
+ <li><p>Let <var>timingInfo</var> be <var>connection</var>'s <a for=connection>timingInfo</a>.
Newline befoore the li.
> @@ -2170,7 +2269,6 @@ 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 -->
-
Please don't remove newlines.
>
<li>
<p>Run these steps, but <a>abort when</a> the ongoing fetch is <a for=fetch>terminated</a>:
<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>, with the following caveats:
+ [[!HTTP]] [[!HTTP-SEMANTICS]] [[!HTTP-COND]] [[!HTTP-CACHING]] [[!HTTP-AUTH]] [[!TLS]]
+ <ol>
This should probably not be an ol as it's not necessarily to be done in order. Perhaps we should have a separate operation that creates a connection to keep this a bit more manageable? And that operation would maybe just be a section with a bunch of paragraphs explaining things.
> @@ -3348,7 +3447,6 @@ the request.
<li><p>Run <a>main fetch</a> given <var>fetchParams</var>.
</ol>
-
No newline removal please.
> @@ -3360,6 +3458,9 @@ steps:
<li><p>Let <var>response</var> be null.
+ <li><p>Set <var>fetchParams</var>'s <a for="fetch params">timing info</a>'s <a for="fetch timing info">fetch start time</a>
Exceeds 100 columns.
> @@ -3360,6 +3458,9 @@ steps:
<li><p>Let <var>response</var> be null.
+ <li><p>Set <var>fetchParams</var>'s <a for="fetch params">timing info</a>'s <a for="fetch timing info">fetch start time</a>
+ to the result of calling <a for=/>unsafe shared current time</a>.
```suggestion
to the <a for=/>unsafe shared current time</a>.
```
might be clear enough and read slightly nicer?
> </ol>
+<p>To <dfn>report resource timing</dfn>, given <a for=/>fetch params</a> <var>fetchParams</var> and
Shouldn't RT define this operation? (Although some of the normalization should happen here I suppose.)
> @@ -3821,6 +3948,9 @@ these steps:
<li><p>Set <var>response</var> to the result of invoking <a for=/>handle fetch</a> for
<var>requestForServiceWorker</var>. [[!HTML]] [[!SW]]
+ <p class=XXX>TODO: set <a for="fetch timing info">worker start time</a> value,
+ once its exact function is clarified.
+ See <a href="https://github.com/w3c/navigation-timing/issues/128">Navigation timing Issue 128</a>.
I would prefer we don't define fields that we don't actually understand for now and instead start with what we know and build it up from there.
> @@ -3952,8 +4082,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
+ <var>timingInfo</var>'s <a for="fetch timing info">fetch start time</a>, and set <var>response</var> to
+ the result of running <a>HTTP-redirect fetch</a> given <var>fetchParams</var> and <var>response</var>.
How does this work for navigations?
> @@ -4620,7 +4755,9 @@ these steps:
<var>request</var>'s <a for=request>current URL</a>'s <a for=url>origin</a>, and
<var>includeCredentials</var>.
</dl>
-
No newline removal please.
> @@ -4720,8 +4862,9 @@ these steps:
</ol>
<li><p><a for=body>Incrementally read</a> <var>request</var>'s <a for=request>body</a> given
- <var>processBodyChunk</var>, <var>processEndOfBody</var>, <var>processBodyError</var>, and
- <var>fetchParams</var>'s <a for="fetch params">task destination</a>.
+ <var>processBodyChunk</var>, <var>processEndOfBody</var>, <var>processBodyError</var>,
+ <var>fetchParams</var>'s <a for="fetch params">task destination</a>, and
+ <var>fetchParams</var>'s <a for="fetch params">timing info</a>.
This is no longer correct, right?
> @@ -4620,7 +4755,9 @@ these steps:
<var>request</var>'s <a for=request>current URL</a>'s <a for=url>origin</a>, and
<var>includeCredentials</var>.
</dl>
-
+ <li>Set <var>timingInfo</var>'s <a for="fetch timing info">connection timing info</a> to the result
+ of calling <a>clamp connection timing info</a> with <var>connection</var>'s <a for=connection>timingInfo</a>
+ and <var>timingInfo</var>'s <a for="fetch timing info">fetch start time</a>.
Newline after this.
> @@ -4634,6 +4771,8 @@ these steps:
`<code>Transfer-Encoding</code>`/`<code>chunked</code>` to <var>request</var>'s
<a for=request>header list</a>.
+ <li>Set <var>timingInfo</var>'s <a for="fetch timing info">request start time</a> to the result
+ of calling <a for=/>unsafe shared current time</a>.
Indentation is wrong. Also newline.
> @@ -4820,17 +4963,24 @@ these steps:
<ol>
<li><p>Let <var>bytes</var> be the transmitted bytes.
+ <li><p>Let <var>timingInfo</var> be the <var>fetchParams</var>'s <a for="fetch params">timing info</a>.
You probably want to do this outside the loop?
>
<li><p>Let <var>codings</var> be the result of <a>extracting header list values</a> given
`<code>Content-Encoding</code>` and <var>response</var>'s <a for=response>header list</a>.
+ <li><p>Set <var>timingInfo</var>'s <a for="fetch timing info">encoded body size</a> to the result of
+ adding <var>timingInfo</var>'s <a for="fetch timing info">encoded body size</a> and <var>bytes</var>.
Increase X by Y is the language, but note that _bytes_ is not a length. You need `<a for="byte sequence">length</a>`.
> @@ -4891,6 +5043,20 @@ these steps:
returning.</span>
</ol>
+<p>To <dfn>clamp connection timing info</dfn>, given <a for=/>connection timing info</a> <var>timingInfo</var>
This should probably be defined together with the structs. It also needs some formatting adjustments, see README.
> </ol>
+<p>To <dfn>report resource timing</dfn>, given <a for=/>fetch params</a> <var>fetchParams</var> and
+<a for=/>response</a> <var>response</var>, perform the following steps:
+<ol>
+ <li>Let <var>globalObject</var> be <var>fetchParams</var>'s <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>If <var>response</var>'s <a for=response>timing allow passed flag</a> is not set, set
Do we still need this flag on response?
> +<ol>
+ <li>Let <var>globalObject</var> be <var>fetchParams</var>'s <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>If <var>response</var>'s <a for=response>timing allow passed flag</a> is not set, set
+ <var>timingInfo</var> to a new <a for=/>fetch timing info</a>, with its
+ <a for="fetch timing info">fetch start time</a> set to <var>timingInfo</var>'s
+ <a for="fetch timing info">fetch start time</a>,
+ <a for="fetch timing info">response end time</a> set to <var>timingInfo</var>'s
+ <a for="fetch timing info">response end time</a>,
+ <a for="fetch timing info">connection timing info</a> set to a new
+ <a for=/>connection timing info</a> with its
+ <a for="connection timing info">alpn negotiated protocol</a> set to <var>timingInfo</var>'s
+ <a for="fetch timing info">connection timing info</a>'s
+ <a for="connection timing info">alpn negotiated protocol</a>.
This is always leaked? That's a bit surprising.
>
<li><p>Let <var>codings</var> be the result of <a>extracting header list values</a> given
`<code>Content-Encoding</code>` and <var>response</var>'s <a for=response>header list</a>.
+ <li><p>Set <var>timingInfo</var>'s <a for="fetch timing info">encoded body size</a> to the result of
+ adding <var>timingInfo</var>'s <a for="fetch timing info">encoded body size</a> and <var>bytes</var>.
But also, as I noted above I don't think we want encoded body size as that's just Content-Length. Or does this have a value in the absence of Content-Length as well? Is there a test for that?
--
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-603943537
Received on Thursday, 4 March 2021 11:13:15 UTC