Resource Timing integration #319

noamr · 2021-03-17T16:29:17Z

When response is a network error or body is fully read,
finalize and report the response.

See:
w3c/resource-timing#261
and w3c/resource-timing#252

xhr.bs

annevk

It seems that the way this works out is that there's a report timing object created just before the load event is invoked, which seems good.

xhr.bs

annevk · 2021-03-23T15:55:59Z

Note that the string formatting comment still stands I think.

xhr.bs

annevk

Apologies for not updating this sooner. While reviewing this I ended up filing whatwg/fetch#1201, but forgot to write down the bits that didn't add up for me here.

annevk · 2021-04-13T13:35:08Z

xhr.bs

@@ -1018,6 +1024,8 @@ return <a>this</a>'s <a>cross-origin credentials</a>.
 <ol>
 <li><p>Set <var>xhr</var>'s <a>state</a> to <i>done</i>.

+ <li><p><a for=/>Report timing</a> for <var>xhr</var>.


We report timings here, but two steps down we set response to a network error. Whereas above we don't report timing at all if response is a network error.

Now per whatwg/fetch#1202 maybe some network errors should get report timing, but I don't think the current setup makes that happen correctly, right?

We always call "Report timing", and let FETCH decide if to report or not (if the response has a timing info).

WDYM by "above we don't report timing at all if response is a network error", I think we do: if <var>xhr</var>'s <a for=XMLHttpRequest>response</a>'s is a <a for=/>network error</a>, run the <a>request error steps</a> for <var>xhr</var> (request error steps would report the timing).

Yeah, I think that's right. Let's solve whatwg/fetch#1215 before landing this then as currently finalize and report timing always returns early for network errors I just discovered.

I think it would also be good to ensure we have test coverage here. In the sense that the final readystatechange/load event has access to the RT entry.

Let's solve whatwg/fetch#1215 before landing this then as currently finalize and report timing always returns early for network errors I just discovered.

whatwg/fetch#1311 addressed this.

Added a requisite test here: web-platform-tests/wpt#31027

