Report A and script loading to resource timing #7180

noamr · 2021-10-07T13:56:19Z

Starting with two elements that perform a full download, and have decent
HTML/fetch integration. Finalize and report resource timing once the response body is fully read or there is a network error,
and before any load/error events are fired.

Resolves part of #6542

A element: report resource timing for download & ping
Script: report resource timing for importing, classic and worker.

Reporting is always done before load/error events are fired, so that
those events could get access to the new entry.

[x] At least two implementers are interested (and none opposed):
This is already implemented in Chrome, Safari and Firefox, and was previously spec'ed as part of resource timing.
Some of the specific discrepancies will be handled as part of WPT.

[x] New tests for previously unspecified scenarios: web-platform-tests/wpt#32513

[X] Implementation bugs are filed:

Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=1290356
Firefox: No currently known bugs
Safari: No currently known bugs
(See WHATWG Working Mode: Changes for more details.)

/infrastructure.html ( diff )
/links.html ( diff )
/webappapis.html ( diff )

yoavweiss

Non-authoritative LGTM % nit

source

noamr · 2022-01-17T13:05:24Z

@domenic, review when possible please?

domenic · 2022-01-18T18:11:32Z

source

+   <i data-x="processResponseConsumeBody">processResponseConsumeBody</i> given
+   <span data-x="concept-response">response</span> <var>res</var> set to
+   <span>finalize and report timing</span> with <var>res</var>, the
+   <span>current global object</span>, and "a".</p></li>


"<code data-x="">a</code>"

domenic · 2022-01-18T18:12:58Z

source

@@ -24226,7 +24227,11 @@ document.body.appendChild(wbr);</code></pre>
    </dl>
   </li>

-   <!--FETCH--><li><p><span data-x="concept-fetch">Fetch</span> <var>request</var>.</p></li>
+   <!--FETCH--><li><p><span data-x="concept-fetch">Fetch</span> <var>request</var>, with
+   <i data-x="processResponseConsumeBody">processResponseConsumeBody</i> given


This ties into whatwg/fetch#1382 ... if someone sends a long trickle body, do we really want to wait for it before finalizing and reporting timing? What do browsers do? That issue suggests maybe they terminate the body early?

Chrome does something equivalent to processResponseEndOfBody. I'll revise.

domenic · 2022-01-18T18:13:29Z

source

+
+    <ol>
+     <li><p><span>Finalize and report timing</span> with <var>response</var>, the
+     <span>current global object</span> and "script".</p></li>


"<code data-x="">script</code>"

domenic · 2022-01-18T18:15:15Z

source

@@ -91620,7 +91637,9 @@ document.querySelector("button").addEventListener("click", bound);
     <span>JavaScript MIME type</span>,</p>
    </ul>

-    <p>then asynchronously complete this algorithm with null, and return.</p>
+    <p>then <span>finalize and report timing</span> with <var>response</var>, the


I think we should consolidate this into the previous step now. Something like:

If any of the following conditions are met:

x;

y; or

z and w

then:

domenic · 2022-01-18T18:16:04Z

source

@@ -91635,6 +91654,9 @@ document.querySelector("button").addEventListener("click", bound);
   data-x="concept-response-url">url</span>, and the <span>default classic script fetch
   options</span>.</p></li>

+   <li><p><span>Finalize and report timing</span> with <var>response</var>, the
+   <span>current global object</span> and "script".</p></li>


Is it correct that this happens after parsing? I could imagine parsing taking a few milliseconds...

Changed to make it happen prior to parsing

domenic · 2022-01-18T18:17:43Z

source


    <p class="note"><var>response</var> is always <span>CORS-same-origin</span>.</p>
   </li>

+   <li><p><span>Finalize and report timing</span> with <var>response</var>, the
+   <span>current global object</span> and "script".</p></li>


Here you report timing before parsing happens. That is cleaner since then you don't have duplicate "finalize and report timing with response, the current global object, and script".

domenic · 2022-01-18T18:20:40Z

source

