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