Fix "scroll to hotspot" functionality in the source view + assembly view #5759
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5759 +/- ##
==========================================
- Coverage 85.70% 85.69% -0.01%
==========================================
Files 315 316 +1
Lines 31130 31141 +11
Branches 8567 8582 +15
==========================================
+ Hits 26679 26686 +7
- Misses 4021 4025 +4
Partials 430 430 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mstange
force-pushed
the
bottom-box-scrolling-improvements
branch
from
61d7bb8 to
3fdb075
Compare
src/types/profile-derived.ts
Outdated
| scrollToLineNumber?: number; | ||
| highlightLineNumber?: number; |
There was a problem hiding this comment.
Hmm I can't remember why we had highlighted line on top of scroll to line number.
Edit: Ah, I think it was because of the marker stacks, right? Can we add some comment here and state.ts to explain their usages. It was a bit confusing to me at first.
But also, what happens if we have both scrollToLineNumber and highlightLineNumber, how does it behave. For example, does first highlighting and then scrolling resets the highlight?
There was a problem hiding this comment.
That's right, Florian added it when clicking frames in marker stacks. With this patch, highlighting and scrolling are fully independent; and passing highlightLineNumber: undefined will unset the current highlight. So it's different from scrolling where the current scrolling isn't changed if the property isn't specified.
This mismatch suggests that, instead of highlightLineNumber?: number, it might be clearer to have highlightedLineNumber: number | null.
src/profile-logic/profile-data.ts
Outdated
| nativeSymbols: nativeSymbolInfos, | ||
| lineNumber, | ||
| scrollToLineNumber: lineNumber, | ||
| highlightLineNumber: lineNumber, |
There was a problem hiding this comment.
Can they be different, or are they always the same value?
There was a problem hiding this comment.
At the moment they're always the same, but the API allows them to be different. Alternatively we could have used a scrollTargetLineIsHighlighted: boolean property.
I want to add a call to a function from line-timings.ts but line-timings.ts already depends on profile-data.ts, and I can't create a circular dependency between profile-data.ts and line-timings.ts.
This means we can compute the right line number for the "hot spot" for the call node that was passed to getBottomBoxInfoForCallNode, and not just for the selected call node. For example, getBottomBoxInfoForCallNode can be called with the right-clicked call node which can be different from the selected call node, and we were scrolling to the wrong spot in that case before this patch. Fixes firefox-devtools#5758.
|
Thanks for the review! I'll adjust the comments to answer the questions you had. |
mstange
force-pushed
the
bottom-box-scrolling-improvements
branch
from
3fdb075 to
7b354c1
Compare
mstange
force-pushed
the
bottom-box-scrolling-improvements
branch
from
7b354c1 to
98b5328
Compare
2 participants
Production | Deploy preview
Fixes #5758 and cleans things up a bit.
Having the samples passed to
getBottomBoxInfoForCallNodewill also make it easier to address the TODO about picking the native symbol with the highest sample count, but I'll do that in a separate PR (or probably as part of #5349).