[soft-navs] SoftNavigationContext created in terms of Interactions. [chromium/src : main]
Michal Mocny (Gerrit)
Michal Mocny voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 19 comments![]( "Open in Gerrit")
This looks quite reasonable, but I need to read SNH again with a fresh brain in the morning. Some of the failed tests / crashes look interesting, but I didn't look into them.
const PerformanceTimelineEntryIdInfo interaction_id_ =I know it's rather innocuous having this, but I think it would be better to push this to the patch it's used in, unless you can do a stack and you think we'll get them both in before branch. Same for HashChangeEvent.
/*is_browser_initiated=*/false, /*is_synchronously_committed=*/true);Are these needed or should can they continue using the default values?
/*is_synchronously_committed=*/true);Is this needed, or should it just continue using the default value?
if (auto id = event_timing_scope.GetInteractionIdInfo()) {style nit: this might be clearer without auto since the type isn't obvious
(see https://google.github.io/styleguide/cppguide.html#Type_deduction).
CHECK(entry);
CHECK(entry->IsKnownToBeAnInteraction());Should these checks also be applied to the `initial_event_timing_`?
uint64_t interaction_id = initial_event_timing_->interactionId();nit: consider inlining below since this is only used once.
CHECK(window_);Why move these up? FWIW I had the CHECK(window_) below the flag check because I didn't want the check to fail in prod without the feature enabled.
uint64_t interaction_id = initial_event_timing_->interactionId();nit: same here - consider inlining below since this is only used once.
if (!event.IsFullyTrusted()) {
return std::nullopt;
}Do we lose this check? I think this is problematic since that was added to fix a crash / unexpected behavior for creating contexts during simulated events.
if (context) {How can this be null? Entries should be automatically removed by GC and never null.
// is tracking it. If the context comes from a task that crossed from
// another, we might have a different SNH instance. This seems to failnit: removing "created in one window" makes sound unclear to me now.
for (const auto& tracked_context : interaction_id_to_context_.Values()) {I think we need to treat this as a hot path (dom mods where we have a context), and I'm worried about iterating here. Maybe it's time to put a back-link on context to the heuristics its associated with?
SoftNavigationContext* context_for_task =Move above line 287 and use that in the ternary expression?
if (context && context->OnPaintFinished()) {I don't think this can be null
TRACE_EVENT_BEGIN("loading", "SoftNavigation",We really should move this into SNC...
class PerformanceEventTiming;nit: merge with other forward declarations above.
reporting_info.processing_end_time = start_time;Worth giving this some kind of duration?
/*navigation_id=*/1, PerformanceTimelineEntryIdInfo(1, 1));We could also, not necessarily now, make a base class for tests with an id generator member and this as a method, and each interaction could use this (or use the same ID via a "should_increment" parameter or something).
Related details
- 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 16 comments![]( "Open in Gerrit")
Ty for review!
Replies first, will work on addressing these next...
const PerformanceTimelineEntryIdInfo interaction_id_ =I know it's rather innocuous having this, but I think it would be better to push this to the patch it's used in, unless you can do a stack and you think we'll get them both in before branch. Same for HashChangeEvent.
Sg. This was more of an "initial direction" that proved unneeded.
/*is_browser_initiated=*/false, /*is_synchronously_committed=*/true);Are these needed or should can they continue using the default values?
good eye-- I had added interaction_id to last param initially, then switched the order specifically to get to do that-- but forgot to make this change :P
// Soft LCP. It is the startTime of the last event timing entry before first
// paint.this changed. It is now first event timing.
Pray I do not change it further :)
CHECK(entry);
CHECK(entry->IsKnownToBeAnInteraction());Should these checks also be applied to the `initial_event_timing_`?
In the constructor? Good idea!
uint64_t interaction_id = initial_event_timing_->interactionId();nit: consider inlining below since this is only used once.
I added `GetInteractionIdInfo` to this class specifically so I wouldn't need to hard-code things like this here, but missed this one. ty
CHECK(window_);Why move these up? FWIW I had the CHECK(window_) below the flag check because I didn't want the check to fail in prod without the feature enabled.
Oh I thought SoftNavigationHeuristicsEnabled needed `window_` and so moved up... ty for noticing
if (!event.IsFullyTrusted()) {
return std::nullopt;
}Do we lose this check? I think this is problematic since that was added to fix a crash / unexpected behavior for creating contexts during simulated events.
I can add the check back, but my expectation was that this check was part of the mini-interaction-event-scope machinery that SNH tried to minic and is being wholesale replaced.
I didn't think that ICP should apply to a subset of Interactions only, and that all the semantics should be up in Event Timing.
So, if this indeed still fails, I think it would be better to move up to the Event Timing scope and prevent such events from being Interactions.
---
You mention this applies to simulated events-- if you mean web automation tools, then maybe we need to keep allowing Event TIming to measure those, but should just turn off the context and leave here.
If you mean real user simulated events, if there is some gap where event timing isn't filtering enough, then we certainly should move up there.
WDYT?
// is tracking it. If the context comes from a task that crossed from
// another, we might have a different SNH instance. This seems to failnit: removing "created in one window" makes sound unclear to me now.
sg.
I wonder if EnsureContextForCurrentWindow should just be folded into GetSoftNavigationContextForCurrentTask() now that we don't pass context* around?
The only other call site is `ReportSoftNavigationToMetrics` but that is already after we emit to perf timeline and have a whole bunch of effects, so probably not needed there.
(Ill leave for this patch)
for (const auto& tracked_context : interaction_id_to_context_.Values()) {I think we need to treat this as a hot path (dom mods where we have a context), and I'm worried about iterating here. Maybe it's time to put a back-link on context to the heuristics its associated with?
Let me see if understand what you mean:
- A task can become scheduled on a window
- That task might have state which has a context*
- That task might modify dom (etc) and call into SNH static members
- Now we need to check, which can be expensive, if this SNH map for *this* window was *the* window for this context
But if we just stored the back pointer from SNC to SNC we could just check directly on the one context.
SGTM.
uint64_t interaction_id) const {TODO: try to pass the full typed struct where possible.
SoftNavigationContext* context_for_task =Move above line 287 and use that in the ternary expression?
sg, originally I didnt even have this but some of the tests failed because I made mistakes while setting them up so added this guard. It will be possible to remove completely, I just wanted to check the try-bots with it. Should have left a comment.
I could also leave this in, but add NotFatal? since the worst effect of this getting wrong is that we assign the wrong id/starttime to the wrong context. We should know, but maybe not worth crashing.
TRACE_EVENT_BEGIN("loading", "SoftNavigation",We really should move this into SNC...
I can do that. I think there are a few other reporting things in here that could, but I'll limit the moves for now.
reporting_info.processing_end_time = start_time;Worth giving this some kind of duration?
Yeah. Even better would be to merge the Utils that window_performance_test.cc uses (which has a lot more careful setup of invarients and simulation for lifecycle)...
Might have to move/rename util files around, but for now I think I'll just duplicate the code.
/*navigation_id=*/1, PerformanceTimelineEntryIdInfo(1, 1));(From AI reviewer:)
Hardcoding the interaction ID to (1, 1) can lead to incorrect test behavior. For example, in `SoftNavigationHeuristicsTest.HeuristicNotResetDuringGCWithActiveContext`, two separate click events are simulated, but they will end up with the same interaction ID and reuse the same `SoftNavigationContext`, which is likely not the intent.
Consider using an ID generator here, or passing the ID in as a parameter, similar to the fix in `InteractionEffectsMonitorTest`.
/*navigation_id=*/1, PerformanceTimelineEntryIdInfo(1, 1));We could also, not necessarily now, make a base class for tests with an id generator member and this as a method, and each interaction could use this (or use the same ID via a "should_increment" parameter or something).
I think that the InteractionID Unittest base in window_performance_test.cc does some of this and probably the soft nav / icp tests should eventually become a subclass of that... But yeah, larger test refactor for later.
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. |
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
Also, found the bug that caused tests to crash:
I added `PerformanceEventTiming::IsKnownToBeAnInteraction` but theres a subtle bug: some interactions which have an id>0 are "ignored" and treated as 0 anyway-- like autoscroll, which is where the tests were failing.
We were accidentally trying to create context with id==0 which the hashmap didn't like using as a key. Good catch, bots!
(There autoscroll are distinct from gesture scroll, which we wont count for different reasons)
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
for (const auto& tracked_context : interaction_id_to_context_.Values()) {Michal MocnyI think we need to treat this as a hot path (dom mods where we have a context), and I'm worried about iterating here. Maybe it's time to put a back-link on context to the heuristics its associated with?
Let me see if understand what you mean:
- A task can become scheduled on a window
- That task might have state which has a context*
- That task might modify dom (etc) and call into SNH static members
- Now we need to check, which can be expensive, if this SNH map for *this* window was *the* window for this context
But if we just stored the back pointer from SNC to SNC we could just check directly on the one context.
SGTM.
I added this, because I think its useful to have a backpointer... but I noticed that SNC already has a window_ and I think we can get SNH from window_, and the goal is to check for this window is the original window... I think
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. |
Michal Mocny (Gerrit)
Michal Mocny added 17 comments![]( "Open in Gerrit")
/*is_browser_initiated=*/false, /*is_synchronously_committed=*/true);Michal MocnyAre these needed or should can they continue using the default values?
good eye-- I had added interaction_id to last param initially, then switched the order specifically to get to do that-- but forgot to make this change :P
Done
Is this needed, or should it just continue using the default value?
Done
if (auto id = event_timing_scope.GetInteractionIdInfo()) {style nit: this might be clearer without auto since the type isn't obvious
(see https://google.github.io/styleguide/cppguide.html#Type_deduction).
Done
// Soft LCP. It is the startTime of the last event timing entry before first
// paint.this changed. It is now first event timing.
Pray I do not change it further :)
Done
CHECK(entry);
CHECK(entry->IsKnownToBeAnInteraction());Michal MocnyShould these checks also be applied to the `initial_event_timing_`?
In the constructor? Good idea!
Done
// `heuristics` will be null if the `window_` was detached but this contextrestore this
uint64_t interaction_id = initial_event_timing_->interactionId();Michal Mocnynit: consider inlining below since this is only used once.
I added `GetInteractionIdInfo` to this class specifically so I wouldn't need to hard-code things like this here, but missed this one. ty
Done
Michal MocnyWhy move these up? FWIW I had the CHECK(window_) below the flag check because I didn't want the check to fail in prod without the feature enabled.
Oh I thought SoftNavigationHeuristicsEnabled needed `window_` and so moved up... ty for noticing
Done
uint64_t interaction_id = initial_event_timing_->interactionId();Michal Mocnynit: same here - consider inlining below since this is only used once.
Done
How can this be null? Entries should be automatically removed by GC and never null.
Done
// is tracking it. If the context comes from a task that crossed from
// another, we might have a different SNH instance. This seems to failMichal Mocnynit: removing "created in one window" makes sound unclear to me now.
sg.
I wonder if EnsureContextForCurrentWindow should just be folded into GetSoftNavigationContextForCurrentTask() now that we don't pass context* around?
The only other call site is `ReportSoftNavigationToMetrics` but that is already after we emit to perf timeline and have a whole bunch of effects, so probably not needed there.
(Ill leave for this patch)
After the change to the "hot path" check for SNH, this function became a one liner-- so I just inlined it and remvoed it.
for (const auto& tracked_context : interaction_id_to_context_.Values()) {Michal MocnyI think we need to treat this as a hot path (dom mods where we have a context), and I'm worried about iterating here. Maybe it's time to put a back-link on context to the heuristics its associated with?
Michal MocnyLet me see if understand what you mean:
- A task can become scheduled on a window
- That task might have state which has a context*
- That task might modify dom (etc) and call into SNH static members
- Now we need to check, which can be expensive, if this SNH map for *this* window was *the* window for this context
But if we just stored the back pointer from SNC to SNC we could just check directly on the one context.
SGTM.
I added this, because I think its useful to have a backpointer... but I noticed that SNC already has a window_ and I think we can get SNH from window_, and the goal is to check for this window is the original window... I think
Done
TODO: try to pass the full typed struct where possible.
Done
I don't think this can be null
Done
Michal MocnyWe really should move this into SNC...
I can do that. I think there are a few other reporting things in here that could, but I'll limit the moves for now.
I moved a small amount of tracing now, but added a comment to the crbug to followup
nit: merge with other forward declarations above.
Done
Michal MocnyWorth giving this some kind of duration?
Yeah. Even better would be to merge the Utils that window_performance_test.cc uses (which has a lot more careful setup of invarients and simulation for lifecycle)...
Might have to move/rename util files around, but for now I think I'll just duplicate the code.
fixed a small amount here, but added comment to crbug to followup with a larger unification here.
Scott Haseley (Gerrit)
Scott Haseley added 4 comments![]( "Open in Gerrit")
Still catching up. Brain-dumping my sleep thoughts before I grab breakfast.
if (!event.IsFullyTrusted()) {
return std::nullopt;
}Michal MocnyDo we lose this check? I think this is problematic since that was added to fix a crash / unexpected behavior for creating contexts during simulated events.
I can add the check back, but my expectation was that this check was part of the mini-interaction-event-scope machinery that SNH tried to minic and is being wholesale replaced.
I didn't think that ICP should apply to a subset of Interactions only, and that all the semantics should be up in Event Timing.
So, if this indeed still fails, I think it would be better to move up to the Event Timing scope and prevent such events from being Interactions.
---
You mention this applies to simulated events-- if you mean web automation tools, then maybe we need to keep allowing Event TIming to measure those, but should just turn off the context and leave here.
If you mean real user simulated events, if there is some gap where event timing isn't filtering enough, then we certainly should move up there.
WDYT?
The problem was simulated event (untrusted) causing a trusted event. This happens because of form controls. See https://chromium-review.googlesource.com/c/chromium/src/+/7466724 (if you're passing that test, this might be okay now)
for (const auto& tracked_context : interaction_id_to_context_.Values()) {Michal MocnyI think we need to treat this as a hot path (dom mods where we have a context), and I'm worried about iterating here. Maybe it's time to put a back-link on context to the heuristics its associated with?
Michal MocnyLet me see if understand what you mean:
- A task can become scheduled on a window
- That task might have state which has a context*
- That task might modify dom (etc) and call into SNH static members
- Now we need to check, which can be expensive, if this SNH map for *this* window was *the* window for this context
But if we just stored the back pointer from SNC to SNC we could just check directly on the one context.
SGTM.
I added this, because I think its useful to have a backpointer... but I noticed that SNC already has a window_ and I think we can get SNH from window_, and the goal is to check for this window is the original window... I think
Thought of that at 4AM in my sleep too... Yeah I think we can just check context->GetWindow() == Window() (making these up). That was why this was added IIRC (cross-window main frame case w/ window.opener()).
If we want to (eventually) exclude other contexts (e.g. if we shutdown early), we can check if they've been shutdown or whatever.
SoftNavigationContext* context = GetRelevantContext(interaction_id);Since the map holds WeakMember<context>, I think there's an edge case where we can create multiple contexts for the same interaction_id. Not sure if you thought about this? I think most won't matter in practice, but the retainer dependency is a bit gnarly (it relies on the paint attribution tracker to manage lifetime).
Okay example:
- keydown
- dom update
- paint tracker holds strong reference
- keyup
finds context
Benign example:
- keydown, create context
- keydown does nothing
- GC happens -- context GCed
- keyup
- new context
Edge case:
- keydown, create context; hold key (same can happen once pointer down gets id)
- update some dom
- paint
- emit ICP
- ...
- remove said updated dom
- GC
- context GCed because no more retainers
- keyup
- no matching context for id, new context
Related details
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 1 comment![]( "Open in Gerrit")
Votes added by Michal Mocny
| Commit-Queue | +1 |
1 comment
Michal MocnyMove above line 287 and use that in the ternary expression?
sg, originally I didnt even have this but some of the tests failed because I made mistakes while setting them up so added this guard. It will be possible to remove completely, I just wanted to check the try-bots with it. Should have left a comment.
I could also leave this in, but add NotFatal? since the worst effect of this getting wrong is that we assign the wrong id/starttime to the wrong context. We should know, but maybe not worth crashing.
- 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. |
Johannes Henkel (Gerrit)
Johannes Henkel added 1 comment![]( "Open in Gerrit")
Looks that this line (processingEnd) needs to be deleted, to reflect that it's no longer part of the SoftNavigation object.
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 5 comments![]( "Open in Gerrit")
Just a couple comments, will review again after lunch.
// Soft LCP. It is the startTime of the last event timing entry before first
// paint.Michal Mocnythis changed. It is now first event timing.
Pray I do not change it further :)
Done
😄
if (!event.IsFullyTrusted()) {
return std::nullopt;
}Michal MocnyDo we lose this check? I think this is problematic since that was added to fix a crash / unexpected behavior for creating contexts during simulated events.
Scott HaseleyI can add the check back, but my expectation was that this check was part of the mini-interaction-event-scope machinery that SNH tried to minic and is being wholesale replaced.
I didn't think that ICP should apply to a subset of Interactions only, and that all the semantics should be up in Event Timing.
So, if this indeed still fails, I think it would be better to move up to the Event Timing scope and prevent such events from being Interactions.
---
You mention this applies to simulated events-- if you mean web automation tools, then maybe we need to keep allowing Event TIming to measure those, but should just turn off the context and leave here.
If you mean real user simulated events, if there is some gap where event timing isn't filtering enough, then we certainly should move up there.
WDYT?
The problem was simulated event (untrusted) causing a trusted event. This happens because of form controls. See https://chromium-review.googlesource.com/c/chromium/src/+/7466724 (if you're passing that test, this might be okay now)
Ah, yeah it's still failing.
The problem is that if you dispatch a simulated click to an element with a target, we dispatch a trusted click to the target. So we create EventTimings for such events, but soft navs filters them out.
I suspect these should be filtered out from EventTiming as well, and maybe that's relatively safe to do? Not sure.
// is tracking it. If the context comes from a task that crossed from
// another, we might have a different SNH instance. This seems to failMichal Mocnynit: removing "created in one window" makes sound unclear to me now.
Michal Mocnysg.
I wonder if EnsureContextForCurrentWindow should just be folded into GetSoftNavigationContextForCurrentTask() now that we don't pass context* around?
The only other call site is `ReportSoftNavigationToMetrics` but that is already after we emit to perf timeline and have a whole bunch of effects, so probably not needed there.
(Ill leave for this patch)
After the change to the "hot path" check for SNH, this function became a one liner-- so I just inlined it and remvoed it.
Sweet!
// TODO(crbug.com/40871933): We don't care to support datetime modals, but
// this behaviour might be similar for iframes, and might be worth
// supporting.note: We ran into this in practice with window.open(), in which case you get two main frames in the same process that can script each other.
Related details
- 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 6 comments![]( "Open in Gerrit")
// `heuristics` will be null if the `window_` was detached but this contextMichal Mocnyrestore this
Done
if (!event.IsFullyTrusted()) {
return std::nullopt;
}Michal MocnyDo we lose this check? I think this is problematic since that was added to fix a crash / unexpected behavior for creating contexts during simulated events.
Scott HaseleyI can add the check back, but my expectation was that this check was part of the mini-interaction-event-scope machinery that SNH tried to minic and is being wholesale replaced.
I didn't think that ICP should apply to a subset of Interactions only, and that all the semantics should be up in Event Timing.
So, if this indeed still fails, I think it would be better to move up to the Event Timing scope and prevent such events from being Interactions.
---
You mention this applies to simulated events-- if you mean web automation tools, then maybe we need to keep allowing Event TIming to measure those, but should just turn off the context and leave here.
If you mean real user simulated events, if there is some gap where event timing isn't filtering enough, then we certainly should move up there.
WDYT?
The problem was simulated event (untrusted) causing a trusted event. This happens because of form controls. See https://chromium-review.googlesource.com/c/chromium/src/+/7466724 (if you're passing that test, this might be okay now)
Done
for (const auto& tracked_context : interaction_id_to_context_.Values()) {Michal MocnyI think we need to treat this as a hot path (dom mods where we have a context), and I'm worried about iterating here. Maybe it's time to put a back-link on context to the heuristics its associated with?
Michal MocnyLet me see if understand what you mean:
- A task can become scheduled on a window
- That task might have state which has a context*
- That task might modify dom (etc) and call into SNH static members
- Now we need to check, which can be expensive, if this SNH map for *this* window was *the* window for this context
But if we just stored the back pointer from SNC to SNC we could just check directly on the one context.
SGTM.
Scott HaseleyI added this, because I think its useful to have a backpointer... but I noticed that SNC already has a window_ and I think we can get SNH from window_, and the goal is to check for this window is the original window... I think
Thought of that at 4AM in my sleep too... Yeah I think we can just check context->GetWindow() == Window() (making these up). That was why this was added IIRC (cross-window main frame case w/ window.opener()).
If we want to (eventually) exclude other contexts (e.g. if we shutdown early), we can check if they've been shutdown or whatever.
Done
// TODO(crbug.com/40871933): We don't care to support datetime modals, but
// this behaviour might be similar for iframes, and might be worth
// supporting.note: We ran into this in practice with window.open(), in which case you get two main frames in the same process that can script each other.
(this comment just moved from somewhere earlier, I didn't add it-- I hope nothing abut the specifics change here.)
SoftNavigationContext* context = GetRelevantContext(interaction_id);Great points-- but I think this would be WAI and the fact that each attempt to get a context by id with an event timing doesn't assume there was a past event means this should just work.
Or do you spot a problem?
We should add tests for it afterwards!
/*navigation_id=*/1, PerformanceTimelineEntryIdInfo(1, 1));(From AI reviewer:)
Hardcoding the interaction ID to (1, 1) can lead to incorrect test behavior. For example, in `SoftNavigationHeuristicsTest.HeuristicNotResetDuringGCWithActiveContext`, two separate click events are simulated, but they will end up with the same interaction ID and reuse the same `SoftNavigationContext`, which is likely not the intent.
Consider using an ID generator here, or passing the ID in as a parameter, similar to the fix in `InteractionEffectsMonitorTest`.
Cannot use a generator here, but we will move this into a test base that will do this, in a subsequent patch. The tests that need separate ids can manually set them, for now.
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. |
Michal Mocny (Gerrit)
Michal Mocny voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michal Mocny
| Commit-Queue | +1 |
Looks that this line (processingEnd) needs to be deleted, to reflect that it's no longer part of the SoftNavigation object.
I added it back to tracing for now, I thought it wasn't needed at all but forgot about these tests.
We could also expose more of the event timings if we want... or even the full list of them.
Related details
- Johannes Henkel
- 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")
InteractionContextWrapperScope interaction_context_wrapper;nit/optional: maybe comment that browser-initiated navigations are considered interactions and this scope sets up handling that for metrics?
}ultra-nit: probably keep the newline before this and add one after to separate the task-attribution block
// is wrapper in this scope, it std::move() the task_scope up to it instead.```suggestion
// is wrapped in this scope, it std::move()d the task_scope up to it instead.
```
// This class is used to "hoist" a `TaskAttributionTracker::TaskScope` up to theThinking about this a bit more, I think the hoisting is a problem. Having the outer scope is nice, but it would need to initialize the state. The invariant is supposed to be that all TaskScopes are "properly nested", like trace events, and I think this breaks that.
For example, suppose you have A() calls B() calls C().
- A() runs, creating an outer InteractionContextWrapperScope scope, initially with the nullopt TaskScope
- A() calls B(), which sets a task state variable (new TaskScope). That scope will capture the current task state (nothing) and restore that upon completion
- B() calls C(), which initializes the outer scope's TaskScope. The current task state (B's state) is captured as the "state to restore".
- C() ends
- B() ends. The null state is restored --> (outer scope is borked!)
- A() ends. B's state is restored --> state leak!
(I actually think I need to nix the TaskScope move c'tors!)
if (!event.IsFullyTrusted()) {Do you know if there's a spec issue for this? This could be a compat issue.
void AddEventTiming(PerformanceEventTiming* entry);Can we remove this? `last_event_timing_` doesn't matter anymore now that you're using the first one.
explicit SoftNavigationContext(LocalDOMWindow& window,nit: can be removed now
TRACE_EVENT_BEGIN("loading", "SoftNavigation",nice
CHECK(initial_event_timing_);nit: You could probably (?) make `initial_event_timing_` const and/or remove this check, since it's already guaranteed to be valid in the c'tor and we don't change it.
if (auto* heuristics = window_->GetSoftNavigationHeuristics()) {Did something change that can make this null now, like in tests? Previously, the heuristics was guaranteed to be valid because:
- It's created by the heuristics class (except in tests)
- if window_ had a heuristics, it's guaranteed to stay until detach
- This is shut down on detatch, and OnPaintFinished() shouldn't be called after detach
if (auto* heuristics = window_->GetSoftNavigationHeuristics()) {You didn't like my style? :P
(FWIW I did that to be consistent with the `if (!condition) { return }` and have the actual logic at the end, as is somewhat idiomatic in chrome (not always tho..))
if (auto* heuristics = window_->GetSoftNavigationHeuristics()) {Similar question, have the guarantees changed? This should never be called after detach.
The problem with the last one is the non-determinism -- sometimes things are grouped, sometimes they're not, depending on GC.
I think it's fine for OT_v1 and shouldn't block this, but I think there's a disconnect in the layering that maybe we can fix when InteractionContext moves out of soft navs, like maybe the context creation and mapping needs to be built into the responsiveness metrics state machine (w/ stronger lifetime guarantees).
Related details
- Johannes Henkel
- 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. |
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
// This class is used to "hoist" a `TaskAttributionTracker::TaskScope` up to theThinking about this a bit more, I think the hoisting is a problem. Having the outer scope is nice, but it would need to initialize the state. The invariant is supposed to be that all TaskScopes are "properly nested", like trace events, and I think this breaks that.
For example, suppose you have A() calls B() calls C().
- A() runs, creating an outer InteractionContextWrapperScope scope, initially with the nullopt TaskScope
- A() calls B(), which sets a task state variable (new TaskScope). That scope will capture the current task state (nothing) and restore that upon completion
- B() calls C(), which initializes the outer scope's TaskScope. The current task state (B's state) is captured as the "state to restore".
- C() ends
- B() ends. The null state is restored --> (outer scope is borked!)
- A() ends. B's state is restored --> state leak!(I actually think I need to nix the TaskScope move c'tors!)
Update: I think this is something task attribution should solve, not event timing. Really this is a problem with propagating task state from the intercept() to handler(), which is clear causation. Let me try to add that.
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
// This class is used to "hoist" a `TaskAttributionTracker::TaskScope` up to theScott HaseleyThinking about this a bit more, I think the hoisting is a problem. Having the outer scope is nice, but it would need to initialize the state. The invariant is supposed to be that all TaskScopes are "properly nested", like trace events, and I think this breaks that.
For example, suppose you have A() calls B() calls C().
- A() runs, creating an outer InteractionContextWrapperScope scope, initially with the nullopt TaskScope
- A() calls B(), which sets a task state variable (new TaskScope). That scope will capture the current task state (nothing) and restore that upon completion
- B() calls C(), which initializes the outer scope's TaskScope. The current task state (B's state) is captured as the "state to restore".
- C() ends
- B() ends. The null state is restored --> (outer scope is borked!)
- A() ends. B's state is restored --> state leak!(I actually think I need to nix the TaskScope move c'tors!)
Update: I think this is something task attribution should solve, not event timing. Really this is a problem with propagating task state from the intercept() to handler(), which is clear causation. Let me try to add that.
Something like https://chromium-review.googlesource.com/c/chromium/src/+/7645691. That makes the last wpt_internal test pass w/ the previous version of this patch.
It turned out to be annoying because there can be a TaskScope higher up because of pushState, so we need different behavior than what we do for events (i.e. we have to overwrite even if JS is running). This could be an interesting thing to discuss for AsyncContext.
Michal Mocny (Gerrit)
Michal Mocny added 4 comments![]( "Open in Gerrit")
// This class is used to "hoist" a `TaskAttributionTracker::TaskScope` up to theScott HaseleyThinking about this a bit more, I think the hoisting is a problem. Having the outer scope is nice, but it would need to initialize the state. The invariant is supposed to be that all TaskScopes are "properly nested", like trace events, and I think this breaks that.
For example, suppose you have A() calls B() calls C().
- A() runs, creating an outer InteractionContextWrapperScope scope, initially with the nullopt TaskScope
- A() calls B(), which sets a task state variable (new TaskScope). That scope will capture the current task state (nothing) and restore that upon completion
- B() calls C(), which initializes the outer scope's TaskScope. The current task state (B's state) is captured as the "state to restore".
- C() ends
- B() ends. The null state is restored --> (outer scope is borked!)
- A() ends. B's state is restored --> state leak!(I actually think I need to nix the TaskScope move c'tors!)
Scott HaseleyUpdate: I think this is something task attribution should solve, not event timing. Really this is a problem with propagating task state from the intercept() to handler(), which is clear causation. Let me try to add that.
Something like https://chromium-review.googlesource.com/c/chromium/src/+/7645691. That makes the last wpt_internal test pass w/ the previous version of this patch.
It turned out to be annoying because there can be a TaskScope higher up because of pushState, so we need different behavior than what we do for events (i.e. we have to overwrite even if JS is running). This could be an interesting thing to discuss for AsyncContext.
Sweet! taking a look.
And yeah this makes me feel a lot better about :D
if (!event.IsFullyTrusted()) {Do you know if there's a spec issue for this? This could be a compat issue.
I havent filed one, yet. A first pass suggests there is no isFullyTrusted spec concept, so we could either explain the use case, or just explain in terms of "userInitiated" as part of adding navigate.
Also, this comes to mind: https://github.com/w3c/largest-contentful-paint/issues/105
CHECK(initial_event_timing_);nit: You could probably (?) make `initial_event_timing_` const and/or remove this check, since it's already guaranteed to be valid in the c'tor and we don't change it.
This check actually failed on the tryjobs on this test yesterday: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/page_load_metrics/integration_tests/first_input_delay_browsertest.cc;l=18?q=MetricIntegrationTest.FirstInputDelay
It looks like the test simulated input then unloads the page.
I wonder if we are reporting SNC (to tracing / metric?) during teardown and no longer have the event timing entry?
SoftNavigationContext* context = GetRelevantContext(interaction_id);So, either we have:
- Context_v1_for_interaction1 for all events, or
- Context_v1_fro_interaction1, then Context_v2_fro_interaction1
Two distinct context, but for the same interaction id, and ultimately matching the same paints (because v1 will only gc if it has no effects).
The only difference is that today we will report the startTime for the first event vs second event-- but this is already something I want to change by default: the "last event before first effect" should roughly match intuitions, but for practical reasons might need to be "last task/animation frame before first effect, and the first event within that task/animation frame".
Filed: crbug.com/490552221
Related details
- Johannes Henkel
- 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. |
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
// This class is used to "hoist" a `TaskAttributionTracker::TaskScope` up to theScott HaseleyThinking about this a bit more, I think the hoisting is a problem. Having the outer scope is nice, but it would need to initialize the state. The invariant is supposed to be that all TaskScopes are "properly nested", like trace events, and I think this breaks that.
For example, suppose you have A() calls B() calls C().
- A() runs, creating an outer InteractionContextWrapperScope scope, initially with the nullopt TaskScope
- A() calls B(), which sets a task state variable (new TaskScope). That scope will capture the current task state (nothing) and restore that upon completion
- B() calls C(), which initializes the outer scope's TaskScope. The current task state (B's state) is captured as the "state to restore".
- C() ends
- B() ends. The null state is restored --> (outer scope is borked!)
- A() ends. B's state is restored --> state leak!(I actually think I need to nix the TaskScope move c'tors!)
Scott HaseleyUpdate: I think this is something task attribution should solve, not event timing. Really this is a problem with propagating task state from the intercept() to handler(), which is clear causation. Let me try to add that.
Michal MocnySomething like https://chromium-review.googlesource.com/c/chromium/src/+/7645691. That makes the last wpt_internal test pass w/ the previous version of this patch.
It turned out to be annoying because there can be a TaskScope higher up because of pushState, so we need different behavior than what we do for events (i.e. we have to overwrite even if JS is running). This could be an interesting thing to discuss for AsyncContext.
Sweet! taking a look.
And yeah this makes me feel a lot better about :D
Acknowledged
Michal Mocny (Gerrit)
Michal Mocny added 10 comments![]( "Open in Gerrit")
const PerformanceTimelineEntryIdInfo interaction_id_ =I know it's rather innocuous having this, but I think it would be better to push this to the patch it's used in, unless you can do a stack and you think we'll get them both in before branch. Same for HashChangeEvent.
Sg. This was more of an "initial direction" that proved unneeded.
Done
InteractionContextWrapperScope interaction_context_wrapper;nit/optional: maybe comment that browser-initiated navigations are considered interactions and this scope sets up handling that for metrics?
Done
ultra-nit: probably keep the newline before this and add one after to separate the task-attribution block
Done
// is wrapper in this scope, it std::move() the task_scope up to it instead.```suggestion
// is wrapped in this scope, it std::move()d the task_scope up to it instead.
```
Done
void AddEventTiming(PerformanceEventTiming* entry);Can we remove this? `last_event_timing_` doesn't matter anymore now that you're using the first one.
Done
explicit SoftNavigationContext(LocalDOMWindow& window,nit: can be removed now
Done
CHECK(initial_event_timing_);Michal Mocnynit: You could probably (?) make `initial_event_timing_` const and/or remove this check, since it's already guaranteed to be valid in the c'tor and we don't change it.
This check actually failed on the tryjobs on this test yesterday: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/page_load_metrics/integration_tests/first_input_delay_browsertest.cc;l=18?q=MetricIntegrationTest.FirstInputDelay
It looks like the test simulated input then unloads the page.
I wonder if we are reporting SNC (to tracing / metric?) during teardown and no longer have the event timing entry?
Done
if (auto* heuristics = window_->GetSoftNavigationHeuristics()) {Did something change that can make this null now, like in tests? Previously, the heuristics was guaranteed to be valid because:
- It's created by the heuristics class (except in tests)
- if window_ had a heuristics, it's guaranteed to stay until detach
- This is shut down on detatch, and OnPaintFinished() shouldn't be called after detach
ty!
Since I added a helper to this class to GetSoftNavigationHeuristics() I just changed all the call sites to use that. If we need to enforce checks we can do it in one place now (but I dont)
if (auto* heuristics = window_->GetSoftNavigationHeuristics()) {You didn't like my style? :P
(FWIW I did that to be consistent with the `if (!condition) { return }` and have the actual logic at the end, as is somewhat idiomatic in chrome (not always tho..))
I was trying to make all the call sites consistent, but I have now reverted to the previous style because (a) I agree I like this style more, (b) the other call sites are now unconditional, based on your other comments.
if (auto* heuristics = window_->GetSoftNavigationHeuristics()) {Similar question, have the guarantees changed? This should never be called after detach.
Done
Related details
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
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")
void AddEventTiming(PerformanceEventTiming* entry);Michal MocnyCan we remove this? `last_event_timing_` doesn't matter anymore now that you're using the first one.
Done
The issue with the test failure was that we still used the context for tracing/reporting even after calling shutdown(), and I could fix it by swapping the order of calls.
By changing this to const, I no longer can reset it to nullptr during shutdown. Not sure if that means we rather have it non-const after all.
Related details
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 7 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
7 comments
LGTM other than the start time thing
if (!event.IsFullyTrusted()) {Maybe put a blurb in the CL description that this is changing? It's low risk, I think, but good to document the change that way, perhaps.
return initial_event_timing_->GetStartTime();Might be worth CHECKing that initial_event_timing_->GetStartTime() isn't 0?
The bot error is showing:
```
Check failed: !new_metrics.start_time.is_zero().
```
CHECK(initial_event_timing_);nit: You could probably (?) make `initial_event_timing_` const and/or remove this check, since it's already guaranteed to be valid in the c'tor and we don't change it.
Michal MocnyThis check actually failed on the tryjobs on this test yesterday: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/page_load_metrics/integration_tests/first_input_delay_browsertest.cc;l=18?q=MetricIntegrationTest.FirstInputDelay
It looks like the test simulated input then unloads the page.
I wonder if we are reporting SNC (to tracing / metric?) during teardown and no longer have the event timing entry?
Done
Oh totally missed that it was cleared in Shutdown(), and const didn't initially make sense. Could be good to be consistent and clear that too, but either way is fine.
SoftNavigationContext* GetRelevantContext(Can you add comments for this one? The other two are straightforward, but I don't know what this one does.
Maybe also something like `GetContextForNavigation()` since that's what it's used for?
SoftNavigationContext* GetSoftNavigationContextForInteractionId(I think all 3 of these methods can/should be private.
if (new_url == old_url) {Nice, I like consolidating all of the decisions here.
Related details
- Johannes Henkel
- 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 5 comments![]( "Open in Gerrit")
Maybe put a blurb in the CL description that this is changing? It's low risk, I think, but good to document the change that way, perhaps.
Done
Might be worth CHECKing that initial_event_timing_->GetStartTime() isn't 0?
The bot error is showing:
```
Check failed: !new_metrics.start_time.is_zero().
```
Added here and constructor, in case theres some issue with it changing between init and reporting/tracing (since we did have an issue with reporting right after destroy, earlier)
CHECK(initial_event_timing_);Michal Mocnynit: You could probably (?) make `initial_event_timing_` const and/or remove this check, since it's already guaranteed to be valid in the c'tor and we don't change it.
Michal MocnyThis check actually failed on the tryjobs on this test yesterday: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/page_load_metrics/integration_tests/first_input_delay_browsertest.cc;l=18?q=MetricIntegrationTest.FirstInputDelay
It looks like the test simulated input then unloads the page.
I wonder if we are reporting SNC (to tracing / metric?) during teardown and no longer have the event timing entry?
Scott HaseleyDone
Oh totally missed that it was cleared in Shutdown(), and const didn't initially make sense. Could be good to be consistent and clear that too, but either way is fine.
Done
Can you add comments for this one? The other two are straightforward, but I don't know what this one does.
Maybe also something like `GetContextForNavigation()` since that's what it's used for?
Done
SoftNavigationContext* GetSoftNavigationContextForInteractionId(I think all 3 of these methods can/should be private.
Done
Related details
- Johannes Henkel
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Johannes Henkel
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Johannes Henkel
Code-Coverage
Code-Owners
Code-Review
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 |
1 comment
if (HasDomModification() && HasUrl()) {I wonder if this check (processing end not being null) was making the unit tests pass before. Could you add TimeOrigin() condition to this to get around the failure?
Also, not totally sure, but without MockTime in unit tests, maybe processing end can also be null?
Related details
- Johannes Henkel
- 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 1 comment![]( "Open in Gerrit")
if (HasDomModification() && HasUrl()) {I wonder if this check (processing end not being null) was making the unit tests pass before. Could you add TimeOrigin() condition to this to get around the failure?
Also, not totally sure, but without MockTime in unit tests, maybe processing end can also be null?
Good idea. I can remove the previous fallback to processing_start in that case, and just check if this is null and leave it un-emitted if so.
Not ideal, but sufficient, for now.
Related details
- Johannes Henkel
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Johannes Henkel
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Johannes Henkel
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 with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
17 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/timing/soft_navigation_context.cc
Insertions: 11, Deletions: 12.
@@ -53,13 +53,6 @@
}
base::TimeTicks SoftNavigationContext::TimeOrigin() const {
- // TODO(crbug.com/490814752): The StartTime value seems to be missing in
- // some unittests. It should not be missing from any real events-- but we
- // provide a fallback to `processing_start` (which must be available).
- if (initial_event_timing_->GetStartTime().is_null()) {
- return initial_event_timing_->GetEventTimingReportingInfo()
- ->processing_start_time;
- }
return initial_event_timing_->GetStartTime();
}
@@ -134,12 +127,18 @@
}
bool SoftNavigationContext::SatisfiesSoftNavNonPaintCriteria() const {
- if (HasDomModification() && HasUrl()) {
- CHECK(!UrlChangeTime().is_null()); // Implied by HasUrl()
- CHECK(!TimeOrigin().is_null());
- return true;
+ // TODO(crbug.com/490814752): Event StartTime value seems to be missing in
+ // some unittests. It should not be missing from any real events.
+ if (TimeOrigin().is_null()) {
+ return false;
}
- return false;
+ // These start false, and become true as we observe effects.
+ if (!HasDomModification() || !HasUrl()) {
+ return false;
+ }
+ CHECK(!UrlChangeTime().is_null());
+ CHECK(!TimeOrigin().is_null());
+ return true;
}
bool SoftNavigationContext::SatisfiesSoftNavPaintCriteria(
```
```
The name of the file: third_party/blink/renderer/core/timing/soft_navigation_heuristics.cc
Insertions: 7, Deletions: 0.
@@ -537,6 +537,13 @@
CHECK_EQ(context->GetSoftNavigationHeuristics(), this);
if (LocalFrameClient* frame_client = frame->Client()) {
+ // TODO(crbug.com/490814752): Some tests simulate events with an impossibly
+ // small start_time value, which is less than the initial reference time,
+ // which makes the duration appear negative. We cannot report such values
+ // without failing expectations (in tests).
+ if (context->TimeOrigin() <= loader->GetTiming().ReferenceMonotonicTime()) {
+ return;
+ }
blink::SoftNavigationMetricsForReporting metrics = {
.soft_navigation_offset = context->SoftNavigationOffset(),
.start_time = loader->GetTiming().MonotonicTimeToPseudoWallTime(
```
Change information
[soft-navs] SoftNavigationContext created in terms of Interactions.
This patch changes SoftNavigationHeuristics from:
- doing its own interaction tracking via direct hooks into navigation and event dispatch
- creating (or reusing) new SoftNavigationContext objects
- establishing task attribution scopes and assigning the new context as task state variables
To instead now doing:
- leveraging the existing Event Timing InteractionId machinery for detecting interactions (i.e. ICP is now 1:1 with INP concept)
- leveraging the existing Event Timing RAII for task attribution scope lifetimes
This is a long awaited change towards converting SoftNavigationContext
into a more generalized InteractionContext. The immediate benefit is
exposing more interaction types to soft-navigatons, giving
SoftNavigation and InteractionContentfulPaint PerformanceEntry access to
event timing interactionId and a common shared startTime.
This also enabled a significant cleanup of now redundant code.
Small additional change in this patch:
- Changes the Event Timing check for `isTrusted` with `isFullyTrusted`
- M android_webview/test/data/web_tests/webexposed/global-interface-listing-expected.txt
- M third_party/blink/renderer/core/display_lock/display_lock_context_test.cc
- M third_party/blink/renderer/core/dom/events/event_dispatcher.cc
- M third_party/blink/renderer/core/frame/history.cc
- M third_party/blink/renderer/core/frame/local_frame_mojo_handler.cc
- M third_party/blink/renderer/core/loader/document_loader.cc
- M third_party/blink/renderer/core/loader/document_loader.h
- M third_party/blink/renderer/core/navigation_api/navigate_event.cc
- M third_party/blink/renderer/core/navigation_api/navigation_api.cc
- M third_party/blink/renderer/core/paint/pre_paint_tree_walk_test.cc
- M third_party/blink/renderer/core/timing/event_timing.cc
- M third_party/blink/renderer/core/timing/event_timing.h
- M third_party/blink/renderer/core/timing/interaction_contentful_paint.cc
- M third_party/blink/renderer/core/timing/interaction_contentful_paint.h
- M third_party/blink/renderer/core/timing/interaction_contentful_paint.idl
- M third_party/blink/renderer/core/timing/interaction_effects_monitor_test.cc
- M third_party/blink/renderer/core/timing/performance_event_timing.cc
- M third_party/blink/renderer/core/timing/performance_event_timing.h
- M third_party/blink/renderer/core/timing/performance_timeline_entry_id_generator.h
- M third_party/blink/renderer/core/timing/soft_navigation_context.cc
- M third_party/blink/renderer/core/timing/soft_navigation_context.h
- M third_party/blink/renderer/core/timing/soft_navigation_entry.cc
- M third_party/blink/renderer/core/timing/soft_navigation_entry.h
- M third_party/blink/renderer/core/timing/soft_navigation_entry.idl
- M third_party/blink/renderer/core/timing/soft_navigation_heuristics.cc
- M third_party/blink/renderer/core/timing/soft_navigation_heuristics.h
- M third_party/blink/renderer/core/timing/soft_navigation_heuristics_test.cc
- M third_party/blink/renderer/core/timing/soft_navigation_heuristics_test_util.cc
- M third_party/blink/renderer/core/timing/soft_navigation_heuristics_test_util.h
- M third_party/blink/renderer/core/timing/soft_navigation_paint_attribution_tracker_test.cc
- M third_party/blink/renderer/core/timing/window_performance.cc
- M third_party/blink/renderer/core/timing/window_performance.h
- M third_party/blink/web_tests/webexposed/global-interface-listing-expected.txt
Code-Review: +1 by Scott Haseley
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |