Skip to content

Commit 435823b

Browse files
Adam RaineDevtools-frontend LUCI CQ
Adam Raine
authored and
Devtools-frontend LUCI CQ
committed
[RPP] Always render passed insights
Some insights would render nothing under passing conditions, but we should render the audit card in the passing insights section even if it has empty content. This CL makes the overall insight render function private and forces each insight to override the `renderContent` function instead. All insights were basically following this pattern already. Bug: 369766999 Change-Id: I91af2f977b63bd1a55ec86ae6964ac0ed8d39a17 Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6068286 Reviewed-by: Connor Clark <[email protected]> Commit-Queue: Adam Raine <[email protected]>
1 parent a416dab commit 435823b

15 files changed

+78
-151
lines changed

‎front_end/panels/timeline/components/SidebarSingleInsightSet.test.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,8 @@ describeWithEnvironment('SidebarSingleInsightSet', () => {
6060
'Render blocking requests',
6161
'Document request latency',
6262
'Third parties',
63+
'INP by phase',
64+
'Layout shift culprits',
6365
'Improve image delivery',
6466
'Optimize viewport for mobile',
6567
'CSS Selector costs',
@@ -69,6 +71,8 @@ describeWithEnvironment('SidebarSingleInsightSet', () => {
6971
return getCleanTextContentFromElements(component.shadowRoot!, '.insight-title');
7072
});
7173
assert.deepEqual(passedInsightTitles, [
74+
'INP by phase',
75+
'Layout shift culprits',
7276
'Improve image delivery',
7377
'Optimize viewport for mobile',
7478
'CSS Selector costs',
@@ -95,8 +99,11 @@ describeWithEnvironment('SidebarSingleInsightSet', () => {
9599
assert.deepEqual(userVisibleTitles, [
96100
'LCP by phase',
97101
'LCP request discovery',
102+
'Layout shift culprits',
98103
'Improve image delivery',
99104
'Third parties',
105+
'INP by phase',
106+
'Render blocking requests',
100107
'Document request latency',
101108
'Optimize viewport for mobile',
102109
'CSS Selector costs',
@@ -107,6 +114,8 @@ describeWithEnvironment('SidebarSingleInsightSet', () => {
107114
});
108115
// Does not include "font display", which is experimental.
109116
assert.deepEqual(passedInsightTitles, [
117+
'INP by phase',
118+
'Render blocking requests',
110119
'Document request latency',
111120
'Optimize viewport for mobile',
112121
'CSS Selector costs',
@@ -136,9 +145,12 @@ describeWithEnvironment('SidebarSingleInsightSet', () => {
136145
assert.deepEqual(userVisibleTitles, [
137146
'LCP by phase',
138147
'LCP request discovery',
148+
'Layout shift culprits',
139149
'Improve image delivery',
140150
'Font display',
141151
'Third parties',
152+
'INP by phase',
153+
'Render blocking requests',
142154
'Document request latency',
143155
'Optimize viewport for mobile',
144156
'CSS Selector costs',
@@ -148,6 +160,8 @@ describeWithEnvironment('SidebarSingleInsightSet', () => {
148160
return getCleanTextContentFromElements(component.shadowRoot!, '.insight-title');
149161
});
150162
assert.deepEqual(passedInsightTitles, [
163+
'INP by phase',
164+
'Render blocking requests',
151165
'Document request latency',
152166
'Optimize viewport for mobile',
153167
'CSS Selector costs',

‎front_end/panels/timeline/components/insights/BaseInsightComponent.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,8 @@ describeWithEnvironment('BaseInsightComponent', () => {
2323
override createOverlays(): TimelineOverlay[] {
2424
return [];
2525
}
26-
override render(): void {
27-
this.renderWithContent(html`<div>test content</div>`);
26+
override renderContent(): LitHtml.LitTemplate {
27+
return html`<div>test content</div>`;
2828
}
2929
}
3030
customElements.define('test-insight-component', TestInsightComponent);

‎front_end/panels/timeline/components/insights/BaseInsightComponent.ts‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ export abstract class BaseInsightComponent<T extends InsightModel<{}>> extends H
7070
};
7171

7272
// eslint-disable-next-line rulesdir/no_bound_component_methods
73-
readonly #boundRender = this.render.bind(this);
73+
readonly #boundRender = this.#render.bind(this);
7474
readonly sharedTableState: TableState = {
7575
selectedRowEl: null,
7676
selectionIsSticky: false,
@@ -196,7 +196,16 @@ export abstract class BaseInsightComponent<T extends InsightModel<{}>> extends H
196196

197197
protected abstract createOverlays(): Overlays.Overlays.TimelineOverlay[];
198198

199-
protected abstract render(): void;
199+
protected abstract renderContent(): LitHtml.LitTemplate;
200+
201+
#render(): void {
202+
if (!this.model) {
203+
return;
204+
}
205+
206+
const output = this.renderContent();
207+
this.#renderWithContent(output);
208+
}
200209

201210
getEstimatedSavingsTime(): Trace.Types.Timing.MilliSeconds|null {
202211
return null;
@@ -238,8 +247,8 @@ export abstract class BaseInsightComponent<T extends InsightModel<{}>> extends H
238247
return null;
239248
}
240249

241-
protected renderWithContent(content: LitHtml.LitTemplate): void {
242-
if (content === LitHtml.nothing || !this.#model) {
250+
#renderWithContent(content: LitHtml.LitTemplate): void {
251+
if (!this.#model) {
243252
LitHtml.render(LitHtml.nothing, this.#shadowRoot, {host: this});
244253
return;
245254
}

‎front_end/panels/timeline/components/insights/CLSCulprits.ts‎

Lines changed: 15 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -125,9 +125,21 @@ export class CLSCulprits extends BaseInsightComponent<CLSCulpritsInsightModel> {
125125
this.dispatchEvent(new EventReferenceClick(event));
126126
}
127127

128-
#renderContent(culprits: Array<string>, worstCluster: Trace.Types.Events.SyntheticLayoutShiftCluster):
129-
LitHtml.LitTemplate {
130-
if (!this.bounds) {
128+
override renderContent(): LitHtml.LitTemplate {
129+
if (!this.model || !this.bounds) {
130+
return LitHtml.nothing;
131+
}
132+
133+
if (!this.model.clusters.length || !this.model.worstCluster) {
134+
return LitHtml.nothing;
135+
}
136+
137+
const worstCluster = this.model.worstCluster;
138+
const culpritsByShift = this.model.shifts;
139+
140+
// TODO: getTopCulprits needs to move to model.
141+
const culprits = this.getTopCulprits(worstCluster, culpritsByShift);
142+
if (culprits.length === 0) {
131143
return LitHtml.nothing;
132144
}
133145

@@ -147,25 +159,6 @@ export class CLSCulprits extends BaseInsightComponent<CLSCulpritsInsightModel> {
147159
</div>`;
148160
// clang-format on
149161
}
150-
151-
override render(): void {
152-
if (!this.model) {
153-
return;
154-
}
155-
156-
const culpritsByShift = this.model.shifts;
157-
if (!this.model.clusters.length || !this.model.worstCluster) {
158-
return;
159-
}
160-
161-
// TODO: getTopCulprits needs to move to model.
162-
const causes = this.getTopCulprits(this.model.worstCluster, culpritsByShift);
163-
164-
const hasCulprits = causes.length > 0;
165-
166-
const output = hasCulprits ? this.#renderContent(causes, this.model.worstCluster) : LitHtml.nothing;
167-
this.renderWithContent(output);
168-
}
169162
}
170163

171164
declare global {

‎front_end/panels/timeline/components/insights/DocumentLatency.ts‎

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ export class DocumentLatency extends BaseInsightComponent<DocumentLatencyInsight
151151
return this.model?.data?.uncompressedResponseBytes ?? null;
152152
}
153153

154-
#renderContent(): LitHtml.LitTemplate {
154+
override renderContent(): LitHtml.LitTemplate {
155155
if (!this.model?.data) {
156156
return LitHtml.nothing;
157157
}
@@ -176,14 +176,6 @@ export class DocumentLatency extends BaseInsightComponent<DocumentLatencyInsight
176176
</div>`;
177177
// clang-format on
178178
}
179-
180-
override render(): void {
181-
if (this.model?.data === undefined) {
182-
return;
183-
}
184-
185-
this.renderWithContent(this.#renderContent());
186-
}
187179
}
188180

189181
declare global {

‎front_end/panels/timeline/components/insights/FontDisplay.ts‎

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ export class FontDisplay extends BaseInsightComponent<FontDisplayInsightModel> {
5454
return this.model?.metricSavings?.FCP ?? null;
5555
}
5656

57-
#renderContent(): LitHtml.LitTemplate {
57+
override renderContent(): LitHtml.LitTemplate {
5858
if (!this.model) {
5959
return LitHtml.nothing;
6060
}
@@ -80,14 +80,6 @@ export class FontDisplay extends BaseInsightComponent<FontDisplayInsightModel> {
8080
</div>`;
8181
// clang-format on
8282
}
83-
84-
override render(): void {
85-
if (!this.model) {
86-
return;
87-
}
88-
89-
this.renderWithContent(this.#renderContent());
90-
}
9183
}
9284

9385
declare global {

‎front_end/panels/timeline/components/insights/ImageDelivery.ts‎

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ export class ImageDelivery extends BaseInsightComponent<ImageDeliveryInsightMode
9494
return rows;
9595
}
9696

97-
#renderContent(): LitHtml.LitTemplate {
97+
override renderContent(): LitHtml.LitTemplate {
9898
if (!this.model) {
9999
return LitHtml.nothing;
100100
}
@@ -139,14 +139,6 @@ export class ImageDelivery extends BaseInsightComponent<ImageDeliveryInsightMode
139139

140140
return html`${sections}`;
141141
}
142-
143-
override render(): void {
144-
if (!this.model) {
145-
return;
146-
}
147-
148-
this.renderWithContent(this.#renderContent());
149-
}
150142
}
151143

152144
declare global {

‎front_end/panels/timeline/components/insights/InteractionToNextPaint.ts‎

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,12 @@ export class InteractionToNextPaint extends BaseInsightComponent<INPInsightModel
9494
];
9595
}
9696

97-
#renderContent(event: Trace.Types.Events.SyntheticInteractionPair): LitHtml.LitTemplate {
97+
override renderContent(): LitHtml.LitTemplate {
98+
const event = this.model?.longestInteractionEvent;
99+
if (!event) {
100+
return LitHtml.nothing;
101+
}
102+
98103
const time = (us: Trace.Types.Timing.MicroSeconds): string =>
99104
i18n.TimeUtilities.millisToString(Platform.Timing.microSecondsToMilliSeconds(us));
100105

@@ -124,14 +129,6 @@ export class InteractionToNextPaint extends BaseInsightComponent<INPInsightModel
124129
</div>`;
125130
// clang-format on
126131
}
127-
128-
override render(): void {
129-
if (!this.model?.longestInteractionEvent) {
130-
return;
131-
}
132-
133-
this.renderWithContent(this.#renderContent(this.model.longestInteractionEvent));
134-
}
135132
}
136133

137134
declare global {

‎front_end/panels/timeline/components/insights/LCPDiscovery.ts‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -165,11 +165,16 @@ export class LCPDiscovery extends BaseInsightComponent<LCPDiscoveryInsightModel>
165165
return getImageData(this.model)?.estimatedSavings ?? null;
166166
}
167167

168-
#renderContent(imageData: LCPImageDiscoveryData): LitHtml.LitTemplate {
168+
override renderContent(): LitHtml.LitTemplate {
169169
if (!this.model) {
170170
return LitHtml.nothing;
171171
}
172172

173+
const imageData = getImageData(this.model);
174+
if (!imageData) {
175+
return LitHtml.nothing;
176+
}
177+
173178
// clang-format off
174179
return html`
175180
<div class="insight-section">
@@ -193,16 +198,6 @@ export class LCPDiscovery extends BaseInsightComponent<LCPDiscoveryInsightModel>
193198
</div>`;
194199
// clang-format on
195200
}
196-
197-
override render(): void {
198-
if (!this.model) {
199-
return;
200-
}
201-
202-
const imageResults = getImageData(this.model);
203-
const output = imageResults ? this.#renderContent(imageResults) : LitHtml.nothing;
204-
this.renderWithContent(output);
205-
}
206201
}
207202

208203
declare global {

‎front_end/panels/timeline/components/insights/LCPPhases.ts‎

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -186,11 +186,13 @@ export class LCPPhases extends BaseInsightComponent<LCPPhasesInsightModel> {
186186
return overlays;
187187
}
188188

189-
#renderContent(phaseData: PhaseData[]): LitHtml.LitTemplate {
189+
override renderContent(): LitHtml.LitTemplate {
190190
if (!this.model) {
191191
return LitHtml.nothing;
192192
}
193193

194+
const phaseData = this.#getPhaseData();
195+
194196
const rows = phaseData.map(({phase, percent}) => {
195197
const section = this.#overlay?.sections.find(section => phase === section.label);
196198
return {
@@ -215,15 +217,6 @@ export class LCPPhases extends BaseInsightComponent<LCPPhasesInsightModel> {
215217
</div>`;
216218
// clang-format on
217219
}
218-
219-
override render(): void {
220-
if (!this.model) {
221-
return;
222-
}
223-
224-
const phaseData = this.#getPhaseData();
225-
this.renderWithContent(this.#renderContent(phaseData));
226-
}
227220
}
228221

229222
declare global {

0 commit comments

Comments
 (0)