[event-timing] Assign fallback for events affected by context menu. [chromium/src : main]
Michal Mocny (Gerrit)
Message from Michal Mocny![]( "Open in Gerrit")
Set Ready For Review
Related details
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")
Next in line. This ended up being a useful fix, which I didn't expect when I started. This is one part of the series of patches to reduce complexity and timers in the responsiveness metrics, which unblocks improving the interaction id machinery.
One small part at a time!
(I will fix the whitespace cl formatting warnings-- some sort of git cl format issues that are getting through the presubmit checks)
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 3 comments![]( "Open in Gerrit")
EXPECT_FALSE(pointerdown_entry->HasKnownEndTime());I could add also expectation on interactionID
RegisterPointerEvent(event_type_names::kContextmenu, contextmenu_timestamp,...and here we can add expectations for the contextmenu event
processing_end_pointerdown);Could extend the test to also add pointerup after contextmenu and ensure it gets the same interactionid..
Might want to move from WindowPerformanceTest to InteractionIdTest...
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 9 comments![]( "Open in Gerrit")
I think this looks good. Mostly just questions and minor stuff. I'll try this locally tomorrow too.
This CL introduces an explicit fallback mechanism right at the event
dispatch to resolve the performance end time for preceding events when a
'contextmenu' event is dispatched.What's the interop story here?
void NotifyPointerdown(PointerEntryAndInfo* pointer_info) const;If you're making other changes, along the lines of the previous comment, this really could use some comments (or be renamed). I'd assume this gets called as soon as a PointerDown is detected (probably to start book-keeping), but that's not what it does.
bool was_notified_ = false;I know this is tied to the "NotifyPointerDown" method, but it's not clear from context in this class what this means. 2 suggestions:
1. Consider renaming to something like `was_entry_emitted_` or `was_flushed_to_peformance_timeline_` or something
2. Add some comments to what this means (maybe to the public methods), especially if not renaming?
if (pending_pointer_down) {
CHECK(pending_pointer_down->GetEntry()->name() ==
event_type_names::kPointerdown);
}Not related to this change, but if you're making any other changes, I think this would be more readable and idiomatic as:
```suggestion
CHECK(!pending_pointer_down || pending_pointer_down->GetEntry()->name() ==
event_type_names::kPointerdown);
```
// A pointerdown followed by contextmenu is assigned an interactionId right
// away. We leave the pointerdown in the map of pointer interactions soThis is probably because I don't know this code, but why? Is this so it can count as an interaction right away, and that we set the contextmenu entry's value to 0 so don't record metrics for it? Just want to confirm my understanding, but it could be worth explaining why in the comment too.
(Aside/Q: do we end up emitting the contextmenu `entry` to the performance timeline with the 0 id?)
// Try handle keyboard event simulated click.This whole block (down to l. 347) is just formatting fixes, right? I didn't spot any functional changes, but want to check.
if (event.type() != event_type_names::kContextmenu) {optional/suggestion: it might be clearer at the call site if this was a CHECK here and the event type check happened at the call site.
Alternatively, it would be neat if this and `ReportEventTimingsWithoutNextPaint()` could share code, taking the FallbackReason as a parameter, since they're doing something similar. But the bodies are probably too different?
!processing_end.is_null() ? processing_end : base::TimeTicks::Now();Should this use a consistent end time here instead of recomputing Now() in each iteration?
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 7 comments![]( "Open in Gerrit")
Will update patch-- thanks for feedback-- wanted to send some of the more interesting answers first.
This CL introduces an explicit fallback mechanism right at the event
dispatch to resolve the performance end time for preceding events when a
'contextmenu' event is dispatched.What's the interop story here?
WIP: https://github.com/w3c/event-timing/issues/154
I think we have appetite to spec this, but I would like to start with speccing the interaction at "processingStart" and the PaintTimingMixin integration first.
Then fallbacks become a bit easier to reason about.
void NotifyPointerdown(PointerEntryAndInfo* pointer_info) const;If you're making other changes, along the lines of the previous comment, this really could use some comments (or be renamed). I'd assume this gets called as soon as a PointerDown is detected (probably to start book-keeping), but that's not what it does.
Will do, even just to help reason about in the next patches.
bool was_notified_ = false;I know this is tied to the "NotifyPointerDown" method, but it's not clear from context in this class what this means. 2 suggestions:
1. Consider renaming to something like `was_entry_emitted_` or `was_flushed_to_peformance_timeline_` or something
2. Add some comments to what this means (maybe to the public methods), especially if not renaming?
Good suggestions-- but also FYI this whole class goes away in 2-3 patches.
That said, pointerdown buffering is the one thing that persists and I may want a similar solution. (My original patch didn't need it, but, it also didn't solve this specific CL as thoroughly as I did now)
// A pointerdown followed by contextmenu is assigned an interactionId right
// away. We leave the pointerdown in the map of pointer interactions soThis is probably because I don't know this code, but why? Is this so it can count as an interaction right away, and that we set the contextmenu entry's value to 0 so don't record metrics for it? Just want to confirm my understanding, but it could be worth explaining why in the comment too.
(Aside/Q: do we end up emitting the contextmenu `entry` to the performance timeline with the 0 id?)
pointerdown are only meant to be "pending" until we know if they scroll, because this is a type of "next paint" that we cannot measure in the typical main thread scheduling way. (even then, I argue we could still measure the latency that pointerdown introduces to scroll start, or something-- but this is a big breaking change).
A contextmenu is basically equivalent to pointerup, and comes before pointerup (or instead of on some platforms or conditions-- I guess it depends how fast the mouse moves over the contextmenu before lifting up).
The contextmenu event itself *is* reported to perf timeline, but is specced NOT to get an interactionid, for 2 reasons:
1. It already overlaps in duration with the pointer events (pointerdown) since this event is part of the same sync event dispatch. So adding this interactionId to it is not helping find new long INP durations.
2. For attribution it would be maximally useful to expose *all* related events rather than just a few-- but this is difficult to do just from the "stream of event types". We've discussed basically doing something like "if the events are part of the same dispatch scope" or something. The Pointer Events spec authors are working on a generalized "gesture" concept that might help.
Basically: contextmenu is the same as beforeinput, input, change, keypress, and a plethora of other useful syncthetic events, some of which are more commonly handled than their lower-level raw pointer/key events-- but they all overlap in time anyway. Getting interactionId attribution just helps with targetSelector (maybe?) and correct processingTime attribution. But this hasn't been a huge source of feedback surprisingly.
For now-- I don't want to change existing behaviour here.
Will expand comments.
// Try handle keyboard event simulated click.This whole block (down to l. 347) is just formatting fixes, right? I didn't spot any functional changes, but want to check.
Yeah I fixed a bunch of it, but it keeps sneaking back, theres something weird with `git cl format` going on.
if (event.type() != event_type_names::kContextmenu) {optional/suggestion: it might be clearer at the call site if this was a CHECK here and the event type check happened at the call site.
Alternatively, it would be neat if this and `ReportEventTimingsWithoutNextPaint()` could share code, taking the FallbackReason as a parameter, since they're doing something similar. But the bodies are probably too different?
The *next* patch tries to consolodate a bunch of the fallback reasons, but I hadn't considered the ReportEventTimingsWithoutNextPaint() be considered part of that. Will consider it.
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 11 comments![]( "Open in Gerrit")
Expanded the test and fixed a couple more subtle issues. Will do more manual testing and audit the WPT (we have a few contextmenu tests)
void NotifyPointerdown(PointerEntryAndInfo* pointer_info) const;Michal MocnyIf you're making other changes, along the lines of the previous comment, this really could use some comments (or be renamed). I'd assume this gets called as soon as a PointerDown is detected (probably to start book-keeping), but that's not what it does.
Will do, even just to help reason about in the next patches.
Done
Michal MocnyI know this is tied to the "NotifyPointerDown" method, but it's not clear from context in this class what this means. 2 suggestions:
1. Consider renaming to something like `was_entry_emitted_` or `was_flushed_to_peformance_timeline_` or something
2. Add some comments to what this means (maybe to the public methods), especially if not renaming?
Good suggestions-- but also FYI this whole class goes away in 2-3 patches.
That said, pointerdown buffering is the one thing that persists and I may want a similar solution. (My original patch didn't need it, but, it also didn't solve this specific CL as thoroughly as I did now)
Done
if (pending_pointer_down) {
CHECK(pending_pointer_down->GetEntry()->name() ==
event_type_names::kPointerdown);
}Not related to this change, but if you're making any other changes, I think this would be more readable and idiomatic as:
```suggestion
CHECK(!pending_pointer_down || pending_pointer_down->GetEntry()->name() ==
event_type_names::kPointerdown);
```
Done
// A pointerdown followed by contextmenu is assigned an interactionId right
// away. We leave the pointerdown in the map of pointer interactions soMichal MocnyThis is probably because I don't know this code, but why? Is this so it can count as an interaction right away, and that we set the contextmenu entry's value to 0 so don't record metrics for it? Just want to confirm my understanding, but it could be worth explaining why in the comment too.
(Aside/Q: do we end up emitting the contextmenu `entry` to the performance timeline with the 0 id?)
pointerdown are only meant to be "pending" until we know if they scroll, because this is a type of "next paint" that we cannot measure in the typical main thread scheduling way. (even then, I argue we could still measure the latency that pointerdown introduces to scroll start, or something-- but this is a big breaking change).
A contextmenu is basically equivalent to pointerup, and comes before pointerup (or instead of on some platforms or conditions-- I guess it depends how fast the mouse moves over the contextmenu before lifting up).
The contextmenu event itself *is* reported to perf timeline, but is specced NOT to get an interactionid, for 2 reasons:
1. It already overlaps in duration with the pointer events (pointerdown) since this event is part of the same sync event dispatch. So adding this interactionId to it is not helping find new long INP durations.
2. For attribution it would be maximally useful to expose *all* related events rather than just a few-- but this is difficult to do just from the "stream of event types". We've discussed basically doing something like "if the events are part of the same dispatch scope" or something. The Pointer Events spec authors are working on a generalized "gesture" concept that might help.
Basically: contextmenu is the same as beforeinput, input, change, keypress, and a plethora of other useful syncthetic events, some of which are more commonly handled than their lower-level raw pointer/key events-- but they all overlap in time anyway. Getting interactionId attribution just helps with targetSelector (maybe?) and correct processingTime attribution. But this hasn't been a huge source of feedback surprisingly.
For now-- I don't want to change existing behaviour here.
Will expand comments.
I've decided not to document this here. This is just too much a part of the design and we can update docs / specs as needed to explain, and/or raise new directions.
But this code is about to churn, anyway.
Michal MocnyThis whole block (down to l. 347) is just formatting fixes, right? I didn't spot any functional changes, but want to check.
Yeah I fixed a bunch of it, but it keeps sneaking back, theres something weird with `git cl format` going on.
I just confirmed with a better diff tool that this is only renaming pending_pointer_down, removing the unneeded (and unhelpful) previous_entry, and changing the accidental +1 nesting of all lines.
if (event.type() != event_type_names::kContextmenu) {Michal Mocnyoptional/suggestion: it might be clearer at the call site if this was a CHECK here and the event type check happened at the call site.
Alternatively, it would be neat if this and `ReportEventTimingsWithoutNextPaint()` could share code, taking the FallbackReason as a parameter, since they're doing something similar. But the bodies are probably too different?
The *next* patch tries to consolodate a bunch of the fallback reasons, but I hadn't considered the ReportEventTimingsWithoutNextPaint() be considered part of that. Will consider it.
Moved the Event check out-- because I actually had a bug because I confused myself because of the Event getting passed in.
The bug was that we called the fallback method *before this event entry* was added to the list of pending, so the entry for the contextmenu wasn't getting the fallback, even though its event was the one passed into here.
I think the pending events and the contextmenu event are both not worth measuring to next paint (for the same reasons).
I think my updated test file catches this.
!processing_end.is_null() ? processing_end : base::TimeTicks::Now();Should this use a consistent end time here instead of recomputing Now() in each iteration?
Good call!
I could add also expectation on interactionID
Done
RegisterPointerEvent(event_type_names::kContextmenu, contextmenu_timestamp,...and here we can add expectations for the contextmenu event
Done
Could extend the test to also add pointerup after contextmenu and ensure it gets the same interactionid..
Might want to move from WindowPerformanceTest to InteractionIdTest...
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 voted Commit-Queue+0![]( "Open in Gerrit")
| Commit-Queue | +0 |
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. |
Johannes Henkel (Gerrit)
Johannes Henkel voted and added 8 comments![]( "Open in Gerrit")
Votes added by Johannes Henkel
| Code-Review | +1 |
8 comments
As you know I'm not that familiar with this code, so my comments may be a little naive. Please enjoy or ignore as you see fit. :-)
FlushPointerdownAndNotifyObservers(pending_pointer_down);I think ideally, the naming of this method and the WasEntryEmitted() thing would be lined up, and since it's potentially not actually "flushing", being verbose and putting the check into the caller may make it easier to understand. E.g. this:
if (!pending_pointer_down->WasEntryEmitted()) {
EmitPointerDownToPerformanceTimeline(pending_pointer_down);
}(EmitPointerDownToPerformanceTimeline could have CHECK(!pending_pointer_down->WasEntryEmitted()) at its beginning.
PS: I see you discussed naming, and probably have more thoughts and context, so please feel free to skip this as you see fit.
entry->GetEventTimingReportingInfo()->pointer_id.value());I think this can also be |pointer_id|.
Potential yak shaving opportunity: I'm seeing that we query the pointer_id_entry_map_ three times, with Contains, at, and erase. If we were to use .Find instead, I think a single lookup could cover it since .Find returns an iterator (which can also be passed to erase).
if (!pending_pointer_down->GetEntry()->HasKnownInteractionID()) {Idle thought: This makes me wonder whether the conditions "has been emitted to perf timeline" and "HasKnownInteractionID" are independent, equivalent, or one implies the other.
if (!pending_pointer_down->GetEntry()->HasKnownInteractionID()) {I think inverting the condition (starting with the "known interaction id" branch) may make it slightly easier to read?
pending_pointer_down = nullptr;This assignment feels redundant as pending_pointer_down isn't read afterwards.
// A pointerdown may be "flushed" to performance timeline when any number ofHmm - It feels elsewhere, "flushed" means it's cleared from the map and/or emitted to UKM? Specifically, FlushAllPointerdownWithMeasuredPointerup, doesn't seem it emits anything to performance timeline.
TEST_P(InteractionIdTest, ContextMenu) {Sweet test! Above, I'm seeing that the tests end with some ukm checking - would this be relevant here as well?
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. |
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
In testing I found that one of the contextmenu WPT was timing out, because the event timing was not meeting the minimum duration threshold any more for reporting.
I could have just updated the test, but I realized that this was symbolic of a less web interoperable behaviour than previously. I solved this in a neat way:
1. I moved the context menu fallback to the ProcessingEnd for the context menu, and
2. I use the known processing end time of that event to apply to all events
The reason for (2) is that in testing I found that the contextmenu doesn't show until the event run the default action. So, the contextmenu end time == the visual feedback time, similar to js prompts. This is the perfect fallback time, that still measures attributable duration.
It also happens to be a lot closer to previous behaviour.
(A side benefit is somewhat cleaner fallback assignment, which will help in next patches)
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 Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
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 7 comments![]( "Open in Gerrit")
FlushPointerdownAndNotifyObservers(pending_pointer_down);I think ideally, the naming of this method and the WasEntryEmitted() thing would be lined up, and since it's potentially not actually "flushing", being verbose and putting the check into the caller may make it easier to understand. E.g. this:
if (!pending_pointer_down->WasEntryEmitted()) {
EmitPointerDownToPerformanceTimeline(pending_pointer_down);
}(EmitPointerDownToPerformanceTimeline could have CHECK(!pending_pointer_down->WasEntryEmitted()) at its beginning.
PS: I see you discussed naming, and probably have more thoughts and context, so please feel free to skip this as you see fit.
I think your suggestion is better, but I will resolve here to reduce churn.
Not to ignore your feedback, but because this patch and a few others are small intermediate patches on the way to *removing all this code*. Let's have a long discussion on style/naming in *that* patch!
entry->GetEventTimingReportingInfo()->pointer_id.value());I think this can also be |pointer_id|.
Potential yak shaving opportunity: I'm seeing that we query the pointer_id_entry_map_ three times, with Contains, at, and erase. If we were to use .Find instead, I think a single lookup could cover it since .Find returns an iterator (which can also be passed to erase).
Acknowledged
if (!pending_pointer_down->GetEntry()->HasKnownInteractionID()) {Idle thought: This makes me wonder whether the conditions "has been emitted to perf timeline" and "HasKnownInteractionID" are independent, equivalent, or one implies the other.
HasKnownInteractionID is meant to handle the case of pointerdown that is still in the "pending" state. Until we see future events that complete the decision making, it is unknown (`nullopt`).
In the current state of the world that this patch is still in, ALL events start with unknown interactionID and have to finish measurement all the way to presentaton time before we even attempt to assign InteractionID to them. And more events than pointerdown will temporarily stay unknown as we wait for buffers and timers...
Eventually the id becomes known, may or may not be 0, and this finally allows us to emit to perf timeline.
So HasKnownInteractionID is necessary but not sufficient to know that we emitted to the perforamnce timeline.
That is the current state of things.
---
The direction I'm trying to move us (the goal of these patches):
- Assign InteractionID immediately at ProcessingStart.
- Remove all complex timers etc for all use cases, so you can always have all the context you need right away
- Pointerdown continued to have a pending state (the one and only outlier now), but it will be handled differently
Future possible direction:
- I think we could evaluate making pointerdown always an interaction, and just ignore its Paint whenever it is handled by a default action (scroll) faster than it can paint. I think treating `pointercancel` as a type of Fallback signal for its paint might be enough -- but we have to finish this work and spec fallback times first.
if (!pending_pointer_down->GetEntry()->HasKnownInteractionID()) {I think inverting the condition (starting with the "known interaction id" branch) may make it slightly easier to read?
Going away soon, and want to keep this less churn here.
This assignment feels redundant as pending_pointer_down isn't read afterwards.
Done
// A pointerdown may be "flushed" to performance timeline when any number ofHmm - It feels elsewhere, "flushed" means it's cleared from the map and/or emitted to UKM? Specifically, FlushAllPointerdownWithMeasuredPointerup, doesn't seem it emits anything to performance timeline.
Agree, but this is all going away and I'm keeping pre-existing as much as possible for this patch.
Sweet test! Above, I'm seeing that the tests end with some ukm checking - would this be relevant here as well?
Good idea, added. Its actually a bit tricky because we could emit after contextmenu but right now we must see pointerup in order to do UKM reporting. SO the contextmenu without pointerup bug would not report to UKM even after this fix here, but it *will* get fixed in the next patches that force all UKM reporting eagerly.
I'll update expectations and add a test for pointerdown + contextmenu only, but in that next patch.
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 voted and added 3 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
3 comments
void FlushPointerdownAndNotifyObservers(This is a lot clearer, thanks!
FlushKeydown();Does this need to be handled?
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michal Mocny
| Commit-Queue | +2 |
1 comment
FlushKeydown();Does this need to be handled?
The keydown event itself used to not be assigned an interactionId at keydown, now it is.
However, we don't send the reported value to UKM until we know we are ready to flush, and this will now not happen after timer, and instead wait for arbitrarily long.
This means that there is a minor regression, only for UKM reporting data-loss in some cases, only for contextmenu key.
I think this is very acceptable as a temporary measure since we are going to flush to ukm immediately, soon.
Related details
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 1 comment![]( "Open in Gerrit")
FlushKeydown();Michal MocnyDoes this need to be handled?
The keydown event itself used to not be assigned an interactionId at keydown, now it is.
However, we don't send the reported value to UKM until we know we are ready to flush, and this will now not happen after timer, and instead wait for arbitrarily long.
This means that there is a minor regression, only for UKM reporting data-loss in some cases, only for contextmenu key.
I think this is very acceptable as a temporary measure since we are going to flush to ukm immediately, soon.
Cool, thanks for the explanation!
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[event-timing] Assign fallback for events affected by context menu.
A context menu interruption can prevent the subsequent 'Next Paint' from
occurring (or being reported) for event timings, especially on platforms
like Mac. This makes it fundamentally difficult/ undeterministic to
measure these types of interactions.
But also, a contextmenu provides real visual feedback to the user that
is an alternative to next paint-- very much like a js alert() prompt,
which we already treat as a fallback value.
There has been a long list of known crbugs related to contextmenu and
artificially large durations, which we have tried to fix with
complicated patching and reliance on timers and "eventual" flushing. To
do this we relied on a `contextmenu_flush_timer_` timer in
ResponsivenessMetrics, and had the interactions state machine call back
in to this measurement code to force flush these event timings. This is
convoluted and brittle (and broken in subtle ways). More importantly, we
are working towards removing the need for these timers and buffering so
we need to fix this here.
I tested locally, and just spamming right click on a simple test page
would consistently lead to various reporting issues (assigning the same
interaction id to multiple pointerdown, missing reports, etc). So our
existing patches are not perfect, anyway.
This CL introduces an explicit fallback mechanism right at the event
dispatch to resolve the performance end time for preceding events when a
'contextmenu' event is dispatched.
After this fix, all tests continue to pass, but manual testing
immediately showcases better behaviour. I haven't found a way to create
a test to reliably reproduce the race conditions.
Another side effect is that we flush these event timings more
quickly/reliably to the performance timeline.
- 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/responsiveness_metrics.cc
- M third_party/blink/renderer/core/timing/responsiveness_metrics.h
- M third_party/blink/renderer/core/timing/window_performance.cc
- M third_party/blink/renderer/core/timing/window_performance.h
- M third_party/blink/renderer/core/timing/window_performance_test.cc
Code-Review: +1 by Scott Haseley, +1 by Johannes Henkel
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |