Skip to content

Commit 734f98e

Browse files
Connor ClarkDevtools-frontend LUCI CQ
authored andcommitted
[RPP] Define common checklist type, component type for insights
Bug: 394402056 Change-Id: I2a0eae4b120678663b143cab6b6292d48df93847 Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6235989 Commit-Queue: Paul Irish <[email protected]> Auto-Submit: Connor Clark <[email protected]> Reviewed-by: Paul Irish <[email protected]>
1 parent a0397ce commit 734f98e

File tree

14 files changed

+206
-155
lines changed

14 files changed

+206
-155
lines changed

config/gni/devtools_grd_files.gni

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1861,6 +1861,7 @@ grd_files_debug_sources = [
18611861
"front_end/panels/timeline/components/ignoreListSetting.css.js",
18621862
"front_end/panels/timeline/components/insights/BaseInsightComponent.js",
18631863
"front_end/panels/timeline/components/insights/CLSCulprits.js",
1864+
"front_end/panels/timeline/components/insights/Checklist.js",
18641865
"front_end/panels/timeline/components/insights/DOMSize.js",
18651866
"front_end/panels/timeline/components/insights/DocumentLatency.js",
18661867
"front_end/panels/timeline/components/insights/EventRef.js",
@@ -1880,6 +1881,7 @@ grd_files_debug_sources = [
18801881
"front_end/panels/timeline/components/insights/ThirdParties.js",
18811882
"front_end/panels/timeline/components/insights/Viewport.js",
18821883
"front_end/panels/timeline/components/insights/baseInsightComponent.css.js",
1884+
"front_end/panels/timeline/components/insights/checklist.css.js",
18831885
"front_end/panels/timeline/components/insights/table.css.js",
18841886
"front_end/panels/timeline/components/insights/types.js",
18851887
"front_end/panels/timeline/components/interactionBreakdown.css.js",

front_end/models/trace/insights/DocumentLatency.test.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ describeWithEnvironment('DocumentLatency', function() {
2727
const insight =
2828
getInsightOrError('DocumentLatency', insights, getFirstOrError(data.Meta.navigationsByNavigationId.values()));
2929
assert.strictEqual(insight.data?.serverResponseTime, 43);
30-
assert(!insight.data?.serverResponseTooSlow);
30+
assert(insight.data?.checklist.serverResponseIsFast.value === true);
3131
assert.deepEqual(insight.metricSavings, {FCP: 0, LCP: 0} as Trace.Insights.Types.MetricSavings);
3232
});
3333

@@ -56,7 +56,7 @@ describeWithEnvironment('DocumentLatency', function() {
5656
const context = createContextForNavigation(data, navigation, data.Meta.mainFrameId);
5757
const insight = Trace.Insights.Models.DocumentLatency.generateInsight(data, context);
5858
assert.strictEqual(insight.data?.serverResponseTime, 1043);
59-
assert(insight.data?.serverResponseTooSlow);
59+
assert(insight.data?.checklist.serverResponseIsFast.value === false);
6060
assert.deepEqual(insight.metricSavings, {FCP: 943, LCP: 943} as Trace.Insights.Types.MetricSavings);
6161
});
6262

@@ -100,7 +100,7 @@ describeWithEnvironment('DocumentLatency', function() {
100100
assert.strictEqual(insight.data?.redirectDuration, 6059);
101101
assert.strictEqual(insight.data?.uncompressedResponseBytes, 111506);
102102
assert.strictEqual(insight.data?.serverResponseTime, 2008);
103-
assert(insight.data?.serverResponseTooSlow);
103+
assert(insight.data?.checklist.serverResponseIsFast.value === false);
104104
assert.deepEqual(insight.metricSavings, {FCP: 7967, LCP: 7967} as Trace.Insights.Types.MetricSavings);
105105
});
106106
});

front_end/models/trace/insights/DocumentLatency.ts

Lines changed: 27 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import * as Helpers from '../helpers/helpers.js';
77
import * as Types from '../types/types.js';
88

