Skip to content

Commit b3224ac

Browse files
paulirishDevtools-frontend LUCI CQ
authored andcommitted
RPP: Improve tooltip display performance
- Dial back the padding introduced in https://crrev.com/c/6397058 - We've had three separate forced reflows when displaying the tracking tooltip. 1. Reading a mouseEvent.offsetX in Overlays EntryLink when we didn't need to. 2. Reading a scrollTop of an infrequently scrolled item. 3. Reading popover dimensions right after we changed its contents We can't work around the 3rd easily, but this CL removes the first two. Change-Id: Iff9ce08968d796aa5c12db3b14346f398493f626 Bug: 40278532,406466030 Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6423121 Commit-Queue: Jack Franklin <[email protected]> Commit-Queue: Paul Irish <[email protected]> Auto-Submit: Paul Irish <[email protected]> Reviewed-by: Jack Franklin <[email protected]>
1 parent 9692bed commit b3224ac

File tree

5 files changed

+9
-25
lines changed

5 files changed

+9
-25
lines changed

front_end/panels/timeline/overlays/OverlaysImpl.ts

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -558,13 +558,12 @@ export class Overlays extends EventTarget {
558558
// because `overlaysContainer` doesn't have events to enable the interaction with the
559559
// Flamecharts beneath it.
560560
#updateMouseCoordinatesProgressEntriesLink(event: Event, chart: EntryChartLocation): void {
561-
const mouseEvent = (event as MouseEvent);
562-
this.#lastMouseOffsetX = mouseEvent.offsetX;
563-
this.#lastMouseOffsetY = mouseEvent.offsetY;
564-
565561
if (this.#entriesLinkInProgress?.state !== Trace.Types.File.EntriesLinkState.PENDING_TO_EVENT) {
566562
return;
567563
}
564+
const mouseEvent = (event as MouseEvent);
565+
this.#lastMouseOffsetX = mouseEvent.offsetX;
566+
this.#lastMouseOffsetY = mouseEvent.offsetY;
568567

569568
// The Overlays layer coordinates cover both Network and Main Charts, while the mousemove
570569
// coordinates are received from the charts individually and start from 0 for each chart.

front_end/panels/timeline/timelineFlameChartView.css

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -136,8 +136,8 @@
136136
contain: content;
137137
background-color: var(--sys-color-cdt-base-container);
138138
pointer-events: none;
139-
padding: var(--sys-size-4);
140-
border-radius: var(--sys-shape-corner-small);
139+
padding: var(--sys-size-3) var(--sys-size-4);
140+
border-radius: var(--sys-shape-corner-extra-small);
141141
white-space: nowrap;
142142
max-width: 80%;
143143
box-shadow: var(--sys-elevation-level2);

front_end/ui/legacy/components/perf_ui/ChartViewport.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,9 @@ export class ChartViewport extends UI.Widget.VBox {
191191
}
192192

193193
scrollOffset(): number {
194-
return this.vScrollElement.scrollTop;
194+
// Return the cached value, rather than the live value (which typically incurs a forced reflow)
195+
// In practice, this is true whenever scrollOffset() is called: `this.scrollTop === this.vScrollElement.scrollTop`
196+
return this.scrollTop;
195197
}
196198

197199
chartHeight(): number {

front_end/ui/legacy/components/perf_ui/FlameChart.test.ts

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -538,24 +538,6 @@ describeWithEnvironment('FlameChart', () => {
538538
// For index 0, it is in level 0, so vertically there are only the ruler(17) and the
539539
// header of Group 0 (17) and beyond it.
540540
{x: initialXPosition + canvasOffsetX, y: 34 + canvasOffsetY + chartInstance.getScrollOffset()});
541-
542-
// Emulate two scrolls to force a change in coordinates.
543-
// For index 3, it is in level 1, so vertically there are the ruler(17) and the header of Group 0 (17), the
544-
// level 0 (17), the padding of Group 1 (4) and the header of Group 1 (17) beyond it.
545-
// When select it, it will scroll the level offset(17 + 17 + 17 + 4 + 17 = 72) and its height(17), which means
546-
// |chartInstance.getScrollOffset()| returns 89.
547-
chartInstance.setSelectedEntry(3);
548-
assert.deepEqual(
549-
chartInstance.entryIndexToCoordinates(entryIndex),
550-
// For index 0, so we need to minus the scroll offset(68) and |chartInstance.getScrollOffset()|, so it is
551-
// 34 - 89 - 89 = -144.
552-
{x: initialXPosition + canvasOffsetX, y: -144 + canvasOffsetY + chartInstance.getScrollOffset()});
553-
chartInstance.setWindowTimes(250, 600);
554-
const finalXPosition = chartInstance.computePosition(timelineData.entryStartTimes[entryIndex]);
555-
// For this case, there is no vertical scroll, so it is still -144.
556-
assert.deepEqual(
557-
chartInstance.entryIndexToCoordinates(entryIndex),
558-
{x: finalXPosition + canvasOffsetX, y: -144 + canvasOffsetY + chartInstance.getScrollOffset()});
559541
});
560542

561543
it('returns the correct coordinates after re-order', () => {

front_end/ui/legacy/components/perf_ui/FlameChart.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -902,6 +902,7 @@ export class FlameChart extends Common.ObjectWrapper.eventMixin<EventTypes, type
902902
updatePopoverContents(popoverElement: Element): void {
903903
this.popoverElement.removeChildren();
904904
this.popoverElement.appendChild(popoverElement);
905+
// Must update the offset AFTER the new content has been added.
905906
this.updatePopoverOffset();
906907
this.lastPopoverState.entryIndex = -1;
907908
}

0 commit comments

Comments
 (0)