Also, report timing info for network errors for the fetch() APIs. The timing info attached to a network error is always "opaque", containing only start/end time and the original request URL. This change currently only applies to the fetch API, and should be applied to the different callers of FETCCH as part of [this](whatwg/html#6542) and [this](whatwg/xhr#319) work. Closes whatwg#1215

Test that: - A ResourceTiming entry is created for an XmlHttpRequest - The entry is available during load event handling - The entry's responseEnd time is not after the load event time - Entries are created for network errors (e.g. CORS fail, wrong scheme) See whatwg/xhr#319

* XHR ResourceTiming entry creation Test that: - A ResourceTiming entry is created for an XmlHttpRequest - The entry is available during load event handling - The entry's responseEnd time is not after the load event time - Entries are created for network errors (e.g. CORS fail, wrong scheme) See whatwg/xhr#319 * lint * Move xhr internals to resource-loaders

noamr · 2021-10-05T12:43:51Z

@annevk I think this should be OK now, in conjunction with the FETCH handling of network errors.

…tonly Automatic update from web-platform-tests XHR ResourceTiming entry creation (#31027) * XHR ResourceTiming entry creation Test that: - A ResourceTiming entry is created for an XmlHttpRequest - The entry is available during load event handling - The entry's responseEnd time is not after the load event time - Entries are created for network errors (e.g. CORS fail, wrong scheme) See whatwg/xhr#319 * lint * Move xhr internals to resource-loaders -- wpt-commits: 167763f156d113e154cf35960741dc1876f556a8 wpt-pr: 31027

* XHR ResourceTiming entry creation Test that: - A ResourceTiming entry is created for an XmlHttpRequest - The entry is available during load event handling - The entry's responseEnd time is not after the load event time - Entries are created for network errors (e.g. CORS fail, wrong scheme) See whatwg/xhr#319 * lint * Move xhr internals to resource-loaders

When response is a network error or body is fully read, finalize and report the response. Depends on whatwg/fetch#1185 See: w3c/resource-timing#261 and w3c/resource-timing#252

noamr · 2021-12-14T07:04:05Z

@annevk: revised to align with how FETCH handles network errors.
Basically we need to report in all of the network-error cases ("handle errors"), and in the "end of body" case when there is no error. In both cases we report to the timeline before any other reporting (e.g. exception) is done.

noamr · 2022-01-17T09:15:34Z

@annevk: revised to align with how FETCH handles network errors. Basically we need to report in all of the network-error cases ("handle errors"), and in the "end of body" case when there is no error. In both cases we report to the timeline before any other reporting (e.g. exception) is done.

@annevk this has been ready for a while for a review. Could you take a look when you get a chance?

xhr.bs

yoavweiss · 2022-02-09T10:35:14Z

xhr.bs

@@ -963,6 +967,8 @@ return <a>this</a>'s <a>cross-origin credentials</a>.
 <li><p>If <var>xhr</var>'s <a for=XMLHttpRequest>response</a> is a <a>network error</a>, then
 return.

+ <li><p><a for=/>Report timing</a> for <var>xhr</var>.


Could we move this to line 966, and avoid calling report timing from 2 different places?

No, because handle errors needs to report errors only in certain conditions + we need to report it if there are no errors. So there will always be two places

It seems to me we could move this up if we make it conditional upon the timed out flag? (And in the future perhaps the timed out flag can be replaced by using an aborted network error.)

Ah it wouldn't work because handle errors is also called from processBodyError, in which case the processEndOfBody callback will not be reached at all. We've been through this in early iterations of this PR

noamr force-pushed the fetch-rt-integ branch from 3225cf9 to 846b864 Compare March 17, 2021 16:42

annevk reviewed Mar 18, 2021

View reviewed changes

xhr.bs Outdated Show resolved Hide resolved

annevk reviewed Mar 22, 2021

View reviewed changes

xhr.bs Outdated Show resolved Hide resolved

xhr.bs Outdated Show resolved Hide resolved

annevk reviewed Mar 23, 2021

View reviewed changes

xhr.bs Outdated Show resolved Hide resolved

noamr changed the title ~~WIP: Resource Timing integration~~ Resource Timing integration Mar 24, 2021

annevk reviewed Mar 25, 2021

View reviewed changes

xhr.bs Outdated Show resolved Hide resolved

annevk mentioned this pull request Mar 25, 2021

New Resource Timing issues whatwg/fetch#1201

Closed

annevk reviewed Apr 13, 2021

View reviewed changes

whatwg deleted a comment from mobinskyman Apr 13, 2021

noamr mentioned this pull request Sep 30, 2021

XHR ResourceTiming entry creation web-platform-tests/wpt#31027

Merged

noamr and others added 10 commits December 14, 2021 08:49

WIP: Resource Timing integration

3e39694

When response is a network error or body is fully read, finalize and report the response. Depends on whatwg/fetch#1185 See: w3c/resource-timing#261 and w3c/resource-timing#252

Move report timing for error handling

4dc3e59

Fix PR nits

84d4776

Fix extra /OL

b64447c

Style fix

2902875

Reference the actual FETCH/RT dfn

a2c3974

Fix line sizes

3ab077a

Update xhr.bs

5d6c993

no need for additional id

a14f102

Report correctly for network errors

23490db

noamr force-pushed the fetch-rt-integ branch from ab9b848 to 23490db Compare December 14, 2021 07:02

Make sure we don't report timeouts

0b6b341

noamr closed this Feb 8, 2022

noamr reopened this Feb 8, 2022

yoavweiss reviewed Feb 9, 2022

View reviewed changes

noamr and others added 2 commits February 9, 2022 14:27

/for/with/

3abf8c9

nit

37ebd4b

annevk approved these changes Feb 11, 2022

View reviewed changes

annevk merged commit b4da01a into whatwg:main Feb 11, 2022

noamr deleted the fetch-rt-integ branch February 11, 2022 12:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resource Timing integration #319

Resource Timing integration #319

noamr commented Mar 17, 2021 •

edited by pr-preview bot

Loading

annevk left a comment

annevk commented Mar 23, 2021

annevk left a comment

annevk Apr 13, 2021

noamr Apr 14, 2021 •

edited

Loading

annevk Apr 15, 2021

noamr Sep 30, 2021

noamr Sep 30, 2021

noamr commented Oct 5, 2021

noamr commented Dec 14, 2021

noamr commented Jan 17, 2022

yoavweiss Feb 9, 2022

noamr Feb 9, 2022

annevk Feb 10, 2022

noamr Feb 10, 2022

Resource Timing integration #319

Resource Timing integration #319

Conversation

noamr commented Mar 17, 2021 • edited by pr-preview bot Loading

annevk left a comment

Choose a reason for hiding this comment

annevk commented Mar 23, 2021

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noamr Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noamr commented Oct 5, 2021

noamr commented Dec 14, 2021

noamr commented Jan 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noamr commented Mar 17, 2021 •

edited by pr-preview bot

Loading

noamr Apr 14, 2021 •

edited

Loading