Re: [whatwg/fetch] Call `finalize and report timing` automatically on end-of-body (PR #1413)

@annevk commented on this pull request.

Could you please triage the comment threads? Reading through the "Conversation" tab it's rather unclear what is still outstanding and you want a reviewer to have another look at and have them resolve and what you consider addressed.

>    </ol>
 
+ <li><p>Assert: <var>timingGlobal</var> is null or a <a for=/>global object</a>.

I don't think this works. It's passed as "client" or a global object and it's never set to null.

> -<var>processEarlyHintsResponse</var> must be an algorithm accepting a <a for=/>response</a>. If
-given, <var>processResponse</var> must be an algorithm accepting a <a for=/>response</a>. If given,
-<var>processResponseEndOfBody</var> must be an algorithm accepting a <a for=/>response</a>. If
-given, <var>processResponseConsumeBody</var> must be an algorithm accepting a <a for=/>response</a>
-and null, failure, or a <a for=/>byte sequence</a>.
+an optional boolean <dfn export for=fetch><var>useParallelQueue</var></dfn> (default false), an
+optional null or string <dfn export for=fetch><var>initiatorType</var></dfn> (default null),
+and an optional "<code>client</code>" or a <a for=/>global object</a>
+<dfn export for=fetch><var>timingGlobal</var></dfn> (default "<code>client</code>"), run the steps
+below. If given, <var>processRequestBodyChunkLength</var> must be an algorithm accepting an integer
+representing the number of bytes transmitted. If given, <var>processRequestEndOfBody</var> must be an
+algorithm accepting no arguments. If given, <var>processEarlyHintsResponse</var> must be an
+algorithm accepting a <a for=/>response</a>. If given, <var>processResponse</var> must be an
+algorithm accepting a <a for=/>response</a>. If given, <var>processResponseEndOfBody</var> must be
+an algorithm accepting a <a for=/>response</a>. If given, <var>processResponseConsumeBody</var> must
+be an algorithm accepting a <a for=/>response</a> and null, failure, or a <a for=/>byte sequence</a>.

It's an interesting approach to add this as an argument to fetch rather than request. Is that what we want? Might we at some point wish to expose it in service worker's fetch event?

> +   <a for=request>destination</a> is "<code>document</code>", then set <var>fetchParams</var>'s
+   <a for="fetch params">controller</a>'s <a for="fetch controller">full timing info</a> to
+   <var>fetchParams</var>'s <a for="fetch params">timing info</a>.
+
+   <li>
+    <p>Set <var>fetchParams</var>'s <a for="fetch params">controller</a>'s
+    <a for="fetch controller">report timing steps</a> to the following steps given a
+    <a for=/>global object</a> <var>global</var>:
+
+    <ol>
+     <li><p>If <var>fetchParams</var>'s <a for="fetch params">request</a>'s <a for=request>URL</a>'s
+     <a for=url>scheme</a> is not an <a>HTTP(S) scheme</a>, then return.
+
+     <li><p>Set <var>timingInfo</var>'s <a for="fetch timing info">end time</a> to the
+     <a>relative high resolution time</a> given <var>unsafeEndTime</var> and
+     <var>global</var>.</p></li>

Nit: no closing tags.

> +     <li><p>Set <var>timingInfo</var>'s <a for="fetch timing info">end time</a> to the
+     <a>relative high resolution time</a> given <var>unsafeEndTime</var> and
+     <var>global</var>.</p></li>
+
+     <li><p>Let <var>cacheState</var> be <var>response</var>'s <a for=response>cache state</a>.
+
+     <li><p>Let <var>bodyInfo</var> be <var>response</var>'s <a for=response>body info</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 the result of <a>creating an opaque timing info</a> for
+      <var>timingInfo</var>, set <var>bodyInfo</var> to a new <a for=/>response body info</a> and
+      set <var>cacheState</var> to the empty string.
+
+      <p class=note>This covers the case of <var>response</var> being a <a>network error</a>.
+     </li>

Nit: no closing tag.

> @@ -3813,15 +3836,17 @@ an optional algorithm <dfn export for=fetch><var>processEarlyHintsResponse</var>
 algorithm <dfn export for=fetch id=process-response><var>processResponse</var></dfn>, an optional
 algorithm <dfn export for=fetch><var>processResponseEndOfBody</var></dfn>, an optional algorithm
 <dfn export for=fetch id=process-response-end-of-body oldids=process-response-end-of-file><var>processResponseConsumeBody</var></dfn>,
-and an optional boolean <dfn export for=fetch><var>useParallelQueue</var></dfn> (default false), run
-the steps below. If given, <var>processRequestBodyChunkLength</var> must be an algorithm accepting
-an integer representing the number of bytes transmitted. If given,
-<var>processRequestEndOfBody</var> must be an algorithm accepting no arguments. If given,
-<var>processEarlyHintsResponse</var> must be an algorithm accepting a <a for=/>response</a>. If
-given, <var>processResponse</var> must be an algorithm accepting a <a for=/>response</a>. If given,
-<var>processResponseEndOfBody</var> must be an algorithm accepting a <a for=/>response</a>. If
-given, <var>processResponseConsumeBody</var> must be an algorithm accepting a <a for=/>response</a>
-and null, failure, or a <a for=/>byte sequence</a>.
+an optional boolean <dfn export for=fetch><var>useParallelQueue</var></dfn> (default false), an
+optional null or string <dfn export for=fetch><var>initiatorType</var></dfn> (default null),
+and an optional "<code>client</code>" or a <a for=/>global object</a>
+<dfn export for=fetch><var>timingGlobal</var></dfn> (default "<code>client</code>"), run the steps

This does not account for it taking "none" as a value, which is stated in the usage section as an option.

Do we have a concrete case for which we need _timingGlobal_ because request's client is inadequate?

> +<p>A <dfn export>fetch resource info</dfn> is a <a for=/>struct</a> used to maintain
+information needed by <cite>Resource Timing</cite> and <cite>Navigation Timing</cite>. It has the
+following <a for=struct>items</a>: [[RESOURCE-TIMING]] [[NAVIGATION-TIMING]]
+
+<dl>
+ <dt><dfn export for="fetch resource info">encoded body size</dfn> (default 0)
+ <dt><dfn export for="fetch resource info">decoded body size</dfn> (default 0)
+ <dd>A number.
+</dl>

Hmm okay. So then what's left here is the naming as per the first comment. We shouldn't repeat "body".

-- 
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/fetch/pull/1413#pullrequestreview-998399907
You are receiving this because you are subscribed to this thread.

Message ID: <whatwg/fetch/pull/1413/review/998399907@github.com>