99
import {
10+
type Checklist,
1011
InsightCategory,
1112
type InsightModel,
1213
type InsightSetContext,
@@ -61,16 +62,6 @@ export const UIStrings = {
6162
* @description Text for a label describing a network request event as taking longer to download because it wasn't compressed.
6263
*/
6364
uncompressedDownload: 'Uncompressed download',
64-
/**
65-
*@description Text for a screen-reader label to tell the user that the icon represents a successful insight check
66-
*@example {Server response time} PH1
67-
*/
68-
successAriaLabel: 'Insight check passed: {PH1}',
69-
/**
70-
*@description Text for a screen-reader label to tell the user that the icon represents an unsuccessful insight check
71-
*@example {Server response time} PH1
72-
*/
73-
failedAriaLabel: 'Insight check failed: {PH1}',
7465
};
7566

7667
const str_ = i18n.i18n.registerUIStrings('models/trace/insights/DocumentLatency.ts', UIStrings);
@@ -87,10 +78,10 @@ const IGNORE_THRESHOLD_IN_BYTES = 1400;
8778
export type DocumentLatencyInsightModel = InsightModel<typeof UIStrings, {
8879
data?: {
8980
serverResponseTime: Types.Timing.Milli,
90-
serverResponseTooSlow: boolean,
9181
redirectDuration: Types.Timing.Milli,
9282
uncompressedResponseBytes: number,
9383
documentRequest?: Types.Events.SyntheticNetworkRequest,
84+
checklist: Checklist<'noRedirects'|'serverResponseIsFast'|'usesCompression'>,
9485
},
9586
}>;
9687

@@ -175,8 +166,8 @@ function getCompressionSavings(request: Types.Events.SyntheticNetworkRequest): n
175166
function finalize(partialModel: PartialInsightModel<DocumentLatencyInsightModel>): DocumentLatencyInsightModel {
176167
let hasFailure = false;
177168
if (partialModel.data) {
178-
hasFailure = partialModel.data.redirectDuration > 0 || partialModel.data.serverResponseTooSlow ||
179-
partialModel.data.uncompressedResponseBytes > 0;
169+
hasFailure = !partialModel.data.checklist.usesCompression.value ||
170+
!partialModel.data.checklist.serverResponseIsFast.value || !partialModel.data.checklist.noRedirects.value;
180171
}
181172

182173
return {
@@ -221,14 +212,35 @@ export function generateInsight(
221212
LCP: overallSavingsMs as Types.Timing.Milli,
222213
};
223214

215+
const uncompressedResponseBytes = getCompressionSavings(documentRequest);
216+
217+
const noRedirects = redirectDuration === 0;
218+
const serverResponseIsFast = !serverResponseTooSlow;
219+
const usesCompression = uncompressedResponseBytes === 0;
220+
224221
return finalize({
225222
relatedEvents: [documentRequest],
226223
data: {
227224
serverResponseTime,
228-
serverResponseTooSlow,
229225
redirectDuration: Types.Timing.Milli(redirectDuration),
230-
uncompressedResponseBytes: getCompressionSavings(documentRequest),
226+
uncompressedResponseBytes,
231227
documentRequest,
228+
checklist: {
229+
noRedirects: {
230+
label: noRedirects ? i18nString(UIStrings.passingRedirects) : i18nString(UIStrings.failedRedirects),
231+
value: noRedirects
232+
},
233+
serverResponseIsFast: {
234+
label: serverResponseIsFast ? i18nString(UIStrings.passingServerResponseTime) :
235+
i18nString(UIStrings.failedServerResponseTime),
236+
value: serverResponseIsFast
237+
},
238+
usesCompression: {
239+
label: usesCompression ? i18nString(UIStrings.passingTextCompression) :
240+
i18nString(UIStrings.failedTextCompression),
241+
value: usesCompression
242+
},
243+
},
232244
},
233245
metricSavings,
234246
});

front_end/models/trace/insights/LCPDiscovery.test.ts

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,12 @@ describeWithEnvironment('LCPDiscovery', function() {
1010
const {data, insights} = await processTrace(this, 'lcp-images.json.gz');
1111
const firstNav = getFirstOrError(data.Meta.navigationsByNavigationId.values());
1212
const insight = getInsightOrError('LCPDiscovery', insights, firstNav);
13-
const {shouldIncreasePriorityHint, shouldPreloadImage, shouldRemoveLazyLoading} = insight;
13+
const {checklist} = insight;
1414

15-
assert.isFalse(shouldRemoveLazyLoading);
16-
assert.isFalse(shouldPreloadImage);
17-
assert.isTrue(shouldIncreasePriorityHint);
15+
assert.exists(checklist);
16+
assert.isFalse(checklist.priorityHinted.value);
17+
assert.isTrue(checklist.requestDiscoverable.value);
18+
assert.isTrue(checklist.eagerlyLoaded.value);
1819
});
1920

2021
it('calculates the LCP optimal time as the document request download start time', async () => {

front_end/models/trace/insights/LCPDiscovery.ts

Lines changed: 10 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import * as Helpers from '../helpers/helpers.js';
88
import * as Types from '../types/types.js';
99

1010
import {
11+
type Checklist,
1112
InsightCategory,
1213
type InsightModel,
1314
type InsightSetContext,
@@ -43,16 +44,6 @@ export const UIStrings = {
4344
* @description Text to tell the user that the LCP request does not have the lazy load property applied.
4445
*/
4546
lazyLoadNotApplied: 'lazy load not applied',
46-
/**
47-
*@description Text for a screen-reader label to tell the user that the icon represents a successful insight check
48-
*@example {Server response time} PH1
49-
*/
50-
successAriaLabel: 'Insight check passed: {PH1}',
51-
/**
52-
*@description Text for a screen-reader label to tell the user that the icon represents an unsuccessful insight check
53-
*@example {Server response time} PH1
54-
*/
55-
failedAriaLabel: 'Insight check failed: {PH1}',
5647
/**
5748
* @description Text status indicating that the the Largest Contentful Paint (LCP) metric timing was not found. "LCP" is an acronym and should not be translated.
5849
*/
@@ -72,12 +63,10 @@ export function deps(): ['NetworkRequests', 'PageLoadMetrics', 'LargestImagePain
7263

7364
export type LCPDiscoveryInsightModel = InsightModel<typeof UIStrings, {
7465
lcpEvent?: Types.Events.LargestContentfulPaintCandidate,
75-
shouldRemoveLazyLoading?: boolean,
76-
shouldIncreasePriorityHint?: boolean,
77-
shouldPreloadImage?: boolean,
7866
/** The network request for the LCP image, if there was one. */
7967
lcpRequest?: Types.Events.SyntheticNetworkRequest,
8068
earliestDiscoveryTimeTs?: Types.Timing.Micro,
69+
checklist?: Checklist<'priorityHinted'|'requestDiscoverable'|'eagerlyLoaded'>,
8170
}>;
8271

8372
function finalize(partialModel: PartialInsightModel<LCPDiscoveryInsightModel>): LCPDiscoveryInsightModel {
@@ -91,9 +80,9 @@ function finalize(partialModel: PartialInsightModel<LCPDiscoveryInsightModel>):
9180
description: i18nString(UIStrings.description),
9281
category: InsightCategory.LCP,
9382
shouldShow: Boolean(
94-
partialModel.lcpRequest &&
95-
(partialModel.shouldIncreasePriorityHint || partialModel.shouldPreloadImage ||
96-
partialModel.shouldRemoveLazyLoading)),
83+
partialModel.lcpRequest && partialModel.checklist &&
84+
(!partialModel.checklist.eagerlyLoaded.value || !partialModel.checklist.requestDiscoverable.value ||
85+
!partialModel.checklist.priorityHinted.value)),
9786
...partialModel,
9887
relatedEvents,
9988
};
@@ -149,10 +138,12 @@ export function generateInsight(
149138

150139
return finalize({
151140
lcpEvent,
152-
shouldRemoveLazyLoading: imageLoadingAttr === 'lazy',
153-
shouldIncreasePriorityHint: imageFetchPriorityHint !== 'high',
154-
shouldPreloadImage: !imgPreloadedOrFoundInHTML,
155141
lcpRequest,
156142
earliestDiscoveryTimeTs: earliestDiscoveryTime ? Types.Timing.Micro(earliestDiscoveryTime) : undefined,
143+
checklist: {
144+
priorityHinted: {label: i18nString(UIStrings.fetchPriorityApplied), value: imageFetchPriorityHint === 'high'},
145+
requestDiscoverable: {label: i18nString(UIStrings.requestDiscoverable), value: imgPreloadedOrFoundInHTML},
146+
eagerlyLoaded: {label: i18nString(UIStrings.lazyLoadNotApplied), value: imageLoadingAttr !== 'lazy'},
147+
},
157148
});
158149
}

front_end/models/trace/insights/types.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,8 @@ export enum InsightCategory {
7777

7878
export type RelatedEventsMap = Map<Types.Events.Event, string[]>;
7979

80+
export type Checklist<Keys extends string> = Record<Keys, {label: Common.UIString.LocalizedString, value: boolean}>;
81+
8082
export type InsightModel<UIStrings extends Record<string, string>, R extends Record<string, unknown>> = R&{
8183
/** Not used within DevTools - this is for external consumers (like Lighthouse). */
8284
strings: UIStrings,

front_end/panels/timeline/components/insights/BUILD.gn

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import("../../../../../scripts/build/typescript/typescript.gni")
1010
generate_css("css_files") {
1111
sources = [
1212
"baseInsightComponent.css",
13+
"checklist.css",
1314
"table.css",
1415
]
1516
}
@@ -18,6 +19,7 @@ devtools_module("insights") {
1819
sources = [
1920
"BaseInsightComponent.ts",
2021
"CLSCulprits.ts",
22+
"Checklist.ts",
2123
"DOMSize.ts",
2224
"DocumentLatency.ts",
2325
"EventRef.ts",
Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
// Copyright 2024 The Chromium Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
/**
6+
* @fileoverview A list of pass/fail conditions for an insight.
7+
*/
8+
9+
import '../../../../ui/components/icon_button/icon_button.js';
10+
11+
import * as i18n from '../../../../core/i18n/i18n.js';
12+
import type * as Trace from '../../../../models/trace/trace.js';
13+
import * as ComponentHelpers from '../../../../ui/components/helpers/helpers.js';
14+
import * as UI from '../../../../ui/legacy/legacy.js';
15+
import * as Lit from '../../../../ui/lit/lit.js';
16+
import type * as Overlays from '../../overlays/overlays.js';
17+
18+
import checklistStylesRaw from './checklist.css.js';
19+
20+
const UIStrings = {
21+
/**
22+
*@description Text for a screen-reader label to tell the user that the icon represents a successful insight check
23+
*@example {Server response time} PH1
24+
*/
25+
successAriaLabel: 'Insight check passed: {PH1}',
26+
/**
27+
*@description Text for a screen-reader label to tell the user that the icon represents an unsuccessful insight check
28+
*@example {Server response time} PH1
29+
*/
30+
failedAriaLabel: 'Insight check failed: {PH1}',
31+
};
32+
33+
const str_ = i18n.i18n.registerUIStrings('panels/timeline/components/insights/Checklist.ts', UIStrings);
34+
const i18nString = i18n.i18n.getLocalizedString.bind(undefined, str_);
35+
36+
// TODO(crbug.com/391381439): Fully migrate off of constructed style sheets.
37+
const checklistStyles = new CSSStyleSheet();
38+
checklistStyles.replaceSync(checklistStylesRaw.cssContent);
39+
40+
const {html} = Lit;
41+
42+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
43+
type GenericChecklist = Trace.Insights.Types.Checklist<any>;
44+
45+
export interface ChecklistData {
46+
checklist: GenericChecklist;
47+
}
48+
49+
export interface TableDataRow {
50+
values: Array<number|string|Lit.LitTemplate>;
51+
overlays?: Overlays.Overlays.TimelineOverlay[];
52+
}
53+
54+
export class Checklist extends HTMLElement {
55+
readonly #shadow = this.attachShadow({mode: 'open'});
56+
readonly #boundRender = this.#render.bind(this);
57+
#checklist?: GenericChecklist;
58+
59+
set checklist(checklist: GenericChecklist) {
60+
this.#checklist = checklist;
61+
void ComponentHelpers.ScheduledRender.scheduleRender(this, this.#boundRender);
62+
}
63+
64+
connectedCallback(): void {
65+
this.#shadow.adoptedStyleSheets.push(checklistStyles);
66+
UI.UIUtils.injectCoreStyles(this.#shadow);
67+
68+
void ComponentHelpers.ScheduledRender.scheduleRender(this, this.#boundRender);
69+
}
70+
71+
#getIcon(check: GenericChecklist['']): Lit.TemplateResult {
72+
const icon = check.value ? 'check-circle' : 'clear';
73+
74+
const ariaLabel = check.value ? i18nString(UIStrings.successAriaLabel, {PH1: check.label}) :
75+
i18nString(UIStrings.failedAriaLabel, {PH1: check.label});
76+
return html`
77+
<devtools-icon
78+
aria-label=${ariaLabel}
79+
name=${icon}
80+
class=${check.value ? 'check-passed' : 'check-failed'}
81+
></devtools-icon>
82+
`;
83+
}
84+
85+
async #render(): Promise<void> {
86+
if (!this.#checklist) {
87+
return;
88+
}
89+
90+
Lit.render(
91+
html`
92+
<ul>
93+
${Object.values(this.#checklist).map(check => html`<li>
94+
${this.#getIcon(check)}
95+
<span>${check.label}</span>
96+
</li>`)}
97+
</ul>`,
98+
this.#shadow, {host: this});
99+
}
100+
}
101+
102+
declare global {
103+
interface HTMLElementTagNameMap {
104+
'devtools-performance-checklist': Checklist;
105+
}
106+
}
107+
108+
customElements.define('devtools-performance-checklist', Checklist);

0 commit comments

Comments
 (0)