Skip to content

Commit

Permalink
[perf] Remove ingest links from tooltip
Browse files Browse the repository at this point in the history
The ingest links can get really long, especially for instances like
AndroidX where they have custom perfetto links for their data points.

This change removes it. In fact, all calls to it were setting it false
anyways.

point-links, which shows the range of commits from the previous
to current, should be sufficient. Per feedback, this is more
beneficial to see all commit information centrally in the tooltip.

Change-Id: I844c4bbb984f665bd6dbd2ca1bd2382f5023ddf6
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/942097
Reviewed-by: Wenbin Zhang <[email protected]>
Commit-Queue: Jeff Yoon <[email protected]>
Reviewed-by: Tony Seaward <[email protected]>
  • Loading branch information
Jeff Yoon authored and SkCQ committed Jan 27, 2025
1 parent 451eb01 commit e4eead4
Show file tree
Hide file tree
Showing 5 changed files with 3 additions and 24 deletions.
1 change: 0 additions & 1 deletion perf/modules/chart-tooltip-sk/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ sk_element(
sk_element_deps = [
"//perf/modules/anomaly-sk",
"//perf/modules/commit-range-sk",
"//perf/modules/ingest-file-links-sk",
"//perf/modules/triage-menu-sk",
"//perf/modules/user-issue-sk",
"//elements-sk/modules/icons/close-icon-sk",
Expand Down
1 change: 0 additions & 1 deletion perf/modules/chart-tooltip-sk/chart-tooltip-sk-demo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,6 @@ const renderTooltips = () => {
dummyAnomaly(12345),
null,
c,
false,
true,
() => {}
);
Expand Down
15 changes: 1 addition & 14 deletions perf/modules/chart-tooltip-sk/chart-tooltip-sk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import { AnomalySk } from '../anomaly-sk/anomaly-sk';
import { lookupCids } from '../cid/cid';
import { CommitRangeSk } from '../commit-range-sk/commit-range-sk';
import '../window/window';
import { IngestFileLinksSk } from '../ingest-file-links-sk/ingest-file-links-sk';
import { TriageMenuSk, NudgeEntry } from '../triage-menu-sk/triage-menu-sk';
import '../triage-menu-sk/triage-menu-sk';
import '../user-issue-sk/user-issue-sk';
Expand Down Expand Up @@ -140,9 +139,6 @@ export class ChartTooltipSk extends ElementSk {
// is selected.
commitRangeSk: CommitRangeSk | null = null;

// Ingest file links element. Provides links based on cid and
ingestFileLinks: IngestFileLinksSk | null = null;

// Shows any buganizer issue associated with a data point.
userIssueSk: UserIssueSk | null = null;

Expand Down Expand Up @@ -192,7 +188,7 @@ export class ChartTooltipSk extends ElementSk {
</div>
<commit-info-sk .commitInfo=${ele.commitInfo}></commit-info-sk>
<commit-range-sk id="tooltip-commit-range-sk"></commit-range-sk>
<ingest-file-links-sk id="tooltip-ingest-file-links"></ingest-file-links-sk>
<point-links-sk id="point-links"></point-links-sk>
${ele.seeMoreText()} ${ele.anomalyTemplate()}
<triage-menu-sk
id="triage-menu"
Expand Down Expand Up @@ -342,7 +338,6 @@ export class ChartTooltipSk extends ElementSk {
this._render();

this.commitRangeSk = this.querySelector('#tooltip-commit-range-sk');
this.ingestFileLinks = this.querySelector('#tooltip-ingest-file-links');
this.userIssueSk = this.querySelector('#tooltip-user-issue-sk');
this.triageMenu = this.querySelector('#triage-menu');

Expand Down Expand Up @@ -383,7 +378,6 @@ export class ChartTooltipSk extends ElementSk {
anomaly: Anomaly | null,
nudgeList: NudgeEntry[] | null,
commit: Commit | null,
displayFileLinks: boolean,
tooltipFixed: boolean,
closeButtonAction: () => void
): void {
Expand All @@ -398,10 +392,6 @@ export class ChartTooltipSk extends ElementSk {
this._close_button_action = closeButtonAction;
this.commitInfo = commit;

if (displayFileLinks && commit_position !== null && test_name !== '') {
this.ingestFileLinks?.load(commit_position, test_name);
}

if (this.userIssueSk !== null) {
this.userIssueSk.bug_id = bug_id;
this.userIssueSk.trace_key = removeSpecialFunctions(this._trace_name);
Expand Down Expand Up @@ -451,9 +441,6 @@ export class ChartTooltipSk extends ElementSk {

set commit_position(val: CommitNumber | null) {
this._commit_position = val;
if (val && this.test_name !== '') {
this.ingestFileLinks?.load(val, this.test_name);
}
this._render();
}

Expand Down
2 changes: 0 additions & 2 deletions perf/modules/chart-tooltip-sk/chart-tooltip-sk_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ describe('chart-tooltip-sk', () => {
null,
null,
false,
false,
() => {}
);
assert.equal(element.test_name, test_name);
Expand All @@ -105,7 +104,6 @@ describe('chart-tooltip-sk', () => {
null,
null,
false,
false,
() => {}
);
assert.equal(element.test_name, test_name);
Expand Down
8 changes: 2 additions & 6 deletions perf/modules/explore-simple-sk/explore-simple-sk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1155,7 +1155,6 @@ export class ExploreSimpleSk extends ElementSk {
name: chart.getTraceName(index.col),
},
json.commitSlice![0],
false,
true
);

Expand Down Expand Up @@ -1208,7 +1207,6 @@ export class ExploreSimpleSk extends ElementSk {
name: chart.getTraceName(index.col),
},
commit,
false,
true
);
}
Expand Down Expand Up @@ -1853,7 +1851,7 @@ export class ExploreSimpleSk extends ElementSk {
c = this.pointToCommitDetailMap.get(key) || null;
}

this.enableTooltip(detail, c, false, false);
this.enableTooltip(detail, c, false);
}
}

Expand Down Expand Up @@ -1976,7 +1974,6 @@ export class ExploreSimpleSk extends ElementSk {
enableTooltip(
pointDetails: PlotSimpleSkTraceEventDetails,
commit: Commit | null,
displayFileLinks: boolean,
fixTooltip: boolean
): void {
// explore-simple-sk is used multiple times on the multi-graph view. To
Expand Down Expand Up @@ -2077,7 +2074,6 @@ export class ExploreSimpleSk extends ElementSk {
anomaly,
nudgeList,
commit,
displayFileLinks,
fixTooltip,
closeBtnAction
);
Expand Down Expand Up @@ -2264,7 +2260,7 @@ export class ExploreSimpleSk extends ElementSk {
if (tooltipEnabled && hasValidTooltipPos) {
this.tooltipSelected = true;

this.enableTooltip(detail, json.commitSlice![0], true, true);
this.enableTooltip(detail, json.commitSlice![0], true);
}
})
.catch(errorMessage);
Expand Down

0 comments on commit e4eead4

Please sign in to comment.