+   <i data-x="processResponseConsumeBody">processResponseConsumeBody</i> given
+   <span data-x="concept-response">response</span> <var>res</var> set to
+   <span>finalize and report timing</span> with <var>res</var>, the
+   <span>current global object</span>, and "a".</p></li>


There is no "current global object" in most of these algorithms. You need to use the global object of one of the various settings objects passed into each algorithm. (I'm not sure which one in each case...)

domenic · 2022-01-19T15:14:18Z

source

+   <i data-x="processResponseEndOfBody">processResponseEndOfBody</i> given
+   <span data-x="concept-response">response</span> <var>res</var> set to
+   <span>finalize and report timing</span> with <var>res</var>, <var>global</var>, and
+   "<code data-x="">ping</code>".</p></li>


OK, so to be clear, if the body is not fully downloaded (because the server trickles it, for example) by the time the page unloads due to the link navigation, no resource timing entry will be created. Is that correct? Are there tests for that case?

By the time the page unloads you can't test whether an RT entry is created.
But RT entry is created when for example you have a link with a target attribute. I'll make sure tests cover this.

Right. I was thinking that maybe browsers sent the resource timing when the response headers came in, not when the whole body did. It sounds like that is not the case.

Yea, I double-checked this.

domenic · 2022-01-19T15:16:06Z

source

+   <li><p>Let <var>global</var> be the
+   <span data-x="concept-settings-object-global">global object</span> of the
+   <span>environment settings object</span> of the <code>Document</code> containing the
+   <span>hyperlink</span>.


"the Document containing the hyperlink" is not defined (although I see that is used in preexisting text); nor is "the environment settings object of [a Document]".

It'd be best to factor this out as follows:

Let client be the element's node document's relevant settings object.

Later, passing client's global object to "finalize and report timing".

I called it settingsObject rather than client, seemed more explicit. Otherwise fixed.

domenic · 2022-01-19T15:18:05Z

source

@@ -91917,7 +91930,18 @@ document.querySelector("button").addEventListener("click", bound);
     <span>ok status</span>,</p></li>
    </ul>

-    <p>then asynchronously complete this algorithm with null, and return.</p>
+    <p>then:</p>


This did not get fixed: #7180 (comment)

OK, maybe I misunderstood that original comment. Will reread

I refactored that bit, hope it's satisfactory.

source

domenic · 2022-01-19T15:20:14Z

source

-    algorithm, and run the remaining steps as part of the fetch's <span>process response</span> for
-    the <span data-x="concept-response">response</span> <var>response</var>.</p>
+    algorithm, and run the remaining steps as part of the fetch's
+    <i>processResponseConsumeBody</i> for <span data-x="concept-response">response</span>


Please make this cross-link to the definition in Fetch, with the appropriate data-x attribute on the .

domenic · 2022-01-19T15:20:41Z

source


    <p class="note"><var>response</var> is always <span>CORS-same-origin</span>.</p>
   </li>

+   <li><p><span>Finalize and report timing</span> with <var>response</var>, the
+   <span>current global object</span> and "script".</p></li>


Missing code around script

noamr · 2022-01-24T16:09:14Z

@domenic, re-review? :)

domenic · 2022-01-24T23:30:51Z

Sorry, on my near-term to-do list but I got to the end of another the day without having made the time. Tomorrow for sure!

domenic · 2022-01-25T21:14:51Z

source

+    <var>response</var>, <var>fetch client settings object</var>'s
+    <span data-x="concept-settings-object-global">global object</span>, and
+    "<code data-x="">other</code>", asynchronously complete this algorithm with
+    null, and return.</p>


Sorry, this is still not great.

To be clear, the structure in your PR right now is:

If either of the following conditions are met:

x; or

y
then:

Finalize and report...

Asynchronously complete with null

Return.

If z and w, then: finalize and report..., asynchronously complete with null, and return.

Can you see how this is bad? I am proposing instead we do the following:

If any of the following conditions are met:

x;

y; or

z and w
then:

Finalize and report...

Asynchronously complete with null

Return.

Ah I see what you mean now. It was entirely unclear to me what you meant in the original comment.

domenic · 2022-01-25T21:15:47Z

source

