-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Editorial: refactor to depend on automatic timing reporting from fetch #7722
Conversation
As long as fetch callers pass in the necessary data through the request concept, they will not have to make additional calls to get timing reported accurately. Note that this does not work if callers want to use useParallelQueue. Downstream PRs: * whatwg/html#7722 * whatwg/xhr#347 * w3c/csswg-drafts#7355 * w3c/beacon#75 * w3c/resource-timing#321 * https://github.com/w3c/navigation-timing/pull/1760 Closes #1208 and closes w3c/navigation-timing#131.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An exciting cleanup! The navigation redirects seem unlikely to be working right now though.
@@ -2620,6 +2623,7 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute | |||
<li><dfn data-x="concept-request-history-navigation-flag" data-x-href="https://fetch.spec.whatwg.org/#concept-request-history-navigation-flag">history-navigation flag</dfn></li> | |||
<li><dfn data-x="concept-request-user-activation" data-x-href="https://fetch.spec.whatwg.org/#request-user-activation">user-activation</dfn></li> | |||
<li><dfn data-x="concept-request-render-blocking" data-x-href="https://fetch.spec.whatwg.org/#request-render-blocking">render-blocking</dfn></li> | |||
<li><dfn data-x="concept-request-initiator-type" data-x-href="https://fetch.spec.whatwg.org/#concept-request-initiator-type">initiator type</dfn></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish this were not called something so similar to "initiator". Maybe "resource timing type"... Also the note "This determines which service workers will receive a fetch event for this fetch." after https://fetch.spec.whatwg.org/#request-initiator-type is misplaced.
I don't consider these blocking for this PR, but maybe you want to fix the name, in which case it might be easier to fix the name first and then update this PR before it gets merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea the whole thing with initiator types is a bit unfortunate. I think we should eventually abandon them altogether when we have an actual attribution chain (what caused this fetch)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the note "This determines which service workers will receive a fetch event for this fetch." after https://fetch.spec.whatwg.org/#request-initiator-type is misplaced.
@@ -89274,10 +89236,6 @@ interface <dfn interface>Location</dfn> { // but see also <a href="#the-location | |||
<p>If <var>failure</var> is true, then:</p> | |||
|
|||
<ol> | |||
<li><p>Call <var>navigationParams</var>'s <span | |||
data-x="navigation-params-process-response-end-of-body">process response end of body</span> | |||
with <var>response</var>.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@noamr is it intentional that we don't do any extra timing-reporting in this case, i.e. when things like CSP or XFO block a navigation?
Same for the 204s and 205s below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not needed because fetch would already report timing for these cases internally.
Unfortunately the current behavior is a cross-origin violation as per w3c/resource-timing#340. Adding a resource timing entry for 204/network-errors in lieu of a load
event exposes that information about a cross-origin-no-TAO iframe.
Note that #8233 should also be closed at that point. (Or we could close it now, I'll leave that up to you.) |
Depends on whatwg/fetch#1413
This clarifies that resource-timing reports are specific to a fetch
and not to a request or a response.
(See WHATWG Working Mode: Changes for more details.)
/browsing-the-web.html ( diff )
/iframe-embed-object.html ( diff )
/images.html ( diff )
/infrastructure.html ( diff )
/input.html ( diff )
/links.html ( diff )
/media.html ( diff )
/semantics.html ( diff )
/server-sent-events.html ( diff )
/webappapis.html ( diff )