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 for auto-reporting timing from fetch #347

Merged
merged 7 commits into from
Aug 15, 2022

Conversation

noamr
Copy link
Contributor

@noamr noamr commented 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.


Preview | Diff

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 noamr changed the title Use conclude instead of finalize when fetching Refactor for auto-reporting timing from fetch May 26, 2022
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 needs updating to align with the revised approach.

xhr.bs Outdated Show resolved Hide resolved
@noamr
Copy link
Contributor Author

noamr commented Jun 14, 2022

This needs updating to align with the revised approach.

Fixed

@annevk
Copy link
Member

annevk commented Jun 15, 2022

So the result here is that for synchronous XMLHttpRequest we don't create a timing entry, right?

@noamr
Copy link
Contributor Author

noamr commented Jun 15, 2022

So the result here is that for synchronous XMLHttpRequest we don't create a timing entry, right?

No, there is no behavioral change. In sync XHR the task that reports the timing would run on the parallel queue, but that's immaterial because the main-thread queue is waiting on it anyway.

@annevk
Copy link
Member

annevk commented Jun 15, 2022

I think there's two problems and they might also have been present with the previous text (though in a different way):

  1. When you're "in parallel" you don't have access to the main thread.
  2. "Pause" means nothing else can happen in the main thread (other than what it allows for). Having state changes happen during pause would be weird.

Perhaps sync XHR needs to invoke "report timing steps" itself?

@noamr
Copy link
Contributor Author

noamr commented Jun 15, 2022

I think there's two problems and they might also have been present with the previous text (though in a different way):

  1. When you're "in parallel" you don't have access to the main thread.
  2. "Pause" means nothing else can happen in the main thread (other than what it allows for). Having state changes happen during pause would be weird.

Perhaps sync XHR needs to invoke "report timing steps" itself?

Sure, that would be an easy solution. I'll revise the fetch spec to only report if not in a parallel queue

@noamr
Copy link
Contributor Author

noamr commented Jun 15, 2022

I think there's two problems and they might also have been present with the previous text (though in a different way):

  1. When you're "in parallel" you don't have access to the main thread.
  2. "Pause" means nothing else can happen in the main thread (other than what it allows for). Having state changes happen during pause would be weird.

Perhaps sync XHR needs to invoke "report timing steps" itself?

Fixed in whatwg/fetch@9f0745a and Report timing for sync XHR

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.
@noamr noamr changed the title Refactor for auto-reporting timing from fetch Editorial: Refactor for auto-reporting timing from fetch Jun 17, 2022
@noamr noamr closed this Jun 17, 2022
@noamr noamr reopened this Jun 17, 2022
@noamr noamr closed this Jun 28, 2022
@noamr noamr reopened this Jun 28, 2022
@noamr
Copy link
Contributor Author

noamr commented Jun 28, 2022

@annevk: I believe this is now ready for a re-review as it links to the corrected fetch.

@domenic
Copy link
Member

domenic commented Aug 15, 2022

This is blocking #353. I will give my own review and ping @annevk about if that's enough.

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.

Looks good modulo one phrasing suggestion.

xhr.bs Outdated
@@ -960,14 +962,13 @@ return <a>this</a>'s <a>cross-origin credentials</a>.
<li><p>If <var>processedResponse</var> is false, then set <a>this</a>'s <a>timed out flag</a> and
<a for="fetch controller">terminate</a> <a>this</a>'s <a for=XMLHttpRequest>fetch controller</a>.

<li><p>Call <a>this</a>'s <a for=XMLHttpRequest>fetch controller</a>'s
<a for="fetch controller">report timing</a> given the <a>current global object</a>.
Copy link
Member

Choose a reason for hiding this comment

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

This would be a bit more conventional as "Report timing for this's fetch controller given the current global object".

Copy link
Member

Choose a reason for hiding this comment

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

I'll push a fixup commit.

@annevk annevk merged commit 30dd504 into whatwg:main Aug 15, 2022
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.

3 participants