-   <li><p>Let <var>source text</var> be the result of <span data-x="UTF-8 decode">UTF-8
+   <li><p><span>Finalize and report timing</span> with <var>response</var>,
+   <var>fetch client settings object</var>'s
+   <span data-x="concept-settings-object-global">global object</span> and


Suggested change

global object and

global object, and

domenic · 2022-01-25T21:15:57Z

source

+   <span data-x="concept-settings-object-global">global object</span> and
+   "<code data-x="">other</code>".</p></li>
+
+    <li><p>Let <var>source text</var> be the result of <span data-x="UTF-8 decode">UTF-8


Indentation changed here, but it should not.

source

Starting with two elements that perform a full download, and have decent HTML/fetch integration. A element: report resource timing for download & ping Script: report resource timing for importing, classic and worker. Reporting is *always* done before load/error events are fired, so that those events could get access to the new entry.

- Use proper global object from env settings - Report before parsing - Change callback for ping

noamr · 2022-01-26T13:15:07Z

@domenic: I did a major refactor to use processResponseAndConsumeBody which makes reporting a lot more straightforward.

domenic

This works pretty well. At some point we should clean the surrounding code in the script-fetching algorithms up to use the second parameter to processResponseConsumeBody, instead of what it currently does of UTF-8 decoding response's body. But that is a preexisting problem and I'm happy to tackle it separately, now that you've factored this PR to not touch that part of things.

Tests look pretty comprehensive so I think this is ready to merge. I'll do so tomorrow unless anyone has thoughts.

noamr mentioned this pull request Oct 7, 2021

WIP: Report resource timing for most HTML elements1 #7147

Closed

3 tasks

yoavweiss reviewed Oct 8, 2021

View reviewed changes

source Show resolved Hide resolved

noamr force-pushed the report-some-resource-timing branch from 3ad0e7f to 84873cb Compare December 14, 2021 16:14

domenic requested changes Jan 18, 2022

View reviewed changes

noamr force-pushed the report-some-resource-timing branch from 0b529b6 to e51e434 Compare January 19, 2022 09:39

domenic requested changes Jan 19, 2022

View reviewed changes

noamr mentioned this pull request Jan 20, 2022

Create resource timing entries for HTML resources #6542

Closed

16 tasks

noamr force-pushed the report-some-resource-timing branch from a3322fb to 062c28b Compare January 24, 2022 15:05

noamr mentioned this pull request Jan 25, 2022

Test ResourceTiming creation for some scenarios web-platform-tests/wpt#32513

Merged

domenic requested changes Jan 25, 2022

View reviewed changes

noamr added 14 commits January 26, 2022 09:36

Make more readable

5dbcf87

Remove <a download>, it should not get an RT entry

eda1ac6

Align with fetch changes

504125d

nit

40c271d

Nit

940be2f

Fix based on CR comments:

259968e

- Use proper global object from env settings - Report before parsing - Change callback for ping

lint

ab74a4d

lint

fb60602

Fix fetch callbacks

be22da7

More CR fixes

9a996c0

client/settings

b58282c

Change worker initiatorType to other to follow implementations

28f0cdc

Refactor to use processResponseConsumeBody

75f9b84

noamr force-pushed the report-some-resource-timing branch from 062c28b to 75f9b84 Compare January 26, 2022 13:07

domenic approved these changes Jan 26, 2022

View reviewed changes

Editorial tweaks

35375d2

domenic merged commit e8e499e into whatwg:main Jan 27, 2022

noamr deleted the report-some-resource-timing branch January 28, 2022 07:50

	<span data-x="concept-settings-object-global">global object</span> and
	<span data-x="concept-settings-object-global">global object</span>, and

Report A and script loading to resource timing #7180

Report A and script loading to resource timing #7180

Uh oh!

Conversation

noamr commented Oct 7, 2021 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yoavweiss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

noamr commented Jan 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

domenic Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noamr Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noamr commented Jan 24, 2022

Uh oh!

domenic commented Jan 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noamr commented Oct 7, 2021 •

edited by pr-preview bot

Loading

domenic Jan 19, 2022 •

edited

Loading

noamr Jan 19, 2022 •

edited

Loading