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