Skip to content
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

Closed
wants to merge 7 commits into from

Conversation

noamr
Copy link
Contributor

@noamr noamr commented Mar 20, 2022

Depends on whatwg/fetch#1413

This clarifies that resource-timing reports are specific to a fetch
and not to a request or a response.

  • At least two implementers are interested (and none opposed):
    • N/a: editorial
  • Tests are written and can be reviewed and commented upon at:
    • N/a: editorial
  • Implementation bugs are filed:
    • N/a: editorial

(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 )

@noamr noamr changed the title Fetch: Use 'conclude' instead of 'finalize' Editorial: refactor to depend on automatic timing reporting from fetch May 26, 2022
annevk pushed a commit to whatwg/fetch that referenced this pull request Jun 17, 2022
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.
Copy link
Member

@domenic domenic left a 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.

source Show resolved Hide resolved
@@ -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>
Copy link
Member

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.

Copy link
Contributor Author

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)

Copy link
Contributor Author

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.

whatwg/fetch#1467

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
@@ -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>
Copy link
Member

@domenic domenic Oct 17, 2022

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.

Copy link
Contributor Author

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.

@domenic
Copy link
Member

domenic commented Oct 17, 2022

I've merged this into #6315 as 4faf1a7, so hopefully this version won't be needed, but let's keep it open until that merges...

@annevk
Copy link
Member

annevk commented Oct 18, 2022

Note that #8233 should also be closed at that point. (Or we could close it now, I'll leave that up to you.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants