Skip to content

Commit 1d90c97

Browse files
jackfranklinDevtools-frontend LUCI CQ
authored andcommitted
RPP: rewrite TimelineSelection class
The `TimelineSelection` class has become confusing to work with since we completed the data migration to the new trace engine. It differentiated selections of types that used to be different concepts in the codebase: frames, trace events and network events. Now, these are all defined as `Trace.Types.Events.Event`, so we don't need each to have its own "selection type". We have all the methods in `Trace.Types.Events` that let you determine the type of event, so rather than duplicate some of those in `TimelineSelection`, we now categorise a selection as one of two things: 1. an event 2. a time range Once you determine your selection is an event, you can use all the same helpers in `Trace.Types.Events` to determine the exact type of event, if you need to. Fixed: 370716587 Change-Id: I256a6841af280a1eff4cecacc39a9f351b3ebc11 Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/5975681 Commit-Queue: Jack Franklin <[email protected]> Reviewed-by: Andres Olivares <[email protected]>
1 parent 33b72e2 commit 1d90c97

14 files changed

+278
-216
lines changed

front_end/models/trace/helpers/Timing.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,14 @@ export function traceWindowMillisecondsToMicroSeconds(bounds: Types.Timing.Trace
118118
range: millisecondsToMicroseconds(bounds.range),
119119
};
120120
}
121+
export function traceWindowMicroSecondsToMilliSeconds(bounds: Types.Timing.TraceWindowMicroSeconds):
122+
Types.Timing.TraceWindowMilliSeconds {
123+
return {
124+
min: microSecondsToMilliseconds(bounds.min),
125+
max: microSecondsToMilliseconds(bounds.max),
126+
range: microSecondsToMilliseconds(bounds.range),
127+
};
128+
}
121129

122130
export function traceWindowFromMilliSeconds(
123131
min: Types.Timing.MilliSeconds, max: Types.Timing.MilliSeconds): Types.Timing.TraceWindowMicroSeconds {
@@ -186,3 +194,8 @@ export interface WindowFitsInsideBounds {
186194
export function windowFitsInsideBounds(data: WindowFitsInsideBounds): boolean {
187195
return data.window.min >= data.bounds.min && data.window.max <= data.bounds.max;
188196
}
197+
198+
export function windowsEqual(
199+
w1: Types.Timing.TraceWindowMicroSeconds, w2: Types.Timing.TraceWindowMicroSeconds): boolean {
200+
return w1.min === w2.min && w1.max === w2.max;
201+
}

front_end/panels/timeline/EventsTimelineTreeView.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import * as VisualLogging from '../../ui/visual_logging/visual_logging.js';
1212

1313
import {Category, IsLong} from './TimelineFilters.js';
1414
import type {TimelineModeViewDelegate} from './TimelinePanel.js';
15-
import {TimelineSelection} from './TimelineSelection.js';
15+
import {selectionIsEvent, type TimelineSelection} from './TimelineSelection.js';
1616
import {TimelineTreeView} from './TimelineTreeView.js';
1717
import {TimelineUIUtils} from './TimelineUIUtils.js';
1818
import * as Utils from './utils/utils.js';
@@ -59,8 +59,8 @@ export class EventsTimelineTreeView extends TimelineTreeView {
5959

6060
override updateContents(selection: TimelineSelection): void {
6161
super.updateContents(selection);
62-
if (TimelineSelection.isTraceEventSelection(selection.object)) {
63-
this.selectEvent(selection.object, true);
62+
if (selectionIsEvent(selection)) {
63+
this.selectEvent(selection.event, true);
6464
}
6565
}
6666

front_end/panels/timeline/TimelineDetailsView.test.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ describeWithEnvironment('TimelineDetailsView', function() {
3434
if (!cssRequest) {
3535
throw new Error('Could not find expected network request.');
3636
}
37-
const selection = Timeline.TimelineSelection.TimelineSelection.fromTraceEvent(cssRequest);
37+
const selection = Timeline.TimelineSelection.selectionFromEvent(cssRequest);
3838

3939
await detailsView.setModel(
4040
{parsedTrace, selectedEvents: null, traceInsightsSets: insights, eventToRelatedInsightsMap: null});
@@ -53,7 +53,7 @@ describeWithEnvironment('TimelineDetailsView', function() {
5353

5454
const frame = parsedTrace.Frames.frames.at(0);
5555
assert.isOk(frame);
56-
const selection = Timeline.TimelineSelection.TimelineSelection.fromFrame(frame);
56+
const selection = Timeline.TimelineSelection.selectionFromEvent(frame);
5757
await detailsView.setSelection(selection);
5858
await doubleRaf(); // to let the image be fetched + rendered.
5959
const detailsContentElement = detailsView.getDetailsContentElementForTest();
@@ -76,7 +76,7 @@ describeWithEnvironment('TimelineDetailsView', function() {
7676

7777
const layoutShift = parsedTrace.LayoutShifts.clusters.at(0)?.events.at(0);
7878
assert.isOk(layoutShift);
79-
const selection = Timeline.TimelineSelection.TimelineSelection.fromTraceEvent(layoutShift);
79+
const selection = Timeline.TimelineSelection.selectionFromEvent(layoutShift);
8080
await detailsView.setSelection(selection);
8181
const detailsContentElement = detailsView.getDetailsContentElementForTest();
8282
// Assert that the right component is rendered. This component has its own
@@ -95,7 +95,7 @@ describeWithEnvironment('TimelineDetailsView', function() {
9595

9696
const layoutShiftCluster = parsedTrace.LayoutShifts.clusters.at(0);
9797
assert.isOk(layoutShiftCluster);
98-
const selection = Timeline.TimelineSelection.TimelineSelection.fromTraceEvent(layoutShiftCluster);
98+
const selection = Timeline.TimelineSelection.selectionFromEvent(layoutShiftCluster);
9999
await detailsView.setSelection(selection);
100100
const detailsContentElement = detailsView.getDetailsContentElementForTest();
101101
// Assert that the right component is rendered. This component has its own
@@ -116,7 +116,7 @@ describeWithEnvironment('TimelineDetailsView', function() {
116116
eventToRelatedInsightsMap: null,
117117
});
118118
const bounds = Trace.Helpers.Timing.traceWindowMilliSeconds(parsedTrace.Meta.traceBounds);
119-
const selection = Timeline.TimelineSelection.TimelineSelection.fromRange(
119+
const selection = Timeline.TimelineSelection.selectionFromRangeMilliSeconds(
120120
bounds.min,
121121
bounds.max,
122122
);

front_end/panels/timeline/TimelineDetailsView.ts

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,12 @@ import {targetForEvent} from './TargetForEvent.js';
2020
import {TimelineLayersView} from './TimelineLayersView.js';
2121
import {TimelinePaintProfilerView} from './TimelinePaintProfilerView.js';
2222
import type {TimelineModeViewDelegate} from './TimelinePanel.js';
23-
import {TimelineSelection} from './TimelineSelection.js';
23+
import {
24+
selectionFromRangeMilliSeconds,
25+
selectionIsEvent,
26+
selectionIsRange,
27+
type TimelineSelection,
28+
} from './TimelineSelection.js';
2429
import {TimelineSelectorStatsView} from './TimelineSelectorStatsView.js';
2530
import {BottomUpTimelineTreeView, CallTreeTimelineTreeView, type TimelineTreeView} from './TimelineTreeView.js';
2631
import {TimelineDetailsContentHelper, TimelineUIUtils} from './TimelineUIUtils.js';
@@ -219,7 +224,7 @@ export class TimelineDetailsView extends UI.Widget.VBox {
219224
return;
220225
}
221226
const visibleWindow = traceBoundsState.milli.timelineTraceWindow;
222-
view.updateContents(this.selection || TimelineSelection.fromRange(visibleWindow.min, visibleWindow.max));
227+
view.updateContents(this.selection || selectionFromRangeMilliSeconds(visibleWindow.min, visibleWindow.max));
223228
}
224229
}
225230

@@ -359,15 +364,18 @@ export class TimelineDetailsView extends UI.Widget.VBox {
359364
this.scheduleUpdateContentsFromWindow(/* forceImmediateUpdate */ true);
360365
return;
361366
}
362-
const selectionObject = this.selection.object;
363-
if (TimelineSelection.isSyntheticNetworkRequestDetailsEventSelection(selectionObject)) {
364-
await this.#setSelectionForNetworkEvent(selectionObject);
365-
} else if (TimelineSelection.isTraceEventSelection(selectionObject)) {
366-
await this.#setSelectionForTraceEvent(selectionObject);
367-
} else if (TimelineSelection.isLegacyTimelineFrame(selectionObject)) {
368-
this.#setSelectionForTimelineFrame(selectionObject);
369-
} else if (TimelineSelection.isRangeSelection(selectionObject)) {
370-
this.updateSelectedRangeStats(this.selection.startTime, this.selection.endTime);
367+
368+
if (selectionIsEvent(selection)) {
369+
if (Trace.Types.Events.isSyntheticNetworkRequest(selection.event)) {
370+
await this.#setSelectionForNetworkEvent(selection.event);
371+
} else if (Trace.Types.Events.isLegacyTimelineFrame(selection.event)) {
372+
this.#setSelectionForTimelineFrame(selection.event);
373+
} else {
374+
await this.#setSelectionForTraceEvent(selection.event);
375+
}
376+
} else if (selectionIsRange(selection)) {
377+
const timings = Trace.Helpers.Timing.traceWindowMicroSecondsToMilliSeconds(selection.bounds);
378+
this.updateSelectedRangeStats(timings.min, timings.max);
371379
}
372380

373381
this.updateContents();

front_end/panels/timeline/TimelineFlameChartDataProvider.ts

Lines changed: 14 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,12 @@ import {ModificationsManager} from './ModificationsManager.js';
4545
import {ThreadAppender} from './ThreadAppender.js';
4646
import timelineFlamechartPopoverStyles from './timelineFlamechartPopover.css.js';
4747
import {FlameChartStyle, Selection} from './TimelineFlameChartView.js';
48-
import {TimelineSelection} from './TimelineSelection.js';
48+
import {
49+
selectionFromEvent,
50+
selectionIsRange,
51+
selectionsEqual,
52+
type TimelineSelection,
53+
} from './TimelineSelection.js';
4954
import * as Utils from './utils/utils.js';
5055

5156
const UIStrings = {
@@ -1130,19 +1135,10 @@ export class TimelineFlameChartDataProvider extends Common.ObjectWrapper.ObjectW
11301135

11311136
createSelection(entryIndex: number): TimelineSelection|null {
11321137
const entry = this.entryData[entryIndex];
1133-
if (!entry) {
1134-
return null;
1135-
}
1136-
1137-
let timelineSelection: TimelineSelection|null = null;
1138-
1139-
if (Trace.Types.Events.isLegacyTimelineFrame(entry)) {
1140-
timelineSelection = TimelineSelection.fromFrame(entry);
1141-
} else {
1142-
timelineSelection = TimelineSelection.fromTraceEvent(entry);
1138+
const timelineSelection: TimelineSelection|null = entry ? selectionFromEvent(entry) : null;
1139+
if (timelineSelection) {
1140+
this.lastSelection = new Selection(timelineSelection, entryIndex);
11431141
}
1144-
1145-
this.lastSelection = new Selection(timelineSelection, entryIndex);
11461142
return timelineSelection;
11471143
}
11481144

@@ -1170,27 +1166,25 @@ export class TimelineFlameChartDataProvider extends Common.ObjectWrapper.ObjectW
11701166
}
11711167

11721168
entryIndexForSelection(selection: TimelineSelection|null): number {
1173-
if (!selection || TimelineSelection.isRangeSelection(selection.object) ||
1174-
TimelineSelection.isSyntheticNetworkRequestDetailsEventSelection(selection.object)) {
1169+
if (!selection || selectionIsRange(selection) || Trace.Types.Events.isNetworkTrackEntry(selection.event)) {
11751170
return -1;
11761171
}
11771172

1178-
if (this.lastSelection && this.lastSelection.timelineSelection.object === selection.object) {
1173+
if (this.lastSelection && selectionsEqual(this.lastSelection.timelineSelection, selection)) {
11791174
return this.lastSelection.entryIndex;
11801175
}
11811176

1177+
const index = this.entryData.indexOf(selection.event);
11821178
// If the index is -1 and the selection is a TraceEvent, it might be
11831179
// the case that this Entry is hidden by the Context Menu action.
11841180
// Try revealing the entry and getting the index again.
1185-
if (this.entryData.indexOf(selection.object) === -1 && TimelineSelection.isTraceEventSelection(selection.object)) {
1181+
if (index > -1) {
11861182
if (this.timelineDataInternal?.selectedGroup) {
1187-
ModificationsManager.activeManager()?.getEntriesFilter().revealEntry(
1188-
selection.object as Trace.Types.Events.Event);
1183+
ModificationsManager.activeManager()?.getEntriesFilter().revealEntry(selection.event);
11891184
this.timelineData(true);
11901185
}
11911186
}
11921187

1193-
const index = this.entryData.indexOf(selection.object);
11941188
if (index !== -1) {
11951189
this.lastSelection = new Selection(selection, index);
11961190
}

front_end/panels/timeline/TimelineFlameChartNetworkDataProvider.ts

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,12 @@ import {ModificationsManager} from './ModificationsManager.js';
1616
import {NetworkTrackAppender, type NetworkTrackEvent} from './NetworkTrackAppender.js';
1717
import timelineFlamechartPopoverStyles from './timelineFlamechartPopover.css.js';
1818
import {FlameChartStyle, Selection} from './TimelineFlameChartView.js';
19-
import {TimelineSelection} from './TimelineSelection.js';
19+
import {
20+
selectionFromEvent,
21+
selectionIsRange,
22+
selectionsEqual,
23+
type TimelineSelection,
24+
} from './TimelineSelection.js';
2025
import * as TimelineUtils from './utils/utils.js';
2126

2227
export class TimelineFlameChartNetworkDataProvider implements PerfUI.FlameChart.FlameChartDataProvider {
@@ -120,7 +125,7 @@ export class TimelineFlameChartNetworkDataProvider implements PerfUI.FlameChart.
120125
return null;
121126
}
122127
const event = this.#events[index];
123-
this.#lastSelection = new Selection(TimelineSelection.fromTraceEvent(event), index);
128+
this.#lastSelection = new Selection(selectionFromEvent(event), index);
124129
return this.#lastSelection.timelineSelection;
125130
}
126131

@@ -173,21 +178,21 @@ export class TimelineFlameChartNetworkDataProvider implements PerfUI.FlameChart.
173178
}
174179

175180
entryIndexForSelection(selection: TimelineSelection|null): number {
176-
if (!selection) {
181+
if (!selection || selectionIsRange(selection)) {
177182
return -1;
178183
}
179184

180-
if (this.#lastSelection && this.#lastSelection.timelineSelection.object === selection.object) {
185+
if (this.#lastSelection && selectionsEqual(this.#lastSelection.timelineSelection, selection)) {
181186
return this.#lastSelection.entryIndex;
182187
}
183188

184-
if (!TimelineSelection.isNetworkEventSelection(selection.object)) {
189+
if (!Trace.Types.Events.isNetworkTrackEntry(selection.event)) {
185190
return -1;
186191
}
187192

188-
const index = this.#events.indexOf(selection.object);
193+
const index = this.#events.indexOf(selection.event);
189194
if (index !== -1) {
190-
this.#lastSelection = new Selection(TimelineSelection.fromTraceEvent(selection.object), index);
195+
this.#lastSelection = new Selection(selectionFromEvent(selection.event), index);
191196
}
192197
return index;
193198
}

front_end/panels/timeline/TimelineFlameChartView.test.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -93,11 +93,10 @@ describeWithEnvironment('TimelineFlameChartView', function() {
9393

9494
function assertSelectionName(name: string) {
9595
const selection = mockViewDelegate.selection;
96-
if (!selection || !Timeline.TimelineSelection.TimelineSelection.isTraceEventSelection(selection.object)) {
96+
if (!selection || !Timeline.TimelineSelection.selectionIsEvent(selection)) {
9797
throw new Error('Selection is not present or not a Trace Event');
9898
}
99-
const object = selection.object;
100-
assert.strictEqual(object.name, name);
99+
assert.strictEqual(selection.event.name, name);
101100
}
102101
});
103102

front_end/panels/timeline/TimelineFlameChartView.ts

Lines changed: 22 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,14 @@ import {
3131
import {TimelineFlameChartNetworkDataProvider} from './TimelineFlameChartNetworkDataProvider.js';
3232
import timelineFlameChartViewStyles from './timelineFlameChartView.css.js';
3333
import type {TimelineModeViewDelegate} from './TimelinePanel.js';
34-
import {TimelineSelection} from './TimelineSelection.js';
34+
import {
35+
rangeForSelection,
36+
selectionFromEvent,
37+
selectionFromRangeMilliSeconds,
38+
selectionIsEvent,
39+
selectionIsRange,
40+
type TimelineSelection,
41+
} from './TimelineSelection.js';
3542
import {AggregatedTimelineTreeView} from './TimelineTreeView.js';
3643
import type {TimelineMarkerStyle} from './TimelineUIUtils.js';
3744

@@ -607,7 +614,7 @@ export class TimelineFlameChartView extends UI.Widget.VBox implements PerfUI.Fla
607614
let startTime = visibleWindow.min;
608615
// Prefer the start time of the selected event, if there is one.
609616
if (this.#currentSelection) {
610-
startTime = Trace.Helpers.Timing.millisecondsToMicroseconds(this.#currentSelection.startTime);
617+
startTime = rangeForSelection(this.#currentSelection).min;
611618
}
612619
this.#createNewTimeRangeFromKeyboard(
613620
startTime, Trace.Types.Timing.MicroSeconds(startTime + timeRangeIncrementValue));
@@ -808,7 +815,9 @@ export class TimelineFlameChartView extends UI.Widget.VBox implements PerfUI.Fla
808815
* TODO(crbug.com/346312365): update the type definitions in ChartViewport.ts
809816
*/
810817
updateRangeSelection(startTime: number, endTime: number): void {
811-
this.delegate.select(TimelineSelection.fromRange(startTime, endTime));
818+
this.delegate.select(selectionFromRangeMilliSeconds(
819+
Trace.Types.Timing.MilliSeconds(startTime), Trace.Types.Timing.MilliSeconds(endTime)));
820+
812821
if (Root.Runtime.experiments.isEnabled(Root.Runtime.ExperimentName.TIMELINE_ANNOTATIONS)) {
813822
const bounds = Trace.Helpers.Timing.traceWindowFromMilliSeconds(
814823
Trace.Types.Timing.MilliSeconds(startTime),
@@ -1008,8 +1017,7 @@ export class TimelineFlameChartView extends UI.Widget.VBox implements PerfUI.Fla
10081017
}
10091018

10101019
highlightEvent(event: Trace.Types.Events.Event|null): void {
1011-
const entryIndex =
1012-
event ? this.mainDataProvider.entryIndexForSelection(TimelineSelection.fromTraceEvent(event)) : -1;
1020+
const entryIndex = event ? this.mainDataProvider.entryIndexForSelection(selectionFromEvent(event)) : -1;
10131021
if (entryIndex >= 0) {
10141022
this.mainFlameChart.highlightEntry(entryIndex);
10151023
} else {
@@ -1075,8 +1083,8 @@ export class TimelineFlameChartView extends UI.Widget.VBox implements PerfUI.Fla
10751083
// AND 2. we have an active time range selection overlay
10761084
// AND 3. The label of the selection is empty
10771085
// then we need to remove it.
1078-
if ((selection === null || !TimelineSelection.isRangeSelection(selection.object)) &&
1079-
this.#timeRangeSelectionAnnotation && !this.#timeRangeSelectionAnnotation.label) {
1086+
if ((selection === null || !selectionIsRange(selection)) && this.#timeRangeSelectionAnnotation &&
1087+
!this.#timeRangeSelectionAnnotation.label) {
10801088
ModificationsManager.activeManager()?.removeAnnotation(this.#timeRangeSelectionAnnotation);
10811089
this.#timeRangeSelectionAnnotation = null;
10821090
}
@@ -1090,14 +1098,11 @@ export class TimelineFlameChartView extends UI.Widget.VBox implements PerfUI.Fla
10901098
void this.detailsView.setSelection(selection);
10911099
}
10921100

1093-
// Create the entry selected overlay if the selection represents a frame or trace event (either network, or anything else)
1094-
if (selection &&
1095-
(TimelineSelection.isTraceEventSelection(selection.object) ||
1096-
TimelineSelection.isSyntheticNetworkRequestDetailsEventSelection(selection.object) ||
1097-
TimelineSelection.isLegacyTimelineFrame(selection.object))) {
1101+
// Create the entry selected overlay if the selection represents a trace event
1102+
if (selectionIsEvent(selection)) {
10981103
this.addOverlay({
10991104
type: 'ENTRY_SELECTED',
1100-
entry: selection.object,
1105+
entry: selection.event,
11011106
});
11021107
}
11031108

@@ -1151,14 +1156,11 @@ export class TimelineFlameChartView extends UI.Widget.VBox implements PerfUI.Fla
11511156
dataProvider: TimelineFlameChartDataProvider|TimelineFlameChartNetworkDataProvider,
11521157
event: Common.EventTarget.EventTargetEvent<{entryIndex: number, withLinkCreationButton: boolean}>): void {
11531158
const selection = dataProvider.createSelection(event.data.entryIndex);
1154-
if (selection &&
1155-
(TimelineSelection.isTraceEventSelection(selection.object) ||
1156-
TimelineSelection.isSyntheticNetworkRequestDetailsEventSelection(selection.object) ||
1157-
TimelineSelection.isLegacyTimelineFrame(selection.object))) {
1159+
if (selectionIsEvent(selection)) {
11581160
this.setSelectionAndReveal(selection);
11591161
ModificationsManager.activeManager()?.createAnnotation({
11601162
type: 'ENTRY_LABEL',
1161-
entry: selection.object,
1163+
entry: selection.event,
11621164
label: '',
11631165
});
11641166
if (event.data.withLinkCreationButton) {
@@ -1187,22 +1189,9 @@ export class TimelineFlameChartView extends UI.Widget.VBox implements PerfUI.Fla
11871189

11881190
#selectionIfTraceEvent(
11891191
index: number, dataProvider: TimelineFlameChartDataProvider|TimelineFlameChartNetworkDataProvider):
1190-
Trace.Types.Events.Event|Trace.Types.Events.SyntheticNetworkRequest|null {
1192+
Trace.Types.Events.Event|null {
11911193
const selection = dataProvider.createSelection(index);
1192-
if (!selection) {
1193-
return null;
1194-
}
1195-
1196-
if (TimelineSelection.isTraceEventSelection(selection.object) ||
1197-
TimelineSelection.isSyntheticNetworkRequestDetailsEventSelection(selection.object)) {
1198-
return selection.object;
1199-
}
1200-
1201-
if (TimelineSelection.isLegacyTimelineFrame(selection.object)) {
1202-
return selection.object as Trace.Types.Events.LegacyTimelineFrame;
1203-
}
1204-
1205-
return null;
1194+
return selectionIsEvent(selection) ? selection.event : null;
12061195
}
12071196

12081197
/**

0 commit comments

Comments
 (0)