Expose initiator-url in resourceTiming for more resources [chromium/src : main]
Scott Haseley (Gerrit)
Scott Haseley added 14 comments![]( "Open in Gerrit")
Nice! I didn't get through the tests yet, but implementation is looking pretty reasonable.
TASK_SCOPE_RESOURCE_TIMING = 10;These shouldn't be renumbered, please move the new one to the end.
v8::Isolate* isolate = v8::Isolate::TryGetCurrent();I think this block needs to be flag-guarded behind the experimental feature flag.
v8::Isolate* isolate = v8::Isolate::TryGetCurrent();Is this duplicated in the other file? It might be nice to have a helper method somewhere, maybe a static method in the new class?
if (resource_timing_context) {Wha happens if this is null and `initiator_url` ends up empty, will it use a fallback somewhere else, or is intended to be empty for now?
void FrameFetchContext::FillInitiatorInfo(FetchInitiatorInfo& initiator_info) {nit: It might be worth adding `CHECK(RuntimeEnabledFeatures::ResourceTimingInitiatorEnabled())` to document that this is only called with an experimental feature enabled.
// Initiator is a imported js file.```suggestion
// Initiator is an imported js file.
```
// The initiator is a JS.Can you expand on this comment? I think this means that we're assuming JS is running because a v8::Context was entered?
Also, is this always guaranteed to be called on the main/worker thread? Just wondering if you'd ever expect `isolate` to be null, and in what situations, since IIUC we Enter() the isolate during init (but only on relevant threads).
TaskAttributionInfo* current_task_state = CurrentTaskState();What about web scheduling task state? For soft navs, we can get away with overwriting this because we know this is only called just prior to dispatching input.
// `tracker` is null if `context` is not a Window or ifIs this a problem for resource timing? PerformanceResourceTiming is available on workers.
// 1. No plan to support `InitiatorUrl` tracking for scripts generated by
// document.write();
// 2. Tracking `InitiatorUrl` for scripts generated by document.write() is
// Not compatible with test
// `wpt_internal/task-tracking/track-doc-write.html`.Seems like (1) is a design decision, and (2) is a bug. If (2) is something that needs to be addressed, I'd link to a crbug, otherwise I don't think this needs to be commented here. I'd suggest rewriting the comment to focus on why document.write() is not supported, or just put simply that the API does not support document.write() (ideally with a link to an explainer section or something).
task_attribution_resource_timing_scope =This should be flag-guarded as well.
#ifndef THIRD_PARTY_BLINK_RENDERER_CORE_TIMING_RESOURCE_TIMING_CONTEXT_H_This needs the copyright boilerplate.
// Returns the `ResourceTimingContext` associated with the task state, whichPlease add a line break between methods.
Related details
- Guohui Deng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Guohui Deng added 14 comments![]( "Open in Gerrit")
I think I fixed the problems Scot pointed out but I also added new code, `initiator_helper` and adding `ResourceTimingContext` to `WebSchedulingTaskState`
Thanks.
These shouldn't be renumbered, please move the new one to the end.
Done
I think this block needs to be flag-guarded behind the experimental feature flag.
Done
Is this duplicated in the other file? It might be nice to have a helper method somewhere, maybe a static method in the new class?
Done. I put it to InitiatorHelper:GetIsolateIfRunningScript().
if (resource_timing_context) {Wha happens if this is null and `initiator_url` ends up empty, will it use a fallback somewhere else, or is intended to be empty for now?
For now, `InitiatorUrl` field in resource timing is an empty string if there is no valid value. An invalid |initiator_url| is converted to an empty string in `PerformanceResourceTiming::initiatorUrl()`
So it doesn't distinguish between 1) lack of implementation in UA 2) the "initiator" really doesn't exist (like the main page loaded by user action)
I expect it to remain that way until we find user case for it.
void FrameFetchContext::FillInitiatorInfo(FetchInitiatorInfo& initiator_info) {nit: It might be worth adding `CHECK(RuntimeEnabledFeatures::ResourceTimingInitiatorEnabled())` to document that this is only called with an experimental feature enabled.
Done
```suggestion
// Initiator is an imported js file.
```
Done
// The initiator is a JS.Can you expand on this comment? I think this means that we're assuming JS is running because a v8::Context was entered?
Also, is this always guaranteed to be called on the main/worker thread? Just wondering if you'd ever expect `isolate` to be null, and in what situations, since IIUC we Enter() the isolate during init (but only on relevant threads).
I made a helper function `third_party/blink/renderer/core/loader/initiator_helper.h` and I put the comments there. And I added an extra condition to align with how the Devtools determines if the initiator is JS.
TaskAttributionInfo* current_task_state = CurrentTaskState();What about web scheduling task state? For soft navs, we can get away with overwriting this because we know this is only called just prior to dispatching input.
oops, I missed it big time. Now I test if the Current task state is an instance of TaskAttributionInfoImpl or WebSchedulingTaskState, and then overwrite |resource_timing_context| accordingly.
// `tracker` is null if `context` is not a Window or ifIs this a problem for resource timing? PerformanceResourceTiming is available on workers.
I removed the comments. FYI at this CL we still not tracking the resource `initiator_url` in worker. But I plan to support that.
// 1. No plan to support `InitiatorUrl` tracking for scripts generated by
// document.write();
// 2. Tracking `InitiatorUrl` for scripts generated by document.write() is
// Not compatible with test
// `wpt_internal/task-tracking/track-doc-write.html`.Seems like (1) is a design decision, and (2) is a bug. If (2) is something that needs to be addressed, I'd link to a crbug, otherwise I don't think this needs to be commented here. I'd suggest rewriting the comment to focus on why document.write() is not supported, or just put simply that the API does not support document.write() (ideally with a link to an explainer section or something).
Done
This should be flag-guarded as well.
Done
#ifndef THIRD_PARTY_BLINK_RENDERER_CORE_TIMING_RESOURCE_TIMING_CONTEXT_H_This needs the copyright boilerplate.
Done
// Returns the `ResourceTimingContext` associated with the task state, whichPlease add a line break between methods.
- 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 added 10 comments![]( "Open in Gerrit")
The review is still a WIP. I've just gone through the task attribution bits, and they mostly LG. I'll look at the loader stuff and tests next, but wanted to send this out.
TaskAttributionInfo* current_task_state = CurrentTaskState();I think we should add a comment here as to why this is different than the other version.
Can you add something like:
```
// Unlike the other version of `SetTaskStateVariable()`, we don't check for
// `WebSchedulingTaskState` because `SoftNavigationContext` is known not be
// created in web scheduling tasks and continuations.
```
TaskAttributionInfo* current_task_state = CurrentTaskState();Guohui DengWhat about web scheduling task state? For soft navs, we can get away with overwriting this because we know this is only called just prior to dispatching input.
oops, I missed it big time. Now I test if the Current task state is an instance of TaskAttributionInfoImpl or WebSchedulingTaskState, and then overwrite |resource_timing_context| accordingly.
Nice. Left some comments about the approach (I'm interested if we can move some of the logic into the TaskAttributionTaskState classes), but this is what we need to do.
Is there test coverage for this (you'd need to do something in a scheduler.postTask() callback or requestIdleTask() callback)? The orange lines on the CL make me think it's not covered yet, although those have been wrong (I haven't reviewed tests yet).
TaskAttributionTaskState* task_state =nit: maybe `current_task_state` and `new_task_state` below, or `previous_task_state` for the existing one, and `current_task_state` for the new one (SetCurrentTaskStateImpl uses `previous_task_state`, so maybe match that).
Same for the other version.
if (auto* task_attribution_info =I wonder if it would be cleaner to add a `ForkAndSetVariable()` (or something) method to `TaskAttributionTaskState`, rather than all the casting?
next_task_id_, soft_navigation_context, resource_timing_context);nit: can you just inline this (remove the temp variable)?
TaskAttributionInfo* inner_task_attribution_info =I think you can just get the soft nav context from `task_state` here, instead of reaching for the inner state (to avoid some temp variables)
TaskAttributionTrackerImpl::SetCurrentTaskStateImpl(`TaskAttributionTaskState::GetCurrent()` is fairly optimized now, but still isn't quite as cheap as I wish it was. Can we change this to take the `previous_task_state`, since you have that in `SetTaskStateVariable`, and add an overload that passes TaskAttributionTaskState::GetCurrent() for when we don't have it?
// `tracker` is null if `context` is not a Window or ifGuohui DengIs this a problem for resource timing? PerformanceResourceTiming is available on workers.
I removed the comments. FYI at this CL we still not tracking the resource `initiator_url` in worker. But I plan to support that.
Task Attribution isn't available on Workers either, so we'll have to think that through. For the web scheduling APIs, I just use TaskAttributionTaskState::Get/SetCurrent() directly, but we can't do that elsewhere.
class CORE_EXPORT ResourceTimingContext finalnit: Please add a class comment about what this.
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 12 comments![]( "Open in Gerrit")
1. I re-wrote the logic that "fork"s the task attribution using all the great suggestions from Scott; 2. I added one more test to cover the "fork" better.
I think all the comments are addressed, but then there is a lot of new code.
Thanks for reviewing.
Cheers,
Guohui
if (resource_timing_context) {Guohui DengWha happens if this is null and `initiator_url` ends up empty, will it use a fallback somewhere else, or is intended to be empty for now?
For now, `InitiatorUrl` field in resource timing is an empty string if there is no valid value. An invalid |initiator_url| is converted to an empty string in `PerformanceResourceTiming::initiatorUrl()`
So it doesn't distinguish between 1) lack of implementation in UA 2) the "initiator" really doesn't exist (like the main page loaded by user action)
I expect it to remain that way until we find user case for it.
Mark as resolved.
// The initiator is a JS.Guohui DengCan you expand on this comment? I think this means that we're assuming JS is running because a v8::Context was entered?
Also, is this always guaranteed to be called on the main/worker thread? Just wondering if you'd ever expect `isolate` to be null, and in what situations, since IIUC we Enter() the isolate during init (but only on relevant threads).
I made a helper function `third_party/blink/renderer/core/loader/initiator_helper.h` and I put the comments there. And I added an extra condition to align with how the Devtools determines if the initiator is JS.
Marked as resolved.
TaskAttributionInfo* current_task_state = CurrentTaskState();I think we should add a comment here as to why this is different than the other version.
Can you add something like:
```
// Unlike the other version of `SetTaskStateVariable()`, we don't check for
// `WebSchedulingTaskState` because `SoftNavigationContext` is known not be
// created in web scheduling tasks and continuations.
```
These are great suggestions! I re-wrote the related code. In the new patchset,
I made `ForkAndSetVariable()` and make suggested comments in two places.
TaskAttributionInfo* current_task_state = CurrentTaskState();Guohui DengWhat about web scheduling task state? For soft navs, we can get away with overwriting this because we know this is only called just prior to dispatching input.
Scott Haseleyoops, I missed it big time. Now I test if the Current task state is an instance of TaskAttributionInfoImpl or WebSchedulingTaskState, and then overwrite |resource_timing_context| accordingly.
Nice. Left some comments about the approach (I'm interested if we can move some of the logic into the TaskAttributionTaskState classes), but this is what we need to do.
Is there test coverage for this (you'd need to do something in a scheduler.postTask() callback or requestIdleTask() callback)? The orange lines on the CL make me think it's not covered yet, although those have been wrong (I haven't reviewed tests yet).
Yes I added one more test for this purpose.
nit: maybe `current_task_state` and `new_task_state` below, or `previous_task_state` for the existing one, and `current_task_state` for the new one (SetCurrentTaskStateImpl uses `previous_task_state`, so maybe match that).
Same for the other version.
I ended up using `previous_task_state` and `next_task_state`. I see `current_task_state` is not used and I think `next_...` is more clear so I used `next..`. instead of `current_...`
I wonder if it would be cleaner to add a `ForkAndSetVariable()` (or something) method to `TaskAttributionTaskState`, rather than all the casting?
Done
next_task_id_, soft_navigation_context, resource_timing_context);nit: can you just inline this (remove the temp variable)?
Done
TaskAttributionInfo* inner_task_attribution_info =I think you can just get the soft nav context from `task_state` here, instead of reaching for the inner state (to avoid some temp variables)
I moved this logic to WebSchedulingTaskState internal function. There I think I need to get the TaskAttributionInfo to get the soft nav context.
I leave this comment unsolved for visibility.
`TaskAttributionTaskState::GetCurrent()` is fairly optimized now, but still isn't quite as cheap as I wish it was. Can we change this to take the `previous_task_state`, since you have that in `SetTaskStateVariable`, and add an overload that passes TaskAttributionTaskState::GetCurrent() for when we don't have it?
Done.
// `tracker` is null if `context` is not a Window or ifGuohui DengIs this a problem for resource timing? PerformanceResourceTiming is available on workers.
Scott HaseleyI removed the comments. FYI at this CL we still not tracking the resource `initiator_url` in worker. But I plan to support that.
Task Attribution isn't available on Workers either, so we'll have to think that through. For the web scheduling APIs, I just use TaskAttributionTaskState::Get/SetCurrent() directly, but we can't do that elsewhere.
Thanks! I didn't know that. There are a few things for `initiator_url` that are of higher priority before I get to the `resource timing` on worker. Would like to consult with you at your convenience later to sort this out.
I keep this unsolved for visibility.
nit: Please add a class comment about what this.
- 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 added 13 comments![]( "Open in Gerrit")
Nice, this is getting close!
auto* tracker = scheduler::TaskAttributionTracker::From(isolate);nit: You might want to add a helper method to InitiatorHelper for getting the URL given the isolate since appears to be the same as in the other file, or a single method that returns std::optional<> where nullopt -> script isn't running.
if (ThreadDebugger::From(isolate) && isolate->InContext()) {I don't quite understand this. The ThreadDebugger is created early in initialization (for main thread: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/core/v8/v8_initializer.cc;l=1052-1053;drc=db569cd7847d479358bd391a18b2c28393803333;bpv=1;bpt=1), so why is this check needed?
const scheduler::TaskAttributionId*,Same here.
const scheduler::TaskAttributionId*,Let's pass this by value here.
virtual TaskAttributionTaskState* ForkAndSetVariable(Can you add a comment for these?
TaskAttributionInfo* inner_task_attribution_info =Guohui DengI think you can just get the soft nav context from `task_state` here, instead of reaching for the inner state (to avoid some temp variables)
I moved this logic to WebSchedulingTaskState internal function. There I think I need to get the TaskAttributionInfo to get the soft nav context.
I leave this comment unsolved for visibility.
Acknowledged
TaskAttributionTaskState* next_task_state;nit: I think it would be marginally cleaner to initialize this in a ternary expression below to make it obvious that it's always set (same below). Otherwise this should probably be initialized to nullptr per style guide.
// Because `SoftNavigationContext` is known not be created in web scheduling
// tasks and continuations, |previous_task_state| actually cannot be a
// WebSchedulingTaskState..This probably isn't needed anymore, right? I think the comment by the NOTREACHED in WebSchedulingTaskState covers it now, and this doesn't really apply here anymore since ForkAndSet being called unconditionally.
TaskAttributionTaskState* previous_task_state =nit/optional: I'd just inline the call in the arg list and get rid of the temp variable.
// `tracker` is null if `context` is not a Window or ifGuohui DengIs this a problem for resource timing? PerformanceResourceTiming is available on workers.
Scott HaseleyI removed the comments. FYI at this CL we still not tracking the resource `initiator_url` in worker. But I plan to support that.
Guohui DengTask Attribution isn't available on Workers either, so we'll have to think that through. For the web scheduling APIs, I just use TaskAttributionTaskState::Get/SetCurrent() directly, but we can't do that elsewhere.
Thanks! I didn't know that. There are a few things for `initiator_url` that are of higher priority before I get to the `resource timing` on worker. Would like to consult with you at your convenience later to sort this out.
I keep this unsolved for visibility.
ACK.
scheduler::TaskAttributionInfo* current_task_attribution_info =nit/optional: You could also delegate to TaskAttributionImpl::ForkAndSetVariable here, but I'm not sure it's much cleaner since you'd still need to create the TaskAttributionInfoImpl in that case.
// content is used to make `PerformanceResourceTiming` report.Can you clarify this? I think it means "Its content is included in PerformanceResourceTiming" ?
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 11 comments![]( "Open in Gerrit")
Yay getting closer!
Thanks for the effort put in the review!
auto* tracker = scheduler::TaskAttributionTracker::From(isolate);nit: You might want to add a helper method to InitiatorHelper for getting the URL given the isolate since appears to be the same as in the other file, or a single method that returns std::optional<> where nullopt -> script isn't running.
Done.
`InitiatorHelper::GetScriptInitiatorUrlFromValidIsolate(isolate)`
if (ThreadDebugger::From(isolate) && isolate->InContext()) {I don't quite understand this. The ThreadDebugger is created early in initialization (for main thread: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/core/v8/v8_initializer.cc;l=1052-1053;drc=db569cd7847d479358bd391a18b2c28393803333;bpv=1;bpt=1), so why is this check needed?
Thanks for pointing this out! I realized for my purpose, I don't need this on workers. So I should not check `ThreadDebugger` like the |InspectorNetworkAgent| does.
I also added the check "main thread" and updated the comment.
const scheduler::TaskAttributionId*,Guohui DengSame here.
Done
Let's pass this by value here.
Done
Can you add a comment for these?
Done
nit: I think it would be marginally cleaner to initialize this in a ternary expression below to make it obvious that it's always set (same below). Otherwise this should probably be initialized to nullptr per style guide.
Done.
Yes it's much better now! 😊
// Because `SoftNavigationContext` is known not be created in web scheduling
// tasks and continuations, |previous_task_state| actually cannot be a
// WebSchedulingTaskState..This probably isn't needed anymore, right? I think the comment by the NOTREACHED in WebSchedulingTaskState covers it now, and this doesn't really apply here anymore since ForkAndSet being called unconditionally.
Done
nit/optional: I'd just inline the call in the arg list and get rid of the temp variable.
Done
scheduler::TaskAttributionInfo* current_task_attribution_info =nit/optional: You could also delegate to TaskAttributionImpl::ForkAndSetVariable here, but I'm not sure it's much cleaner since you'd still need to create the TaskAttributionInfoImpl in that case.
Yes I think delegating to `TaskAttributionImpl::ForkAndSetVariable` is very clear: logically it is a `TaskAttributionImpl` being forked. The reason why I didn't do that yet is that the `ForkAndSetVariable` is not exposed via `TaskAttributionInfo` interface. So I have to "cast" to `TaskAttributionImpl`. (Although the infrastructure for the cast is in place)
// content is used to make `PerformanceResourceTiming` report.Can you clarify this? I think it means "Its content is included in PerformanceResourceTiming" ?
yes that's a better way to phrase it and I modified the comment accordingly 😊
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 added 12 comments![]( "Open in Gerrit")
Okay, I think this LGTM % comments (mostly nits). I'll stamp once the merge conflict / comments are all resolved.
Adding mmocny to take a pass as well.
DCHECK(IsMainThread());nit: Can we use `CHECK` here and below (current style guidance is to prefer CHECK, unless justified: https://source.chromium.org/chromium/chromium/src/+/main:styleguide/c++/checks.md)?
// JavaScript is rurrently running for "initiator" purpose as in 2025.```suggestion
// JavaScript is rurrently running for "initiator".
```
(redundant given lead of paragraph)
Also, maybe move this down to where the call is being made (above line 31), since it's an implementation detail (vs. a description of the method).
// As in 2025, there is no V8 API that definitively tells if JavaScript```suggestion
// As of 2025, there is no V8 API that definitively tells if JavaScript
```
if (ThreadDebugger::From(isolate) && isolate->InContext()) {Guohui DengI don't quite understand this. The ThreadDebugger is created early in initialization (for main thread: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/core/v8/v8_initializer.cc;l=1052-1053;drc=db569cd7847d479358bd391a18b2c28393803333;bpv=1;bpt=1), so why is this check needed?
Thanks for pointing this out! I realized for my purpose, I don't need this on workers. So I should not check `ThreadDebugger` like the |InspectorNetworkAgent| does.
I also added the check "main thread" and updated the comment.
ACK.
// Fork to a new copy, overriding with specified TaskAttributionID andultra nit (same below): s/TaskAttributionID/TaskAttributionId (match case of type)
scheduler::TaskAttributionInfo* current_task_attribution_info =Guohui Dengnit/optional: You could also delegate to TaskAttributionImpl::ForkAndSetVariable here, but I'm not sure it's much cleaner since you'd still need to create the TaskAttributionInfoImpl in that case.
Yes I think delegating to `TaskAttributionImpl::ForkAndSetVariable` is very clear: logically it is a `TaskAttributionImpl` being forked. The reason why I didn't do that yet is that the `ForkAndSetVariable` is not exposed via `TaskAttributionInfo` interface. So I have to "cast" to `TaskAttributionImpl`. (Although the infrastructure for the cast is in place)
Yeah you'd have to cast first. The advantage of calling fork explicitly is the implementation is in place, but I'm okay with either. If we add more task attribution variables, this will need to change to something more scalable anyway.
// content is used to make `PerformanceResourceTiming` report.Guohui DengCan you clarify this? I think it means "Its content is included in PerformanceResourceTiming" ?
yes that's a better way to phrase it and I modified the comment accordingly 😊
Done
the initiator_url points to script that setups the event handler.```suggestion
the initiator_url points to script that sets up the event handler.
```
and verifies that `iniatitor_url`s in ResourceTiming are expected.nit/suggestion (here and other tests):
```suggestion
and verifies that `iniatitor_url`s in ResourceTiming entries are as expected.
```
<p> This test verifies that, for an iframe loaded by script,
the initiatorUrl is properly reported.
</p>This should work for same-frame postMessage (maybe consider adding a test?), but I'm not sure if task attribution will ever support cross-frame propagation. Adding the test with the ideal state SGTM, but we should probably (separately) discuss what should happen for this.
" initiatorUlr from postTask()", resource + " timeout");typo?
```suggestion
" initiatorUrl from postTask()", resource + " timeout");
```
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. |
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
- Guohui Deng
- Michal Mocny
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. |
Michal Mocny (Gerrit)
Michal Mocny added 7 comments![]( "Open in Gerrit")
Seems reasonable, and I appreciate all the tests including those that still fail. (I didn't review all the tests thoroughly)
The `initiator-url` is now exposed for the following resources: 1)
Resources fetched by script, in most common conditions. Resources
fetched in event handlers, and imported scripts are not covered yet. So
far there is no plan to support Other rarer conditions. 2) Iframe pages.Would you mind cleaning this up to make it clearer what IS supported after this patch, what ISNT YET supported but WILL be, vs what isnt.
I think its:
Supported:
- Resources fetched by script
- Iframe pages
Not supported (yet):
- Resources fetched by event handlers
- Imported scripts
Not supported (and unplanned):
- ? "other rare conditions"
(I see mention of stylesheet initiated scripts in the CL but I dont know if its the middle or the final category)
InitiatorHelper::GetScriptInitiatorUrlFromValidIsolate(isolate);I think it is it possible for this to return empty string-- do we want to fallback to document->Url in that case or keep empty string?
// JavaScript is rurrently running for "initiator" purpose as in 2025.```suggestion
// JavaScript is rurrently running for "initiator".
```(redundant given lead of paragraph)
Also, maybe move this down to where the call is being made (above line 31), since it's an implementation detail (vs. a description of the method).
+1 to remove redundancy and to move comment down.
I suggest something as simple as:
"We are assuming that |isolate->InContext()| indicates that JavaScript is running, though this isn't guaranteed. There is no v8 API that is truly definitive for this. This matches existing |InspectorNetworkAgent| usage."
class InitiatorHelper final {Nit: should this be ResourceInitiatorHelper? Will it expand beyond?
ResourceTimingContext*);Scott: Should we start calling these "AsyncVariable" or at least "ContextValue" or something?
{Nit: this scope spans to the end of the function-- could it be removed? (just leave unnested)
If ever someone added any followup code after `ExecuteScriptBlockInternal` would it be important that the Scope be closed?
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. |
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
Also, needs a Rebase and re-upload
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
ResourceTimingContext*);Scott: Should we start calling these "AsyncVariable" or at least "ContextValue" or something?
Probably. I think I suggested `ResourceTimingContext` to match SNC. I want to think about if we should have an `AsyncVariable` base class and generalize this (leaning towards yes, but as a follow-up). I'm thinking `AsyncVariable` + a slot in an array (vs. a map, since we have so few) might be better than leaking types here. It would make `Fork()` more scalable too. I'm okay with this as-is or changing to `ResourceTimingAsyncVariable`.
Guohui Deng (Gerrit)
Guohui Deng added 15 comments![]( "Open in Gerrit")
The `initiator-url` is now exposed for the following resources: 1)
Resources fetched by script, in most common conditions. Resources
fetched in event handlers, and imported scripts are not covered yet. So
far there is no plan to support Other rarer conditions. 2) Iframe pages.Would you mind cleaning this up to make it clearer what IS supported after this patch, what ISNT YET supported but WILL be, vs what isnt.
I think its:
Supported:
- Resources fetched by script
- Iframe pages
Not supported (yet):
- Resources fetched by event handlers
- Imported scripts
Not supported (and unplanned):
- ? "other rare conditions"
(I see mention of stylesheet initiated scripts in the CL but I dont know if its the middle or the final category)
Done. And I fixed an error in the comment about "import JS".
InitiatorHelper::GetScriptInitiatorUrlFromValidIsolate(isolate);I think it is it possible for this to return empty string-- do we want to fallback to document->Url in that case or keep empty string?
For the "initiator_url" I intend to either report the url in a definitive way or leave it blank meaning "UA hasn't implemented it"/"We don't know the value". Since
we expect that we cannot report the `initaitor_url` for all resources anyway, the goal is to provide useful information as much as possible. The provided `initiator_url` is deemed to be reliable and will be used without further investigation.
This is very different from dev tools, where they always use "fallback" if that's possible. Because for dev tools it's always better to point to somewhere so the developers can start investigating than to point to nothing.
nit: Can we use `CHECK` here and below (current style guidance is to prefer CHECK, unless justified: https://source.chromium.org/chromium/chromium/src/+/main:styleguide/c++/checks.md)?
Done
// JavaScript is rurrently running for "initiator" purpose as in 2025.Michal Mocny```suggestion
// JavaScript is rurrently running for "initiator".
```(redundant given lead of paragraph)
Also, maybe move this down to where the call is being made (above line 31), since it's an implementation detail (vs. a description of the method).
+1 to remove redundancy and to move comment down.
I suggest something as simple as:
"We are assuming that |isolate->InContext()| indicates that JavaScript is running, though this isn't guaranteed. There is no v8 API that is truly definitive for this. This matches existing |InspectorNetworkAgent| usage."
Done
// As in 2025, there is no V8 API that definitively tells if JavaScript```suggestion
// As of 2025, there is no V8 API that definitively tells if JavaScript
```
Guohui Deng
I took Michal's advice and changed the comment accordingly. I think his version is more concise.
Nit: should this be ResourceInitiatorHelper? Will it expand beyond?
Done
Scott HaseleyScott: Should we start calling these "AsyncVariable" or at least "ContextValue" or something?
Probably. I think I suggested `ResourceTimingContext` to match SNC. I want to think about if we should have an `AsyncVariable` base class and generalize this (leaning towards yes, but as a follow-up). I'm thinking `AsyncVariable` + a slot in an array (vs. a map, since we have so few) might be better than leaking types here. It would make `Fork()` more scalable too. I'm okay with this as-is or changing to `ResourceTimingAsyncVariable`.
That makes sense. I prefer to leave this as it is in this CL as it's already a very large one and Scott is O.K. with either.
I will be very happy to make improvements in a separate follow-up CL.
(Regarding of the `Fork()...`, I would like to continue the discussion at the
web_scheduling_task_state.cc)
// Fork to a new copy, overriding with specified TaskAttributionID andultra nit (same below): s/TaskAttributionID/TaskAttributionId (match case of type)
Done
// `tracker` is null if `context` is not a Window or ifGuohui DengIs this a problem for resource timing? PerformanceResourceTiming is available on workers.
Scott HaseleyI removed the comments. FYI at this CL we still not tracking the resource `initiator_url` in worker. But I plan to support that.
Guohui DengTask Attribution isn't available on Workers either, so we'll have to think that through. For the web scheduling APIs, I just use TaskAttributionTaskState::Get/SetCurrent() directly, but we can't do that elsewhere.
Scott HaseleyThanks! I didn't know that. There are a few things for `initiator_url` that are of higher priority before I get to the `resource timing` on worker. Would like to consult with you at your convenience later to sort this out.
I keep this unsolved for visibility.
ACK.
Acknowledged
scheduler::TaskAttributionInfo* current_task_attribution_info =Guohui Dengnit/optional: You could also delegate to TaskAttributionImpl::ForkAndSetVariable here, but I'm not sure it's much cleaner since you'd still need to create the TaskAttributionInfoImpl in that case.
Scott HaseleyYes I think delegating to `TaskAttributionImpl::ForkAndSetVariable` is very clear: logically it is a `TaskAttributionImpl` being forked. The reason why I didn't do that yet is that the `ForkAndSetVariable` is not exposed via `TaskAttributionInfo` interface. So I have to "cast" to `TaskAttributionImpl`. (Although the infrastructure for the cast is in place)
Yeah you'd have to cast first. The advantage of calling fork explicitly is the implementation is in place, but I'm okay with either. If we add more task attribution variables, this will need to change to something more scalable anyway.
As I commented in a different file, I would like to make the improvements in a follow-up CL.
I noticed that to delegate to TaskAttributionInfoImpl::ForkAndSetVariable(), there are 3 steps:
1) GetTaskAttributionInfo() to get member TaskAttributionInfoImpl;
2) Call Fork() on TaskAttributionInfoImpl;
3) Make a new WebSchedulingTaskState that wraps the result from previous step
Two casts are needed. The second one is from `TaskAttributionTaskState`(from `Fork`) to `TaskAttributionInfo`. Either new casting infrastructure needs to be added, or we make a `Fork` function that returns `TaskAttributionInfoImpl` type. I am not sure which one is preferable, or let's please continue the discussion offline?
Cheers,
Nit: this scope spans to the end of the function-- could it be removed? (just leave unnested)
If ever someone added any followup code after `ExecuteScriptBlockInternal` would it be important that the Scope be closed?
That scope is indeed superfluous. I removed it.
the initiator_url points to script that setups the event handler.```suggestion
the initiator_url points to script that sets up the event handler.
```
Done
and verifies that `iniatitor_url`s in ResourceTiming are expected.nit/suggestion (here and other tests):
```suggestion
and verifies that `iniatitor_url`s in ResourceTiming entries are as expected.
```
Done
<p> This test verifies that, for an iframe loaded by script,
the initiatorUrl is properly reported.
</p>This should work for same-frame postMessage (maybe consider adding a test?), but I'm not sure if task attribution will ever support cross-frame propagation. Adding the test with the ideal state SGTM, but we should probably (separately) discuss what should happen for this.
There are separate tests for postMessage (currently failing) and I would add more separate tests for that if needed. Unfortunately, `postMessage` is not supported yet at this CL.
I modified the comment here because it wasn't accurate.
" initiatorUlr from postTask()", resource + " timeout");typo?
```suggestion
" initiatorUrl from postTask()", resource + " timeout");
```
- Michal Mocny
- 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 added 7 comments![]( "Open in Gerrit")
Thanks for all the cleanup, a few more minor things.
// The initiator is a JS.nit: "a JS" is a little hard to parse -- does this mean a <script> or is it any JavaScript? I think it means "the initiator is the currently executing JavaScript." Maybe you can elaborate a bit.
// As in 2025, there is no V8 API that definitively tells if JavaScriptGuohui Deng```suggestion
// As of 2025, there is no V8 API that definitively tells if JavaScript
```
I took Michal's advice and changed the comment accordingly. I think his version is more concise.
Acknowledged
static KURL GetScriptInitiatorUrlFromValidIsolate(v8::Isolate* isolate) {nit: maybe just make this take v8::Isolate& instead of adding the suffix (i.e. remove `FromValidIsolate`? It's not too common to add a suffix like this, but rather either enforce it via a ref or document that it can't be null, which this does via CHECK.
scheduler::TaskAttributionInfo* current_task_attribution_info =Guohui Dengnit/optional: You could also delegate to TaskAttributionImpl::ForkAndSetVariable here, but I'm not sure it's much cleaner since you'd still need to create the TaskAttributionInfoImpl in that case.
Scott HaseleyYes I think delegating to `TaskAttributionImpl::ForkAndSetVariable` is very clear: logically it is a `TaskAttributionImpl` being forked. The reason why I didn't do that yet is that the `ForkAndSetVariable` is not exposed via `TaskAttributionInfo` interface. So I have to "cast" to `TaskAttributionImpl`. (Although the infrastructure for the cast is in place)
Guohui DengYeah you'd have to cast first. The advantage of calling fork explicitly is the implementation is in place, but I'm okay with either. If we add more task attribution variables, this will need to change to something more scalable anyway.
As I commented in a different file, I would like to make the improvements in a follow-up CL.
I noticed that to delegate to TaskAttributionInfoImpl::ForkAndSetVariable(), there are 3 steps:
1) GetTaskAttributionInfo() to get member TaskAttributionInfoImpl;
2) Call Fork() on TaskAttributionInfoImpl;
3) Make a new WebSchedulingTaskState that wraps the result from previous stepTwo casts are needed. The second one is from `TaskAttributionTaskState`(from `Fork`) to `TaskAttributionInfo`. Either new casting infrastructure needs to be added, or we make a `Fork` function that returns `TaskAttributionInfoImpl` type. I am not sure which one is preferable, or let's please continue the discussion offline?
Cheers,
Acknowledged
Guohui DengNit: this scope spans to the end of the function-- could it be removed? (just leave unnested)
If ever someone added any followup code after `ExecuteScriptBlockInternal` would it be important that the Scope be closed?
That scope is indeed superfluous. I removed it.
Thanks, that improves the diffs as well, which is nice.
// ExecuteScriptBlockInternal() is split just in order to preventnit: can you change this comment back to avoid git blame churn?
Related details
- Guohui Deng
- Michal Mocny
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 3 comments![]( "Open in Gerrit")
// The initiator is a JS.nit: "a JS" is a little hard to parse -- does this mean a <script> or is it any JavaScript? I think it means "the initiator is the currently executing JavaScript." Maybe you can elaborate a bit.
Done
static KURL GetScriptInitiatorUrlFromValidIsolate(v8::Isolate* isolate) {nit: maybe just make this take v8::Isolate& instead of adding the suffix (i.e. remove `FromValidIsolate`? It's not too common to add a suffix like this, but rather either enforce it via a ref or document that it can't be null, which this does via CHECK.
Done. Good idea and thanks.
// ExecuteScriptBlockInternal() is split just in order to preventnit: can you change this comment back to avoid git blame churn?
- Michal Mocny
- 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 5 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
5 comments
LGTM % a couple final nits and anything else Michal has.
I'll be in tomorrow if it needs a re-stamp, but OOO until the new year after that.
#include "third_party/blink/renderer/platform/scheduler/public/task_attribution_info.h"nit: I don't think these task attribution includes are needed any more?
// The initiator is a JS.Guohui Dengnit: "a JS" is a little hard to parse -- does this mean a <script> or is it any JavaScript? I think it means "the initiator is the currently executing JavaScript." Maybe you can elaborate a bit.
Done
Acknowledged
// We are assuming that |isolate->InContext()| indicates that JavaScriptformatting nit: looks like an extra leading space got into this comment on each line; can you fix that before landing?
```suggestion
// We are assuming that |isolate->InContext()| indicates that JavaScript
```
<p> This test verifies that, for an iframe loaded by script,
the initiatorUrl is properly reported.
</p>Guohui DengThis should work for same-frame postMessage (maybe consider adding a test?), but I'm not sure if task attribution will ever support cross-frame propagation. Adding the test with the ideal state SGTM, but we should probably (separately) discuss what should happen for this.
There are separate tests for postMessage (currently failing) and I would add more separate tests for that if needed. Unfortunately, `postMessage` is not supported yet at this CL.
I modified the comment here because it wasn't accurate.
ACK. I'd suggest adding a test for same-frame postMessage() -- which should work -- as well (e.g. third_party/blink/web_tests/wpt_internal/task-tracking/track-postmessage.html works). Can be done as a follow-up too.
Related details
- Guohui Deng
- Michal Mocny
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. |
Michal Mocny (Gerrit)
Michal Mocny voted and added 8 comments![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
8 comments
Substantively LGTM and most comments can be considered clarifications or optional suggestions.
FWIW I think the integration with task attribution, given the latest API, is quite clean and understandable-- a good sign!
And this patch adds a few more conveniences that make it even easier to use. Powerful!
The `initiator-url` is now exposed for the following resources: 1)
Resources fetched by script, in most common conditions. Resources
fetched in event handlers, and imported scripts are not covered yet. So
far there is no plan to support Other rarer conditions. 2) Iframe pages.Guohui DengWould you mind cleaning this up to make it clearer what IS supported after this patch, what ISNT YET supported but WILL be, vs what isnt.
I think its:
Supported:
- Resources fetched by script
- Iframe pages
Not supported (yet):
- Resources fetched by event handlers
- Imported scripts
Not supported (and unplanned):
- ? "other rare conditions"
(I see mention of stylesheet initiated scripts in the CL but I dont know if its the middle or the final category)
Done. And I fixed an error in the comment about "import JS".
Done-- super useful update thank you so much!
info->initiator_url = fallback_timing_info_->initiator_url;QQ: why do we set initiator_url unconditionally here, but conditionally elsewhere? Is it just because its cheap to copy here and exposing the data is guarded anyway?
(it feels weird that ~some of the use cases "do something" without the flag, and the flag guard acts as a type of documentation on related features imho)
(Feel free to resolve without changing if this is correct, im just checking)
#include "third_party/blink/renderer/platform/scheduler/public/task_attribution_info.h"nit: I don't think these task attribution includes are needed any more?
Do we have a CLI tool that helps find spurious includes?
CHECK(RuntimeEnabledFeatures::ResourceTimingInitiatorEnabled());Just confirming: it seems like we already have a feature for resource timing initiator, but without task attribution support. With this patch we extend that feature.
However, now you cannot turn off just the task-attribution parts without also turning off the current implementation (its "all or nothing").
I have no concerns about this, Im just checking that no-one is relying on the existing feature as-is?
return;Just checking: this patch used to *always* return the current document url. Now, if you have an `isolate` it will *never* return document URL, even if GetScriptInitiatorUrl is empty (i.e. due to lack of task attribution support for context propagation).
(Similarly in `HTMLFrameOwnerElement::LoadOrRedirectSubframe`)
---
Perhaps the pattern of:
1. checking for ResourceTimingInitiatorEnabled
2. then for active isolate
3. then use GetScriptInitiatorUrl
3. otherwise fallback to document URL
...should be added to ResourceInitiatorHelper rather than duplicated in the various callers? Unless we think the specifics will be customized most of the time?
ExecutionContext* context) {FWIW It confused me when reviewing to find that we have a global `SetTaskStateVariable()` that takes 2 args, both of which are a different "context" type, when we also have `TaskAttributionTrackerImpl::SetTaskStateVariable` which takes a single context...
No idea how to resolve. Maybe when "Context" rename to "Variable" or something like that this is less of a problem... Or maybe its just my lack of familiarity/intuition with the "ExecutionContext".
---
Nit: Perhaps just rename this to something like `TrySetTaskStateVariableIf...`, but I don't know what the If should be... "TrackingTaskState"? "ContextNotDestroyed"? "IsolateActive"?
scriptNit: Is there any value in `SetTaskStateVariable(nullptr)`?
If not, since you need `script` to create a context, could you just add that to the surrounding `if()` statement?
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. |
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
Nit: Is there any value in `SetTaskStateVariable(nullptr)`?
If not, since you need `script` to create a context, could you just add that to the surrounding `if()` statement?
I wondered about that too. It will override the current initiator URL if there is one, which I assumed was the desired behavior -- but maybe not?
Guohui Deng (Gerrit)
Guohui Deng added 8 comments![]( "Open in Gerrit")
I left most of the comments "unsolved" for visibility. I can close them later.
I will be OOO starting tomorrow until 29th. (Possibly I will take additional days off along the new year day in that following week)
I hope you both had a wonderful year. Wishing you a happy holiday season and a great start to the new year.
Cheers,
Guohui
info->initiator_url = fallback_timing_info_->initiator_url;QQ: why do we set initiator_url unconditionally here, but conditionally elsewhere? Is it just because its cheap to copy here and exposing the data is guarded anyway?
(it feels weird that ~some of the use cases "do something" without the flag, and the flag guard acts as a type of documentation on related features imho)
(Feel free to resolve without changing if this is correct, im just checking)
The calculation of `fallback_timing_info_->...` looks simpler than the one in a different file, because in this file, the resource is an iframe that's being loaded in a main frame, it's either:
a) Javascript running in MainThread loading an iframe
b) an iframe statically included in a html
So we use `GetIsolateIfRunningScriptOnMainThread` to determine if it's a) or b)
if it's a), we "try" to use `taskAttribution` to find initiator url (and if we cannot find it we return empty url; and if it's b), the `initiator` is the document_url.
#include "third_party/blink/renderer/platform/scheduler/public/task_attribution_info.h"Michal Mocnynit: I don't think these task attribution includes are needed any more?
Do we have a CLI tool that helps find spurious includes?
Done
CHECK(RuntimeEnabledFeatures::ResourceTimingInitiatorEnabled());Just confirming: it seems like we already have a feature for resource timing initiator, but without task attribution support. With this patch we extend that feature.
However, now you cannot turn off just the task-attribution parts without also turning off the current implementation (its "all or nothing").
I have no concerns about this, Im just checking that no-one is relying on the existing feature as-is?
Yes it's true that the "initiator" feature depends on "task attribution" feature. The dependency is added now because starting from this CL the "initiator" feature uses "task attribution".
(previously "initiator" didn't work on resources fetched by JavaScript).
return;Just checking: this patch used to *always* return the current document url. Now, if you have an `isolate` it will *never* return document URL, even if GetScriptInitiatorUrl is empty (i.e. due to lack of task attribution support for context propagation).
(Similarly in `HTMLFrameOwnerElement::LoadOrRedirectSubframe`)
---
Perhaps the pattern of:
1. checking for ResourceTimingInitiatorEnabled
2. then for active isolate
3. then use GetScriptInitiatorUrl
3. otherwise fallback to document URL...should be added to ResourceInitiatorHelper rather than duplicated in the various callers? Unless we think the specifics will be customized most of the time?
The current behavior is the desired one. The "initiator" reported in Resource timing is supposed to be collected and used directly without further investigation.
In addition, we don't expect to report the "initiator" for all resources due to the complexity and diminished return value. So we only plan to report "initiator" for a large subset of resources accurately.
(This is very different from the "initiator" in dev tools where they use fallback url and they prioritize "at least pointing to something")
So the general logic of finding "initiator" for resource timing is like this:
a) determine the type of the resource
b) based on the type of the resource, "try" to find the "initiator". If we cannot find it, we report "empty URL".
// We are assuming that |isolate->InContext()| indicates that JavaScriptformatting nit: looks like an extra leading space got into this comment on each line; can you fix that before landing?
```suggestion
// We are assuming that |isolate->InContext()| indicates that JavaScript
```
Done.
ExecutionContext* context) {FWIW It confused me when reviewing to find that we have a global `SetTaskStateVariable()` that takes 2 args, both of which are a different "context" type, when we also have `TaskAttributionTrackerImpl::SetTaskStateVariable` which takes a single context...
No idea how to resolve. Maybe when "Context" rename to "Variable" or something like that this is less of a problem... Or maybe its just my lack of familiarity/intuition with the "ExecutionContext".
---
Nit: Perhaps just rename this to something like `TrySetTaskStateVariableIf...`, but I don't know what the If should be... "TrackingTaskState"? "ContextNotDestroyed"? "IsolateActive"?
Indeed, there are two very different "context"s here and they are very confusing, in addition to the question "what is this `ExecutionContext` for".
I think your suggestions solved the problem already? :) I adopted them and now it's very clear that `resource_timing_context` is the variable value to set, and the function name implies why the "ExecutionContext" would be used.
Note: I **didn't** make any changes to the other two `SetTaskStateVariable`, where there is only one parameter, and it's very clear that such parameter is the variable value to set.
Maybe @shas...@chromium.org could take one more look at this for us and advise? Thanks a lot.
Scott HaseleyNit: Is there any value in `SetTaskStateVariable(nullptr)`?
If not, since you need `script` to create a context, could you just add that to the surrounding `if()` statement?
I wondered about that too. It will override the current initiator URL if there is one, which I assumed was the desired behavior -- but maybe not?
Done. In addition, I think the existing "initiator URL" should always be empty so either hehavir is correct and putting `script` in the `if` condition is cleaner.
Related details
- Michal Mocny
- 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 added 2 comments![]( "Open in Gerrit")
return;Guohui DengJust checking: this patch used to *always* return the current document url. Now, if you have an `isolate` it will *never* return document URL, even if GetScriptInitiatorUrl is empty (i.e. due to lack of task attribution support for context propagation).
(Similarly in `HTMLFrameOwnerElement::LoadOrRedirectSubframe`)
---
Perhaps the pattern of:
1. checking for ResourceTimingInitiatorEnabled
2. then for active isolate
3. then use GetScriptInitiatorUrl
3. otherwise fallback to document URL...should be added to ResourceInitiatorHelper rather than duplicated in the various callers? Unless we think the specifics will be customized most of the time?
The current behavior is the desired one. The "initiator" reported in Resource timing is supposed to be collected and used directly without further investigation.
In addition, we don't expect to report the "initiator" for all resources due to the complexity and diminished return value. So we only plan to report "initiator" for a large subset of resources accurately.(This is very different from the "initiator" in dev tools where they use fallback url and they prioritize "at least pointing to something")
So the general logic of finding "initiator" for resource timing is like this:
a) determine the type of the resource
b) based on the type of the resource, "try" to find the "initiator". If we cannot find it, we report "empty URL".
Can you document that here, i.e. maybe you can expand that comment to say that the URL will be empty if the code running is not task-attributable, and in that case the URL will be empty?
ExecutionContext* context) {Guohui DengFWIW It confused me when reviewing to find that we have a global `SetTaskStateVariable()` that takes 2 args, both of which are a different "context" type, when we also have `TaskAttributionTrackerImpl::SetTaskStateVariable` which takes a single context...
No idea how to resolve. Maybe when "Context" rename to "Variable" or something like that this is less of a problem... Or maybe its just my lack of familiarity/intuition with the "ExecutionContext".
---
Nit: Perhaps just rename this to something like `TrySetTaskStateVariableIf...`, but I don't know what the If should be... "TrackingTaskState"? "ContextNotDestroyed"? "IsolateActive"?
Indeed, there are two very different "context"s here and they are very confusing, in addition to the question "what is this `ExecutionContext` for".
I think your suggestions solved the problem already? :) I adopted them and now it's very clear that `resource_timing_context` is the variable value to set, and the function name implies why the "ExecutionContext" would be used.
Note: I **didn't** make any changes to the other two `SetTaskStateVariable`, where there is only one parameter, and it's very clear that such parameter is the variable value to set.
Maybe @shas...@chromium.org could take one more look at this for us and advise? Thanks a lot.
Things involving script execution, which setting a task state variable is in preparation for, will often take an `ExecutionContext`. And that can lead to competing contexts in the same method, e.g. `v8::Context` vs. `ExecutionContext`. I typically just name the variable `execution_context` in that case.
The `ExecutionContext` guard is to avoid setting task state for detached frames, since we don't run script in that case. The dual check is used because `ExecutionContextLifecyleObserver::GetExecutionContext()` returns a null context after detach (so I wanted to support that ergonomically), but you can still have a reference to a detached context (although most places, I think, likely check the for a detached context before calling these methods, but I didn't want to rely on that).
Hmm I find `IfTrackingTaskState()` more confusing. I'd either change it back and document in a function comment that this does nothing if the the EC is detached, or change it to `SetTaskStateVariableIfNotDetached()`. But FWIW that suffix isn't very comment, as we typically bail on anything script related if there's a null/detached execution context.
What about adding a comment like:
```
// Sets the given `resource_timing_context` in preparation for executing script in
// `execution_context`. Does nothing if the corresponding `execution_context` is
// not a Window or if the Window is detached.
```
?
Related details
- Guohui Deng
- Michal Mocny
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")
return;Guohui DengJust checking: this patch used to *always* return the current document url. Now, if you have an `isolate` it will *never* return document URL, even if GetScriptInitiatorUrl is empty (i.e. due to lack of task attribution support for context propagation).
(Similarly in `HTMLFrameOwnerElement::LoadOrRedirectSubframe`)
---
Perhaps the pattern of:
1. checking for ResourceTimingInitiatorEnabled
2. then for active isolate
3. then use GetScriptInitiatorUrl
3. otherwise fallback to document URL...should be added to ResourceInitiatorHelper rather than duplicated in the various callers? Unless we think the specifics will be customized most of the time?
Scott HaseleyThe current behavior is the desired one. The "initiator" reported in Resource timing is supposed to be collected and used directly without further investigation.
In addition, we don't expect to report the "initiator" for all resources due to the complexity and diminished return value. So we only plan to report "initiator" for a large subset of resources accurately.(This is very different from the "initiator" in dev tools where they use fallback url and they prioritize "at least pointing to something")
So the general logic of finding "initiator" for resource timing is like this:
a) determine the type of the resource
b) based on the type of the resource, "try" to find the "initiator". If we cannot find it, we report "empty URL".
Can you document that here, i.e. maybe you can expand that comment to say that the URL will be empty if the code running is not task-attributable, and in that case the URL will be empty?
I added some comment in file `resource_initiator_helper.h`, where the function `GetScriptInitiatorUrl` is declared and defined.
ExecutionContext* context) {Guohui DengFWIW It confused me when reviewing to find that we have a global `SetTaskStateVariable()` that takes 2 args, both of which are a different "context" type, when we also have `TaskAttributionTrackerImpl::SetTaskStateVariable` which takes a single context...
No idea how to resolve. Maybe when "Context" rename to "Variable" or something like that this is less of a problem... Or maybe its just my lack of familiarity/intuition with the "ExecutionContext".
---
Nit: Perhaps just rename this to something like `TrySetTaskStateVariableIf...`, but I don't know what the If should be... "TrackingTaskState"? "ContextNotDestroyed"? "IsolateActive"?
Scott HaseleyIndeed, there are two very different "context"s here and they are very confusing, in addition to the question "what is this `ExecutionContext` for".
I think your suggestions solved the problem already? :) I adopted them and now it's very clear that `resource_timing_context` is the variable value to set, and the function name implies why the "ExecutionContext" would be used.
Note: I **didn't** make any changes to the other two `SetTaskStateVariable`, where there is only one parameter, and it's very clear that such parameter is the variable value to set.
Maybe @shas...@chromium.org could take one more look at this for us and advise? Thanks a lot.
Things involving script execution, which setting a task state variable is in preparation for, will often take an `ExecutionContext`. And that can lead to competing contexts in the same method, e.g. `v8::Context` vs. `ExecutionContext`. I typically just name the variable `execution_context` in that case.
The `ExecutionContext` guard is to avoid setting task state for detached frames, since we don't run script in that case. The dual check is used because `ExecutionContextLifecyleObserver::GetExecutionContext()` returns a null context after detach (so I wanted to support that ergonomically), but you can still have a reference to a detached context (although most places, I think, likely check the for a detached context before calling these methods, but I didn't want to rely on that).
Hmm I find `IfTrackingTaskState()` more confusing. I'd either change it back and document in a function comment that this does nothing if the the EC is detached, or change it to `SetTaskStateVariableIfNotDetached()`. But FWIW that suffix isn't very comment, as we typically bail on anything script related if there's a null/detached execution context.
What about adding a comment like:
```
// Sets the given `resource_timing_context` in preparation for executing script in
// `execution_context`. Does nothing if the corresponding `execution_context` is
// not a Window or if the Window is detached.
```?
Done. I reverted the change and added the comment you suggested.
Related details
- Michal Mocny
- 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 1 comment![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
- Guohui Deng
- Michal Mocny
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. |
Michal Mocny (Gerrit)
Michal Mocny voted and added 5 comments![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
5 comments
info->initiator_url = fallback_timing_info_->initiator_url;Guohui DengQQ: why do we set initiator_url unconditionally here, but conditionally elsewhere? Is it just because its cheap to copy here and exposing the data is guarded anyway?
(it feels weird that ~some of the use cases "do something" without the flag, and the flag guard acts as a type of documentation on related features imho)
(Feel free to resolve without changing if this is correct, im just checking)
The calculation of `fallback_timing_info_->...` looks simpler than the one in a different file, because in this file, the resource is an iframe that's being loaded in a main frame, it's either:
a) Javascript running in MainThread loading an iframe
b) an iframe statically included in a htmlSo we use `GetIsolateIfRunningScriptOnMainThread` to determine if it's a) or b)
if it's a), we "try" to use `taskAttribution` to find initiator url (and if we cannot find it we return empty url; and if it's b), the `initiator` is the document_url.
Acknowledged
#include "third_party/blink/renderer/platform/scheduler/public/task_attribution_info.h"Michal Mocnynit: I don't think these task attribution includes are needed any more?
Guohui DengDo we have a CLI tool that helps find spurious includes?
Done
Acknowledged
CHECK(RuntimeEnabledFeatures::ResourceTimingInitiatorEnabled());Guohui DengJust confirming: it seems like we already have a feature for resource timing initiator, but without task attribution support. With this patch we extend that feature.
However, now you cannot turn off just the task-attribution parts without also turning off the current implementation (its "all or nothing").
I have no concerns about this, Im just checking that no-one is relying on the existing feature as-is?
Yes it's true that the "initiator" feature depends on "task attribution" feature. The dependency is added now because starting from this CL the "initiator" feature uses "task attribution".
(previously "initiator" didn't work on resources fetched by JavaScript).
Acknowledged
return;Guohui DengJust checking: this patch used to *always* return the current document url. Now, if you have an `isolate` it will *never* return document URL, even if GetScriptInitiatorUrl is empty (i.e. due to lack of task attribution support for context propagation).
(Similarly in `HTMLFrameOwnerElement::LoadOrRedirectSubframe`)
---
Perhaps the pattern of:
1. checking for ResourceTimingInitiatorEnabled
2. then for active isolate
3. then use GetScriptInitiatorUrl
3. otherwise fallback to document URL...should be added to ResourceInitiatorHelper rather than duplicated in the various callers? Unless we think the specifics will be customized most of the time?
Scott HaseleyThe current behavior is the desired one. The "initiator" reported in Resource timing is supposed to be collected and used directly without further investigation.
In addition, we don't expect to report the "initiator" for all resources due to the complexity and diminished return value. So we only plan to report "initiator" for a large subset of resources accurately.(This is very different from the "initiator" in dev tools where they use fallback url and they prioritize "at least pointing to something")
So the general logic of finding "initiator" for resource timing is like this:
a) determine the type of the resource
b) based on the type of the resource, "try" to find the "initiator". If we cannot find it, we report "empty URL".
Guohui DengCan you document that here, i.e. maybe you can expand that comment to say that the URL will be empty if the code running is not task-attributable, and in that case the URL will be empty?
I added some comment in file `resource_initiator_helper.h`, where the function `GetScriptInitiatorUrl` is declared and defined.
Acknowledged
ExecutionContext* context) {Guohui DengFWIW It confused me when reviewing to find that we have a global `SetTaskStateVariable()` that takes 2 args, both of which are a different "context" type, when we also have `TaskAttributionTrackerImpl::SetTaskStateVariable` which takes a single context...
No idea how to resolve. Maybe when "Context" rename to "Variable" or something like that this is less of a problem... Or maybe its just my lack of familiarity/intuition with the "ExecutionContext".
---
Nit: Perhaps just rename this to something like `TrySetTaskStateVariableIf...`, but I don't know what the If should be... "TrackingTaskState"? "ContextNotDestroyed"? "IsolateActive"?
Scott HaseleyIndeed, there are two very different "context"s here and they are very confusing, in addition to the question "what is this `ExecutionContext` for".
I think your suggestions solved the problem already? :) I adopted them and now it's very clear that `resource_timing_context` is the variable value to set, and the function name implies why the "ExecutionContext" would be used.
Note: I **didn't** make any changes to the other two `SetTaskStateVariable`, where there is only one parameter, and it's very clear that such parameter is the variable value to set.
Maybe @shas...@chromium.org could take one more look at this for us and advise? Thanks a lot.
Guohui DengThings involving script execution, which setting a task state variable is in preparation for, will often take an `ExecutionContext`. And that can lead to competing contexts in the same method, e.g. `v8::Context` vs. `ExecutionContext`. I typically just name the variable `execution_context` in that case.
The `ExecutionContext` guard is to avoid setting task state for detached frames, since we don't run script in that case. The dual check is used because `ExecutionContextLifecyleObserver::GetExecutionContext()` returns a null context after detach (so I wanted to support that ergonomically), but you can still have a reference to a detached context (although most places, I think, likely check the for a detached context before calling these methods, but I didn't want to rely on that).
Hmm I find `IfTrackingTaskState()` more confusing. I'd either change it back and document in a function comment that this does nothing if the the EC is detached, or change it to `SetTaskStateVariableIfNotDetached()`. But FWIW that suffix isn't very comment, as we typically bail on anything script related if there's a null/detached execution context.
What about adding a comment like:
```
// Sets the given `resource_timing_context` in preparation for executing script in
// `execution_context`. Does nothing if the corresponding `execution_context` is
// not a Window or if the Window is detached.
```?
Done. I reverted the change and added the comment you suggested.
Acknowledged
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 1 comment![]( "Open in Gerrit")
Need `chrome_track_event.proto` owner review. Thanks.
Related details
- Mikhail Khokhlov
- Stephen Nusko
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 1 comment![]( "Open in Gerrit")
<p> This test verifies that, for an iframe loaded by script,
the initiatorUrl is properly reported.
</p>Guohui DengThis should work for same-frame postMessage (maybe consider adding a test?), but I'm not sure if task attribution will ever support cross-frame propagation. Adding the test with the ideal state SGTM, but we should probably (separately) discuss what should happen for this.
Scott HaseleyThere are separate tests for postMessage (currently failing) and I would add more separate tests for that if needed. Unfortunately, `postMessage` is not supported yet at this CL.
I modified the comment here because it wasn't accurate.
ACK. I'd suggest adding a test for same-frame postMessage() -- which should work -- as well (e.g. third_party/blink/web_tests/wpt_internal/task-tracking/track-postmessage.html works). Can be done as a follow-up too.
Yes I will add that test along with other improvements we discussed about in the next CL.
Related details
- Mikhail Khokhlov
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikhail Khokhlov (Gerrit)
Mikhail Khokhlov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mikhail Khokhlov
| Code-Review | +1 |
1 comment
- Guohui Deng
- Stephen Nusko
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 added 1 comment![]( "Open in Gerrit")
- Stephen Nusko
Guohui Deng (Gerrit)
Guohui Deng voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
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/57009.
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
- Guohui Deng
- Stephen Nusko
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")
- Stephen Nusko
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")
Unreviewed changes
16 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
Expose initiator-url in resourceTiming for more resources
TaskAttribution is extended to track `initiator-url".
The progress of the "initiatorUrl" reporting after this CL is the below:
A) Supported:
(1) Any static resource from a html file;
(2) Resources fetched in script, in main thread, that's not
an imported script, under the condition that the resource fetch
happens after script loading or beyond a number of common ways
to dispatch an asynchronous call: postTask(); fulfillments of
a promise; queueMicrotask(); requestAnimationFrame();
requestIdleCallback(); setInterval(); setTimeout();
XMLHttpRequest();
B) Not Supported yet but planned:
(1) CSS resources that are initiated from another CSS file;
(2) Script resources that are imported JS;
(3) Resources fetched in the main thread, by the script that are
dispatched via an EventHandler, or a MessageHandler;
(4) Resource fetched by script running in a worker thread;
C) Not planned to support: any rare cases that requires significant
efforts, and any resources fetched via deprecated APIs. Example:
(1) document.write();
(2) Resources fetched in the main thread, by the script that are
dispatched via rarely used asynchronous calls.
In addition, previous tests are re-organized and refactored.
Credit to previous contributor: Some "inline-script" test cases are
merged from(with significant modification):
https://chromium-review.googlesource.com/c/chromium/src/+/4812813 And
For script initiators, TaskAttributionInfo is used according to this
draft CL:
https://chromium-review.googlesource.com/c/chromium/src/+/4931296/21
PERFETTO_TESTS=`autoninja -C out/Default perfetto_diff_tests &&
out/Default/bin/run_perfetto_diff_tests`
- M base/tracing/protos/chrome_track_event.proto
- M third_party/blink/renderer/core/html/html_frame_owner_element.cc
- M third_party/blink/renderer/core/loader/build.gni
- M third_party/blink/renderer/core/loader/frame_fetch_context.cc
- A third_party/blink/renderer/core/loader/resource_initiator_helper.h
- M third_party/blink/renderer/core/scheduler/dom_scheduler.cc
- M third_party/blink/renderer/core/scheduler/task_attribution_info_impl.cc
- M third_party/blink/renderer/core/scheduler/task_attribution_info_impl.h
- M third_party/blink/renderer/core/scheduler/task_attribution_task_state.h
- M third_party/blink/renderer/core/scheduler/task_attribution_task_state_test.cc
- M third_party/blink/renderer/core/scheduler/task_attribution_tracker_impl.cc
- M third_party/blink/renderer/core/scheduler/task_attribution_tracker_impl.h
- M third_party/blink/renderer/core/scheduler/task_attribution_util.h
- M third_party/blink/renderer/core/scheduler/web_scheduling_task_state.cc
- M third_party/blink/renderer/core/scheduler/web_scheduling_task_state.h
- M third_party/blink/renderer/core/script/pending_script.cc
- M third_party/blink/renderer/core/timing/build.gni
- A third_party/blink/renderer/core/timing/resource_timing_context.h
- M third_party/blink/renderer/platform/scheduler/public/task_attribution_info.h
- M third_party/blink/renderer/platform/scheduler/public/task_attribution_tracker.h
- M third_party/blink/web_tests/TestExpectations
- M third_party/blink/web_tests/external/wpt/lint.ignore
- A third_party/blink/web_tests/external/wpt/resource-timing/resources/iframe-post-message.html
- A 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/loading-resources-in-posted-task.js
- R third_party/blink/web_tests/external/wpt/resource-timing/resources/loading-resources.js
- A third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/event-handler.html
- A third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/external-script-in-posted-task.html
- A third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/external-script.html
- A third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/iframe-post-message.html
- M third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/inline-document-write.html
- A third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/inline-script.html
- A third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/post-task.html
- A 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.html
- A third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/request-animation-frame.html
- A third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/request-idle-callback.html
- A third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/set-interval.html
- A third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/set-timeout.html
- R third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/static-resource.html
- A third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/xml-http-request.html
- D third_party/blink/web_tests/external/wpt/resource-timing/tentative/script-initiated.html
Code-Review: +1 by Michal Mocny, +1 by Scott Haseley, +1 by Mikhail Khokhlov
| 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/57009
Related details
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 abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |