[event-timing]: Move interaction ID assignment to ResponsivenessMetrics [chromium/src : main]
Michal Mocny (Gerrit)
Michal Mocny voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
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. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 2 comments![]( "Open in Gerrit")
std::round((event_end_time - event_creation_time).InMillisecondsF() / 8) *Blink Style Guide: Pay close attention to function signatures. 'SetDuration' expects a 'double' (DOMHighResTimeStamp), and 'std::round' returns a 'double'. Please use 'double' instead of 'base::TimeDelta' for 'rounded_duration' to avoid type mismatch.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
first_pointer_down_event_timing_.Clear();Use 'nullptr' to clear the Member variable 'first_pointer_down_event_timing_' instead of '.Clear()'. (Blink Style Guide: C++ Features - Null Pointers)
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
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 2 comments![]( "Open in Gerrit")
std::round((event_end_time - event_creation_time).InMillisecondsF() / 8) *Blink Style Guide: Pay close attention to function signatures. 'SetDuration' expects a 'double' (DOMHighResTimeStamp), and 'std::round' returns a 'double'. Please use 'double' instead of 'base::TimeDelta' for 'rounded_duration' to avoid type mismatch.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
OK But Won't Fix: Pre-existing and unrelated to this patch.
first_pointer_down_event_timing_.Clear();Use 'nullptr' to clear the Member variable 'first_pointer_down_event_timing_' instead of '.Clear()'. (Blink Style Guide: C++ Features - Null Pointers)
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
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. |
Johannes Henkel (Gerrit)
Johannes Henkel voted and added 4 comments![]( "Open in Gerrit")
Votes added by Johannes Henkel
| Code-Review | +1 |
4 comments
// in PerformanceObservers and the Performance Timeline```suggestion
// in PerformanceObservers and the Performance Timeline.
```
// same logic, so we need to ignore the return value when InteractionId isThis part of the comment... are we actually ignoring some return value?
It seems in l. 377 the return value from SetPointerIdAndRecordLatency is used and it's the only way to return false from TryAssignInteractionId.
return true;I think the justification for "return true" comes from SetKeyIdAndRecord latency guaranteeing that it's going to call entry->SetInteractionIdInfo. Do you want to put some check statement that interaction id info is actually set?
// This won't have an interactionId if it is a mousedown, but, I don't
// know any platform that will emit mousedown without pointerdown, so thisThis comment is a little funny, as the reader may not know who 'I' is, or which platforms 'I' knows. :-)
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 4 comments![]( "Open in Gerrit")
// in PerformanceObservers and the Performance Timeline```suggestion
// in PerformanceObservers and the Performance Timeline.
```
Done
// same logic, so we need to ignore the return value when InteractionId isThis part of the comment... are we actually ignoring some return value?
It seems in l. 377 the return value from SetPointerIdAndRecordLatency is used and it's the only way to return false from TryAssignInteractionId.
This is some old comment that doesn't make sense to me either, will just remove it.
I think what it is trying to say is that this can return false :P
return true;I think the justification for "return true" comes from SetKeyIdAndRecord latency guaranteeing that it's going to call entry->SetInteractionIdInfo. Do you want to put some check statement that interaction id info is actually set?
I don't want to change the existing behaviour in this patch, esp since the next patch changes behaviour. We already have a CHECK for interactionID from the caller of this function, when we actually do the reporting to performance timeline.
// This won't have an interactionId if it is a mousedown, but, I don't
// know any platform that will emit mousedown without pointerdown, so thisThis comment is a little funny, as the reader may not know who 'I' is, or which platforms 'I' knows. :-)
:) fair!
We still have tests for it so I left it, and it will go away. I can probably just remove the comment as useless.
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 4 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
4 comments
LGTM. A few optional/future things; feel free to ignore all for this patch.
EventTimestamps event_timestamps);Also follow-up maybe: should this struct be copied every time it's passed around, or should we pass by const ref (here and above)?
Member<PerformanceEventTiming> event_timing_entry);I know this was here, but this should be a raw pointer, not a Member<>. Can we fix this as a follow-up?
// interactionID. The spec for Event Timing was updated this way.nit/optional: Consider linking to the spec section instead, although I'd assume this is going away?
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 4 comments![]( "Open in Gerrit")
ty all!
EventTimestamps event_timestamps);Also follow-up maybe: should this struct be copied every time it's passed around, or should we pass by const ref (here and above)?
Next patch deletes it!
Member<PerformanceEventTiming> event_timing_entry);I know this was here, but this should be a raw pointer, not a Member<>. Can we fix this as a follow-up?
Ah I think this still needs a fix, thanks.
// interactionID. The spec for Event Timing was updated this way.nit/optional: Consider linking to the spec section instead, although I'd assume this is going away?
I think this part, even after the new rework, should have a spec link.
In general, I will work on event-timing spec to update and then add links to the final impleemntation for all the parts.
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 voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
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]: Move interaction ID assignment to ResponsivenessMetrics
This refactoring separates interaction ID assignment and metrics
recording from the WindowPerformance class, moving the logic into
ResponsivenessMetrics to improve architectural separation and prepare
for subsequent performance timing updates.
Also cleans up the FirstInput mechanism which relies on making a copy of
the EventTiming entry. In this patch that still relies on the old school
FID state machine in order to keep existing semantics, but it takes us a
step closer to just being "report the first interaction event timing".
Summary of Changes:
- Migrated interaction ID logic from WindowPerformance to ResponsivenessMetrics.
- Clean up FirstInput reporting related logic.
- Renamed internal WindowPerformance methods for better clarity:
- ReportEventTimings -> TryFlushEventTimingQueue
- ReportEvent -> FlushEventTiming
- DispatchFirstInputTiming -> TryReportAsFirstInputTiming
- NotifyAndAddEventTimingBuffer -> ReportEventTimingToPerformanceTimeline
This is a non-functional refactoring to prepare for future feature work.
- 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. |