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. |