RPP: fix incorrect Interactions timeline [devtools/devtools-frontend : main]
0 views
Skip to first unread message
Jack Franklin (Gerrit)
Sep 22, 2025, 4:32:19 AM (2 days ago) Sep 22
to Finnur Thorarinsson, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org
Attention needed from Finnur Thorarinsson
Jack Franklin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jack Franklin
| Auto-Submit | +1 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Finnur Thorarinsson
Submit Requirements:
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Finnur Thorarinsson (Gerrit)
Sep 23, 2025, 11:42:40 AM (15 hours ago) Sep 23
to Jack Franklin, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org
Attention needed from Jack Franklin
Finnur Thorarinsson voted and added 8 comments![]( "Open in Gerrit")
Votes added by Finnur Thorarinsson
| Code-Review | +1 |
8 comments
Patchset-level comments
Finnur Thorarinsson . resolved
Just nits and observations...
File front_end/models/trace/handlers/UserInteractionsHandler.test.ts
Line 153, Patchset 2 (Latest):
// 1.pointerdown, 2. pointerup, 3. clickFinnur Thorarinsson . unresolved
nit: Missing period at the end. :)
Line 153, Patchset 2 (Latest):
// 1.pointerdown, 2. pointerup, 3. clickFinnur Thorarinsson . unresolved
nit: missing space after period.
Line 153, Patchset 2 (Latest):
// 1.pointerdown, 2. pointerup, 3. clickFinnur Thorarinsson . unresolved
Looks like the test expectations have changed, but the comment is documenting the old way, not the new way...
Line 156, Patchset 2 (Latest):
Finnur Thorarinsson . resolved
nit: Is this intentional? Don't care much either way, just thought I'd ask.
Line 177, Patchset 2 (Latest):
assert.strictEqual(keyboardInteraction.mainThreadHandling, 143901);Finnur Thorarinsson . resolved
Just an observation... this variable name (mainThreadHandling) is not a very descriptive one, for a duration... (beyond the scope for this CL though)
Line 193, Patchset 2 (Latest):
// end events were used which meant the durations of the interaction eventsFinnur Thorarinsson . unresolved
micro-not: comma before 'which'.
Related details
Attention is currently required from:
- Jack Franklin
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jack Franklin (Gerrit)
Sep 23, 2025, 12:35:29 PM (14 hours ago) Sep 23
to Finnur Thorarinsson, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org
Jack Franklin voted and added 6 comments![]( "Open in Gerrit")
Votes added by Jack Franklin
| Auto-Submit | +1 |
| Commit-Queue | +2 |
6 comments
File front_end/models/trace/handlers/UserInteractionsHandler.test.ts
nit: missing space after period.
Jack Franklin
Done
Looks like the test expectations have changed, but the comment is documenting the old way, not the new way...
Jack Franklin
Done
nit: Missing period at the end. :)
Jack Franklin
Done
Line 155, Patchset 2:
// 4. keydown, 5. keyupFinnur Thorarinsson . resolved
Jack Franklinditto. :)
Done
Line 177, Patchset 2:
assert.strictEqual(keyboardInteraction.mainThreadHandling, 143901);Finnur Thorarinsson . resolved
Just an observation... this variable name (mainThreadHandling) is not a very descriptive one, for a duration... (beyond the scope for this CL though)
Jack Franklin
Yeah, I would agree. I will send another CL to rename. I'd probably just lob `Duration` on the end. wdyt?
Line 193, Patchset 2:
// end events were used which meant the durations of the interaction eventsFinnur Thorarinsson . resolved
micro-not: comma before 'which'.
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devtools-frontend LUCI CQ (Gerrit)
Sep 23, 2025, 3:13:59 PM (11 hours ago) Sep 23
to Jack Franklin, Finnur Thorarinsson, devtools-rev...@chromium.org
Devtools-frontend LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: front_end/models/trace/handlers/UserInteractionsHandler.test.ts
Insertions: 3, Deletions: 3.
The diff is too large to show. Please review the diff.
```
Change information
Commit message:
RPP: fix incorrect Interactions timeline
This is the same fix as crrev.com/c/6969008 but applied to the
UserInteractionsHandler, which does not use our generic event pairing
implementation (it should, but it works slightly differently with how we
construct the events, so I will tackle that in a separate CL).
Perfetto changed to reuse IDs for non-overlapping event pairs, and the
UserInteractionsHandler did not account for this. This CL ensures that
it does, meaning that we no longer pair events inaccurately in the
timeline.
Fixed: 446590230
Change-Id: I99571cbf131ab2b694eb0a7883f4f83a06fd9cb3
Commit-Queue: Jack Franklin <jacktf...@chromium.org>
Auto-Submit: Jack Franklin <jacktf...@chromium.org>
Reviewed-by: Finnur Thorarinsson <fin...@chromium.org>
Files:
- M front_end/models/trace/handlers/UserInteractionsHandler.test.ts
- M front_end/models/trace/handlers/UserInteractionsHandler.ts
- M front_end/panels/timeline/fixtures/traces/BUILD.gn
- M front_end/panels/timeline/fixtures/traces/README.md
- A front_end/panels/timeline/fixtures/traces/interaction-events-with-shared-ids.json.gz
Change size: M
Delta: 5 files changed, 142 insertions(+), 74 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Finnur Thorarinsson
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages