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

Call finalize and report timing automatically on end-of-body #1413

Merged
merged 31 commits into from
Jun 17, 2022

Conversation

noamr
Copy link
Contributor

@noamr noamr commented Mar 16, 2022

Closes #1208
Closes w3c/navigation-timing#131

A new method called conclude reports the timing for the resource.
The timing info used is the one attached to the fetch rather than
one attached to a response.

This is done to reduce ambiguities when timing info is attached to
the response, as it's unclear what happens to that timing info
when a response is cloned, reused, serialized or stored & restored
from cache.

Call-site patches:

Update resource/navigation to use conclude / resource-info: whatwg/html#7722
Update XHR to use conclude: whatwg/xhr#347
Update CSS to use conclude: w3c/csswg-drafts#7355
Update beacon to use conclude: w3c/beacon#75
Update resource timing to use resource info: w3c/resource-timing#321
Update navigation timing to use resource info: https://github.com/w3c/navigation-timing/pull/1760

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

@noamr noamr mentioned this pull request Mar 16, 2022
4 tasks
@noamr
Copy link
Contributor Author

noamr commented Mar 16, 2022

(Note that this is a breaking change, not sure if to leave finalize response with a deprecated ref to avoid build breakage until refs are fixed? or commit this together with caller fixes?

@annevk
Copy link
Member

annevk commented Mar 18, 2022

I would prefer it if we attempt to swap it all at once. And we should also try to preserve working ID. Removing references seem fine as ideally we'd fix all of those as part of the swap.

@noamr
Copy link
Contributor Author

noamr commented Mar 18, 2022

I would prefer it if we attempt to swap it all at once. And we should also try to preserve working ID. Removing references seem fine as ideally we'd fix all of those as part of the swap.

OK, I'll add an alternate ID and work on the corresponding HTML/CSS/XHR patches

noamr added a commit to noamr/xhr that referenced this pull request Mar 20, 2022
Depends on whatwg/fetch#1413

This clarifies that a resource timing entry is always for a
fetch and not for a request or response.
noamr added a commit to noamr/html that referenced this pull request 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.
@noamr
Copy link
Contributor Author

noamr commented Mar 20, 2022

@annevk: I posted 6 related PRs...

noamr added a commit to noamr/html that referenced this pull request Apr 5, 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.
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generally looks like an improvement to me, but there's a couple of things that still need to be worked out.

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
@whatwg whatwg deleted a comment from hamad8395 Apr 21, 2022
@noamr
Copy link
Contributor Author

noamr commented May 3, 2022

Another review, @annevk ?

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned on Matrix, I think it would be good if someone else looked at this before I give it a final pass. Maybe @yutakahirano is interested?

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
@yutakahirano
Copy link
Member

As I mentioned on Matrix, I think it would be good if someone else looked at this before I give it a final pass. Maybe @yutakahirano is interested?

Are you looking for a pure code review, or do you want to know how likely Chrome implements the spec change?

@noamr
Copy link
Contributor Author

noamr commented May 12, 2022

As I mentioned on Matrix, I think it would be good if someone else looked at this before I give it a final pass. Maybe @yutakahirano is interested?

Are you looking for a pure code review, or do you want to know how likely Chrome implements the spec change?

The current spec change is purely editorial but will allow subsequent behavior-affecting changes, such as defining how encoded/decoded body size works when service workers are involved

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
@yutakahirano
Copy link
Member

I suspect you've already discussed this, but I'm not very comfortable with allowing / asking external users (xhr, html, etc) to control controller's state (abort is an exception). Is it possible / desirable to call "conclude" in the fetch spec and allow external users to set parameters (to fetch params, for example)?

@noamr
Copy link
Contributor Author

noamr commented May 18, 2022

I suspect you've already discussed this, but I'm not very comfortable with allowing / asking external users (xhr, html, etc) to control controller's state (abort is an exception). Is it possible / desirable to call "conclude" in the fetch spec and allow external users to set parameters (to fetch params, for example)?

Yes it's been discussed months ago... Also terminating is something that external specs can do.
In most cases it's possible for fetch to report timing on its own, but in some cases the caller decides to avoid reporting or to report a network error based on the response. For example, script fetching checks for particular content-types, and style subresources are not reported when fetched from a cross-origin imported stylesheet.

We chose to do it this way because then it's clear when the reporting happens relative to other observable behaviors such as element load/error events.
This is actually not new in this PR, before that the concluded state was request's "done" flag which is set from outside in finalize and report timing - here I'm attempting to make this more readable and explicit.

@noamr
Copy link
Contributor Author

noamr commented May 18, 2022

I suspect you've already discussed this, but I'm not very comfortable with allowing / asking external users (xhr, html, etc) to control controller's state (abort is an exception). Is it possible / desirable to call "conclude" in the fetch spec and allow external users to set parameters (to fetch params, for example)?

Yes it's been discussed months ago... Also terminating is something that external specs can do. In most cases it's possible for fetch to report timing on its own, but in some cases the caller decides to avoid reporting or to report a network error based on the response. For example, script fetching checks for particular content-types, and style subresources are not reported when fetched from a cross-origin imported stylesheet.

We chose to do it this way because then it's clear when the reporting happens relative to other observable behaviors such as element load/error events. This is actually not new in this PR, before that the concluded state was request's "done" flag which is set from outside in finalize and report timing - here I'm attempting to make this more readable and explicit.

One thing I'm open to is to separate timing-reporting from the state change - allow fetch to set the "concluded" state on error/EOF but leave the reporting to the caller, which can only happen post-conclude. @annevk WDYT?

@yutakahirano
Copy link
Member

One thing I'm open to is to separate timing-reporting from the state change - allow fetch to set the "concluded" state on error/EOF but leave the reporting to the caller, which can only happen post-conclude.

That sounds great to me.

@noamr
Copy link
Contributor Author

noamr commented May 18, 2022

One thing I'm open to is to separate timing-reporting from the state change - allow fetch to set the "concluded" state on error/EOF but leave the reporting to the caller, which can only happen post-conclude.

That sounds great to me.

@annevk does that sound ok to you?

@annevk
Copy link
Member

annevk commented May 23, 2022

Don't we set the "done flag" in "fetch response handover" today? Currently "finalize and report timing" does not make any state changes of that sort that I can see and it's called from one of the callback algorithms, at least in fetch(), which is what I'd expect.

@noamr
Copy link
Contributor Author

noamr commented Jun 8, 2022

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.

Triaged again, I think all comments have been handled.

@noamr
Copy link
Contributor Author

noamr commented Jun 14, 2022

@annevk: ping for review?

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I think we're close, but there's at least one somewhat involved problem left.

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Show resolved Hide resolved
fetch.bs Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented Jun 14, 2022

Triaged again, I think all comments have been handled.

To be clear, in the future please either resolve them (if you're a 100% sure) or add a comment (if you'd like another pass on that comment thread). I've now gone and resolved the older comments, but help with that is appreciated.

@noamr
Copy link
Contributor Author

noamr commented Jun 14, 2022

Triaged again, I think all comments have been handled.

To be clear, in the future please either resolve them (if you're a 100% sure) or add a comment (if you'd like another pass on that comment thread). I've now gone and resolved the older comments, but help with that is appreciated.

Sure, will do. Thanks!

@annevk
Copy link
Member

annevk commented Jun 14, 2022

Tentative commit message:

Inline "finalize and report timing"

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:
* https://github.com/whatwg/html/pull/7722
* https://github.com/whatwg/xhr/pull/347
* https://github.com/w3c/csswg-drafts/pull/7160
* https://github.com/w3c/beacon/pull/75
* https://github.com/w3c/resource-timing/pull/321
* https://github.com/w3c/navigation-timing/pull/1760

Closes #1208 and closes https://github.com/w3c/navigation-timing/pull/131.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yutakahirano any final thoughts on this?

@noamr
Copy link
Contributor Author

noamr commented Jun 14, 2022

Tentative commit message:


Inline "finalize and report timing"



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:

* https://github.com/whatwg/html/pull/7722

* https://github.com/whatwg/xhr/pull/347

* https://github.com/w3c/csswg-drafts/pull/7160

* https://github.com/w3c/beacon/pull/75

* https://github.com/w3c/resource-timing/pull/321

* https://github.com/w3c/navigation-timing/pull/1760



Closes #1208 and closes https://github.com/w3c/navigation-timing/pull/131.

Looks good! Should I update the OP or unnecessary?

@annevk
Copy link
Member

annevk commented Jun 14, 2022

No need, I'll give @yutakahirano a couple more days before I merge this.

noamr added a commit to noamr/csswg-drafts that referenced this pull request Jun 14, 2022
See whatwg/fetch#1413

Fetch now reports timing automatically if initiator is passed.
@noamr
Copy link
Contributor Author

noamr commented Jun 14, 2022

Tentative commit message:

Inline "finalize and report timing"

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:
* https://github.com/whatwg/html/pull/7722
* https://github.com/whatwg/xhr/pull/347
* https://github.com/w3c/csswg-drafts/pull/7160
* https://github.com/w3c/beacon/pull/75
* https://github.com/w3c/resource-timing/pull/321
* https://github.com/w3c/navigation-timing/pull/1760

Closes #1208 and closes https://github.com/w3c/navigation-timing/pull/131.

The correct downstream link for CSS is w3c/csswg-drafts#7355

Copy link
Member

@yutakahirano yutakahirano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM.

fetch.bs Outdated
<a for="request">initiator type</a> is non-null and <var>fetchParams</var>'s
<a for="fetch params">request</a>'s <a for=request>client</a> is non-null, then run
<var>fetchParams</var>'s <a for="fetch params">controller</a>'s
<a for="fetch controller">report timing steps</a> given <var>fetchParams</var>'s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to give only one argument ("global") to report timing steps, right?

Copy link
Contributor Author

@noamr noamr Jun 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, removed extra arg, good catch

@annevk annevk merged commit 4c93f89 into whatwg:main Jun 17, 2022
tabatkins pushed a commit to w3c/csswg-drafts that referenced this pull request Jun 17, 2022
See whatwg/fetch#1413

Fetch now reports timing automatically if initiator is passed.
annevk pushed a commit to whatwg/xhr that referenced this pull request Aug 15, 2022
Bishwarupjee pushed a commit to Bishwarupjee/xhr that referenced this pull request Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

When response comes from a worker, some of the timing-info should pass through
5 participants