Enable InitiatorUrl for ResourceTiming on dedicated and shared workers [chromium/src : main]
Guohui Deng (Gerrit)
Guohui Deng added 17 comments![]( "Open in Gerrit")
Enable InitiatorUrl for ResourceTiming on worker threadGuohui DengUpdate to match recent changes?
```suggestion
Enable InitiatorUrl for ResourceTiming on dedicated and shared workers
```
Done
Guohui DengPlease add a bit of implementation context, i.e. that it enables task attribution on select worker threads to enable this (and also _why_ that's needed).
I added a paragraph. is it detailed enough?
void TaskAttributionTrackerImpl::OnBeginNestedRunLoopOnMainThread() {Guohui DengWhy change this? This is called by ThreadSchedulerBase, which is also used for the worker thread scheduler.
Scott HaseleyI am limiting this to "main thread" only. The reason is the comment in file "third_party/blink/renderer/platform/scheduler/common/thread_scheduler_base.cc"
The cleanest solution is to create the tracker in "v8_per_isolate_data.cc" only in: 1) mainworld or 2) worker. But it looks like that's difficult to do.
Guohui DengHmm I don't love this. Part of the reason I want to limit this to workers (and exclude worklets) is because of the conflict with the other usage of CPED -- TaskAttributionTracker should "own" CPED for the isolate it's associated with.
Can it be created lazily or outside of the constructor? For example, V8PerIsolateData can have a method called when the worker global scope is initialized.
Guohui DengLazy creation is a good idea! I think I can add a separate mechanism to create "tracker" in `V8PerIsolateData` for "dedicated and shared workers" if the "initiator url" feature is enabled. I will update the CL once I succeed.
Guohui DengPatchset 3) manages to create the |tracker| conditionally within V8PerIsolateData, so |tracker| won't exist for Worklets.
Lazy Creation is implemented.
// TODO(crbug.com/40919714) Captures task states in some worker worlds too.
// This function will be removed.Scott HaseleyYeah let's either remove this or change it to something like:
```suggestion
// Returns the current task state if `script_state` is associated with the main
// world or a Worker.
//
// TODO(crbug.com/41494072): Consider supporting propagation for all worlds and
// removing this function.
```
Guohui DengFYI this function was removed in https://chromium-review.googlesource.com/c/chromium/src/+/7762579
Thanks! I rebased 😊
if (RuntimeEnabledFeatures::ResourceTimingInitiatorEnabled()) {
V8PerIsolateData::From(GetIsolate())->InitializeTaskAttributionTracker();
}Guohui DengPlease move this to above the comment since that's for the line directly below this.
Done
task_attribution_tracker_ = task_attribution_tracker_factory(GetIsolate());Guohui DengLet's be consistent and only create this if kTaskAttributionInfrastructureDisabledForTesting isn't enabled.
Done
// It's the intention that these functions must be placed in a separate file.Guohui Dengnit: Add a comment as to _why_ that's the case?
Done
function fetch_in_fun(resource) {Guohui Deng```suggestion
function fetch_in_function(resource) {
```(I think it's clearer if not abbreviated here if that's okay).
Done
const promise = observe_entry_no_timeout(resource);
const entry = await promise;Guohui Dengnit: this might be more readable.
```suggestion
const entry = await observe_entry_no_timeout(resource);
```
Done
port.postMessage( {result: entry.initiatorUrl,Guohui Deng```suggestion
port.postMessage({result: entry.initiatorUrl,
```(I really wish we had auto-formatting for js files!)
Done
Thank you for catching these formatting errors! 😊
const promise = observe_entry_no_timeout(resource);
const entry = await promise;
postMessage( {result: entry.initiatorUrl,Guohui DengSame here:
```suggestion
const entry = await observe_entry_no_timeout(resource);
postMessage({result: entry.initiatorUrl,
```
Done
done();Guohui DengI think technically this isn't needed (here and elsewhere) because this uses the multi-global pattern? https://web-platform-tests.org/writing-tests/testharness.html#tests-for-other-or-multiple-globals-any-js. Or is it?
`done()`s are indeed, not needed. I am removing them.
result = event.data.result;
expected = event.data.expected;
resolveWorker();Guohui Dengnit: Fix inconsistent indentation.
```suggestion
result = event.data.result;
expected = event.data.expected;
resolveWorker();
```
Done
result = event.data.result;
expected = event.data.expected;
worker.terminate();
resolveWorker();Guohui Dengnit: Fix inconsistent indentation.
```suggestion
result = event.data.result;
expected = event.data.expected;
worker.terminate();
resolveWorker();
```
Done
scheduler.postTask( function() {fetch_in_fun(resource)} );Guohui Deng```suggestion
scheduler.postTask(function() {fetch_in_fun(resource)});
```
Done
instantPromise.then( function() {fetch_in_fun(resource)} );Guohui Deng(Here and elsewhere)
```suggestion
instantPromise.then(function() {fetch_in_fun(resource)});
```
Done
const instantPromise = Promise.resolve();
instantPromise.then( function() {load_image(label, img)} );Guohui DengMaybe clearer to omit the temporary? It's clear that it's an instantly resolved promise.
```suggestion
Promise.resolve().then(function() {load_image(label, img)});
```
Done
Related details
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 3 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
3 comments
LGTM % tiny nit
Nate: do you mind doing a second review on this?
Guohui DengPlease add a bit of implementation context, i.e. that it enables task attribution on select worker threads to enable this (and also _why_ that's needed).
I added a paragraph. is it detailed enough?
Most people working on Workers aren't familiar with this feature, so I think it's important to expand on the what/why. Ideally there would be a spec link or document to point to so people can learn more. I'd suggest including the explainer and chromestatus link in these CLs, and adding some more context: maybe append, "This allows the different initiator URLs to be propagated across async tasks (e.g. setTimeout) coming from different different scripts, e.g. importScripts()" (or something like that).
// If not on main thread,`task_attribution_tracker_` can be initializednit:
```suggestion
// If not on the main thread,`task_attribution_tracker_` can be initialized
```
Related details
- Guohui Deng
- Nate Chapin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Chapin (Gerrit)
Nate Chapin voted and added 2 comments![]( "Open in Gerrit")
Votes added by Nate Chapin
| Code-Review | +1 |
2 comments
void WorkerFetchContext::FillInitiatorInfo(FetchInitiatorInfo& initiator_info) {Potential followup: Can we merge most or all of this with `FrameFetchContext::FillInitiatorInfo` and move the logic down to `BaseFetchContext`?
void InitializeTaskAttributionTracker();Nit: let's put "OnWorkerThread" in the name here, since we `CHECK(!IsMainThread())` in the implementation.
Related details
- Guohui Deng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Guohui Deng added 4 comments![]( "Open in Gerrit")
Guohui DengPlease add a bit of implementation context, i.e. that it enables task attribution on select worker threads to enable this (and also _why_ that's needed).
Scott HaseleyI added a paragraph. is it detailed enough?
Most people working on Workers aren't familiar with this feature, so I think it's important to expand on the what/why. Ideally there would be a spec link or document to point to so people can learn more. I'd suggest including the explainer and chromestatus link in these CLs, and adding some more context: maybe append, "This allows the different initiator URLs to be propagated across async tasks (e.g. setTimeout) coming from different different scripts, e.g. importScripts()" (or something like that).
Done. I used your writing with minor editing.
void WorkerFetchContext::FillInitiatorInfo(FetchInitiatorInfo& initiator_info) {Potential followup: Can we merge most or all of this with `FrameFetchContext::FillInitiatorInfo` and move the logic down to `BaseFetchContext`?
Sounds good. I would like to look into it after it's clear what exactly we do on the worker thread. I left a comment here.
Nit: let's put "OnWorkerThread" in the name here, since we `CHECK(!IsMainThread())` in the implementation.
Done
// If not on main thread,`task_attribution_tracker_` can be initializednit:
```suggestion
// If not on the main thread,`task_attribution_tracker_` can be initialized
```
- Nate Chapin
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Chapin (Gerrit)
Nate Chapin voted Code-Review+1![]( "Open in Gerrit")
- Guohui Deng
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 2 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
LGTM
Guohui DengPlease add a bit of implementation context, i.e. that it enables task attribution on select worker threads to enable this (and also _why_ that's needed).
Scott HaseleyI added a paragraph. is it detailed enough?
Guohui DengMost people working on Workers aren't familiar with this feature, so I think it's important to expand on the what/why. Ideally there would be a spec link or document to point to so people can learn more. I'd suggest including the explainer and chromestatus link in these CLs, and adding some more context: maybe append, "This allows the different initiator URLs to be propagated across async tasks (e.g. setTimeout) coming from different different scripts, e.g. importScripts()" (or something like that).
Done. I used your writing with minor editing.
Cool, thanks.
Related details
- Guohui Deng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Guohui Deng added 2 comments![]( "Open in Gerrit")
Thank you both for reviewing. I am merging this.
void WorkerFetchContext::FillInitiatorInfo(FetchInitiatorInfo& initiator_info) {Guohui DengPotential followup: Can we merge most or all of this with `FrameFetchContext::FillInitiatorInfo` and move the logic down to `BaseFetchContext`?
Sounds good. I would like to look into it after it's clear what exactly we do on the worker thread. I left a comment here.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Guohui Deng voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/59406.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Enable InitiatorUrl for ResourceTiming on dedicated and shared workers
This CL is part of implementation of "initiator for resource timing"
feature. Explainer:
https://github.com/MicrosoftEdge/MSEdgeExplainers/blob/main/ResourceTimingInitiatorInfo/explainer.md
Chromestatus: https://chromestatus.com/feature/4876586949804032
TaskAttribution is enabled on dedicated and shared workers to facilitate
accurate initiator tracking for resources. This allows the different
initiator URLs to be propagated across async tasks (e.g. setTimeout())
coming from different scripts(e.g. importScripts()). So, we can report
initiator with better granularity than simply reporting worker script
for all the resources and still be able to track back to the caller when
an async task fetches a resource.
There are still limitations on worker thread.
What are supported:
1) Resources dynamically fetched by worker script, either
directly or via a number of common async function calls
covered by WPT tests that are added in this CL.
2) JS files imported by "importScripts()"
What will be supported in future:
1) Worker script itself;
What will be either supported or documented about if not supported:
1) Modular files used in worker thread;
2) Resources fetched in nested worker script;
3) Resources fetched in files that are imported via
"importScripts()".
- M third_party/blink/renderer/bindings/core/v8/js_based_event_listener.cc
- M third_party/blink/renderer/core/html/html_frame_owner_element.cc
- M third_party/blink/renderer/core/loader/frame_fetch_context.cc
- M third_party/blink/renderer/core/loader/resource_initiator_helper.h
- M third_party/blink/renderer/core/loader/worker_fetch_context.cc
- M third_party/blink/renderer/core/loader/worker_fetch_context.h
- M third_party/blink/renderer/core/scheduler/dom_task.cc
- M third_party/blink/renderer/core/workers/dedicated_worker_thread.cc
- M third_party/blink/renderer/core/workers/shared_worker_thread.cc
- M third_party/blink/renderer/core/workers/worker_global_scope.cc
- M third_party/blink/renderer/platform/bindings/v8_per_isolate_data.cc
- M third_party/blink/renderer/platform/bindings/v8_per_isolate_data.h
- M third_party/blink/web_tests/external/wpt/lint.ignore
- M third_party/blink/web_tests/external/wpt/resource-timing/resources/loading-resource-lib.js
- A third_party/blink/web_tests/external/wpt/resource-timing/resources/message-handler-in-shared-worker.js
- A third_party/blink/web_tests/external/wpt/resource-timing/resources/message-handler-in-worker.js
- A third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/direct.any.js
- A third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/post-message-shared-worker.html
- A third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/post-message-worker.html
- A third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/post-task.any.js
- A third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/promise.any.js
- M third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/promise.html
- A third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/queue-microtask.any.js
- A third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/set-interval.any.js
- A third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/set-timeout.any.js
Code-Review: +1 by Nate Chapin, +1 by Scott Haseley
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/59406
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |