Skip to content

Commit 39884c0

Browse files
Adam RaineDevtools-frontend LUCI CQ
Adam Raine
authored and
Devtools-frontend LUCI CQ
committed
[RPP] Add insight messages to related insight chips
This adds image specific recommendations for the image delivery insight. In a future patch, these messages will move next to the image as designed in the mocks. For now the insight chips area will be fine. https://screenshot.googleplex.com/9gw3N97GuEz8rLh Bug: 372897377 Change-Id: Ifccb7b4c4f26f779f05cf3130c323ce22601064d Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6065232 Commit-Queue: Adam Raine <[email protected]> Reviewed-by: Connor Clark <[email protected]>
1 parent e46ebf2 commit 39884c0

File tree

9 files changed

+215
-34
lines changed

9 files changed

+215
-34
lines changed

‎front_end/models/trace/insights/ImageDelivery.test.ts‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,10 @@ import {describeWithEnvironment} from '../../../testing/EnvironmentHelpers.js';
66
import {getFirstOrError, getInsightOrError} from '../../../testing/InsightHelpers.js';
77
import {TraceLoader} from '../../../testing/TraceLoader.js';
88

9+
import * as Insights from './insights.js';
10+
11+
const {ImageOptimizationType} = Insights.Models.ImageDelivery;
12+
913
export async function processTrace(testContext: Mocha.Suite|Mocha.Context|null, traceFile: string) {
1014
const {parsedTrace, insights} = await TraceLoader.traceEngine(testContext, traceFile);
1115
if (!insights) {
@@ -43,7 +47,7 @@ describeWithEnvironment('ImageDelivery', function() {
4347
optimizations: [
4448
{
4549
byteSavings: 1057876,
46-
type: 'modern-format-or-compression',
50+
type: ImageOptimizationType.MODERN_FORMAT_OR_COMPRESSION,
4751
},
4852
],
4953
url:
@@ -53,7 +57,7 @@ describeWithEnvironment('ImageDelivery', function() {
5357
optimizations: [
5458
{
5559
byteSavings: 682028,
56-
type: 'video-format',
60+
type: ImageOptimizationType.VIDEO_FORMAT,
5761
},
5862
],
5963
url:
@@ -63,7 +67,7 @@ describeWithEnvironment('ImageDelivery', function() {
6367
optimizations: [
6468
{
6569
byteSavings: 49760,
66-
type: 'compression',
70+
type: ImageOptimizationType.ADJUST_COMPRESSION,
6771
},
6872
],
6973
url:
@@ -73,7 +77,9 @@ describeWithEnvironment('ImageDelivery', function() {
7377
optimizations: [
7478
{
7579
byteSavings: 41421,
76-
type: 'responsive-size',
80+
type: ImageOptimizationType.RESPONSIVE_SIZE,
81+
fileDimensions: {width: 2048, height: 1356},
82+
displayDimensions: {width: 200, height: 132},
7783
},
7884
],
7985
url:
@@ -83,11 +89,13 @@ describeWithEnvironment('ImageDelivery', function() {
8389
optimizations: [
8490
{
8591
byteSavings: 134075,
86-
type: 'modern-format-or-compression',
92+
type: ImageOptimizationType.MODERN_FORMAT_OR_COMPRESSION,
8793
},
8894
{
8995
byteSavings: 162947,
90-
type: 'responsive-size',
96+
type: ImageOptimizationType.RESPONSIVE_SIZE,
97+
fileDimensions: {width: 640, height: 436},
98+
displayDimensions: {width: 200, height: 136},
9199
},
92100
],
93101
url: 'https://onlinepngtools.com/images/examples-onlinepngtools/elephant-hd-quality.png',

‎front_end/models/trace/insights/ImageDelivery.ts‎

Lines changed: 80 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,12 @@ import * as i18n from '../../../core/i18n/i18n.js';
66
import * as Helpers from '../helpers/helpers.js';
77
import type * as Types from '../types/types.js';
88

9-
import {InsightCategory, type InsightModel, type InsightSetContext, type RequiredData} from './types.js';
9+
import {
10+
InsightCategory,
11+
type InsightModel,
12+
type InsightSetContext,
13+
type RequiredData,
14+
} from './types.js';
1015

1116
const UIStrings = {
1217
/**
@@ -18,6 +23,30 @@ const UIStrings = {
1823
*/
1924
description:
2025
'Reducing the download time of images can improve the perceived load time of the page and LCP. [Learn more about optimizing image size](https://developer.chrome.com/docs/lighthouse/performance/uses-optimized-images/)',
26+
/**
27+
* @description Message displayed in a chip explaining that an image file size is large for the # of pixels it has and recommends possible adjustments to improve the image size.
28+
* @example {50 MB} PH1
29+
*/
30+
useCompression: 'Increasing the image compression factor could improve this image\'s download size. (Est {PH1})',
31+
/**
32+
* @description Message displayed in a chip explaining that an image file size is large for the # of pixels it has and recommends possible adjustments to improve the image size.
33+
* @example {50 MB} PH1
34+
*/
35+
useModernFormat:
36+
'Using a modern image format (WebP, AVIF) or increasing the image compression could improve this image\'s download size. (Est {PH1})',
37+
/**
38+
* @description Message displayed in a chip advising the user to use video formats instead of GIFs because videos generally have smaller file sizes.
39+
* @example {50 MB} PH1
40+
*/
41+
useVideoFormat: 'Using video formats instead of GIFs can improve the download size of animated content. (Est {PH1})',
42+
/**
43+
* @description Message displayed in a chip explaining that an image was displayed on the page with dimensions much smaller than the image file dimensions.
44+
* @example {50 MB} PH1
45+
* @example {1000x500} PH2
46+
* @example {100x50} PH3
47+
*/
48+
useResponsiveSize:
49+
'This image file is larger than it needs to be ({PH2}) for its displayed dimensions ({PH3}). Use responsive images to reduce the image download size. (Est {PH1})',
2150
};
2251

2352
const str_ = i18n.i18n.registerUIStrings('models/trace/insights/ImageDelivery.ts', UIStrings);
@@ -52,13 +81,23 @@ export function deps(): ['NetworkRequests', 'Meta', 'ImagePainting'] {
5281
return ['NetworkRequests', 'Meta', 'ImagePainting'];
5382
}
5483

55-
export type ImageOptimizationType = 'modern-format-or-compression'|'compression'|'video-format'|'responsive-size';
56-
57-
export interface ImageOptimization {
58-
type: ImageOptimizationType;
59-
byteSavings: number;
84+
export enum ImageOptimizationType {
85+
ADJUST_COMPRESSION = 'ADJUST_COMPRESSION',
86+
MODERN_FORMAT_OR_COMPRESSION = 'MODERN_FORMAT_OR_COMPRESSION',
87+
VIDEO_FORMAT = 'VIDEO_FORMAT',
88+
RESPONSIVE_SIZE = 'RESPONSIVE_SIZE',
6089
}
6190

91+
export type ImageOptimization = {
92+
type: Exclude<ImageOptimizationType, ImageOptimizationType.RESPONSIVE_SIZE>,
93+
byteSavings: number,
94+
}|{
95+
type: ImageOptimizationType.RESPONSIVE_SIZE,
96+
byteSavings: number,
97+
fileDimensions: {width: number, height: number},
98+
displayDimensions: {width: number, height: number},
99+
};
100+
62101
export interface OptimizableImage {
63102
request: Types.Events.SyntheticNetworkRequest;
64103
optimizations: ImageOptimization[];
@@ -75,6 +114,24 @@ export type ImageDeliveryInsightModel = InsightModel<{
75114
optimizableImages: OptimizableImage[],
76115
}>;
77116

117+
function getOptimizationMessage(optimization: ImageOptimization): string {
118+
const byteSavingsText = i18n.ByteUtilities.bytesToString(optimization.byteSavings);
119+
switch (optimization.type) {
120+
case ImageOptimizationType.ADJUST_COMPRESSION:
121+
return i18nString(UIStrings.useCompression, {PH1: byteSavingsText});
122+
case ImageOptimizationType.MODERN_FORMAT_OR_COMPRESSION:
123+
return i18nString(UIStrings.useModernFormat, {PH1: byteSavingsText});
124+
case ImageOptimizationType.VIDEO_FORMAT:
125+
return i18nString(UIStrings.useVideoFormat, {PH1: byteSavingsText});
126+
case ImageOptimizationType.RESPONSIVE_SIZE:
127+
return i18nString(UIStrings.useResponsiveSize, {
128+
PH1: byteSavingsText,
129+
PH2: `${optimization.fileDimensions.width}x${optimization.fileDimensions.height}`,
130+
PH3: `${optimization.displayDimensions.width}x${optimization.displayDimensions.height}`,
131+
});
132+
}
133+
}
134+
78135
function finalize(partialModel: Omit<ImageDeliveryInsightModel, 'title'|'description'|'category'|'shouldShow'>):
79136
ImageDeliveryInsightModel {
80137
return {
@@ -83,7 +140,8 @@ function finalize(partialModel: Omit<ImageDeliveryInsightModel, 'title'|'descrip
83140
category: InsightCategory.LCP,
84141
shouldShow: partialModel.optimizableImages.length > 0,
85142
...partialModel,
86-
relatedEvents: partialModel.optimizableImages.map(image => image.request),
143+
relatedEvents: new Map(
144+
partialModel.optimizableImages.map(image => [image.request, image.optimizations.map(getOptimizationMessage)])),
87145
};
88146
}
89147

@@ -148,22 +206,33 @@ export function generateInsight(
148206
if (imageBytes > GIF_SIZE_THRESHOLD) {
149207
const percentSavings = estimateGIFPercentSavings(request);
150208
const byteSavings = Math.round(imageBytes * percentSavings);
151-
optimizations.push({type: 'video-format', byteSavings});
209+
optimizations.push({type: ImageOptimizationType.VIDEO_FORMAT, byteSavings});
152210
}
153211
} else if (bytesPerPixel > TARGET_BYTES_PER_PIXEL_AVIF) {
154212
const idealAvifImageSize = Math.round(TARGET_BYTES_PER_PIXEL_AVIF * imageFilePixels);
155213
const byteSavings = imageBytes - idealAvifImageSize;
156214
if (request.args.data.mimeType !== 'image/webp' && request.args.data.mimeType !== 'image/avif') {
157-
optimizations.push({type: 'modern-format-or-compression', byteSavings});
215+
optimizations.push({type: ImageOptimizationType.MODERN_FORMAT_OR_COMPRESSION, byteSavings});
158216
} else {
159-
optimizations.push({type: 'compression', byteSavings});
217+
optimizations.push({type: ImageOptimizationType.ADJUST_COMPRESSION, byteSavings});
160218
}
161219
}
162220

163221
const wastedPixelRatio = 1 - (largestImageDisplayPixels / imageFilePixels);
164222
if (wastedPixelRatio > 0) {
165223
const byteSavings = Math.round(wastedPixelRatio * imageBytes);
166-
optimizations.push({type: 'responsive-size', byteSavings});
224+
optimizations.push({
225+
type: ImageOptimizationType.RESPONSIVE_SIZE,
226+
byteSavings,
227+
fileDimensions: {
228+
width: Math.round(largestImagePaint.args.data.srcWidth),
229+
height: Math.round(largestImagePaint.args.data.srcHeight),
230+
},
231+
displayDimensions: {
232+
width: Math.round(largestImagePaint.args.data.width),
233+
height: Math.round(largestImagePaint.args.data.height),
234+
},
235+
});
167236
}
168237

169238
optimizations = optimizations.filter(optimization => optimization.byteSavings > BYTE_SAVINGS_THRESHOLD);

‎front_end/models/trace/insights/types.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,13 +61,15 @@ export enum InsightCategory {
6161
CLS = 'CLS',
6262
}
6363

64+
export type RelatedEventsMap = Map<Types.Events.Event, string[]>;
65+
6466
export type InsightModel<R extends Record<string, unknown>> = R&{
6567
title: Common.UIString.LocalizedString,
6668
description: Common.UIString.LocalizedString,
6769
category: InsightCategory,
6870
/** True if there is anything of interest to display to the user. */
6971
shouldShow: boolean,
70-
relatedEvents?: Types.Events.Event[],
72+
relatedEvents?: RelatedEventsMap | Types.Events.Event[],
7173
warnings?: InsightWarning[],
7274
metricSavings?: MetricSavings,
7375
};

‎front_end/panels/timeline/TimelineFlameChartView.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -518,8 +518,13 @@ export class TimelineFlameChartView extends
518518
if (Root.Runtime.experiments.isEnabled(Root.Runtime.ExperimentName.TIMELINE_DIM_UNRELATED_EVENTS)) {
519519
// The insight's `relatedEvents` property likely already includes the events associated with
520520
// and overlay, but just in case not, include both arrays. Duplicates are fine.
521-
const relatedEvents = [...entries, ...this.#activeInsight?.model.relatedEvents || []];
522-
this.#dimInsightRelatedEvents(relatedEvents);
521+
let relatedEventsList = this.#activeInsight?.model.relatedEvents;
522+
if (!relatedEventsList) {
523+
relatedEventsList = [];
524+
} else if (relatedEventsList instanceof Map) {
525+
relatedEventsList = Array.from(relatedEventsList.keys());
526+
}
527+
this.#dimInsightRelatedEvents([...entries, ...relatedEventsList]);
523528
}
524529

525530
if (options.updateTraceWindow) {

‎front_end/panels/timeline/TimelinePanel.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1956,11 +1956,19 @@ export class TimelinePanel extends UI.Panel.Panel implements Client, TimelineMod
19561956
if (traceInsightsSets) {
19571957
for (const [insightSetKey, insightSet] of traceInsightsSets) {
19581958
for (const model of Object.values(insightSet.model)) {
1959-
for (const event of model.relatedEvents ?? []) {
1959+
let relatedEvents = model.relatedEvents;
1960+
if (!relatedEvents) {
1961+
relatedEvents = new Map();
1962+
} else if (Array.isArray(relatedEvents)) {
1963+
relatedEvents = new Map(relatedEvents.map(e => [e, []]));
1964+
}
1965+
1966+
for (const [event, messages] of relatedEvents.entries()) {
19601967
const relatedInsights = this.#eventToRelatedInsights.get(event) ?? [];
19611968
this.#eventToRelatedInsights.set(event, relatedInsights);
19621969
relatedInsights.push({
19631970
insightLabel: model.title,
1971+
messages,
19641972
activateInsight: () => {
19651973
this.#setActiveInsight({model, insightSetKey});
19661974
},

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ describeWithEnvironment('RelatedInsightChips', () => {
3333
const relatedInsight: Components.RelatedInsightChips.RelatedInsight = {
3434
insightLabel: 'Some fake insight',
3535
activateInsight: () => {},
36+
messages: [],
3637
};
3738
const relatedMap: Components.RelatedInsightChips.EventToRelatedInsightsMap = new Map();
3839
relatedMap.set(FAKE_EVENT, [relatedInsight]);
@@ -50,11 +51,43 @@ describeWithEnvironment('RelatedInsightChips', () => {
5051
assert.deepEqual(text, ['Some fake insight']);
5152
});
5253

54+
it('renders any insight messages', async () => {
55+
const relatedInsight: Components.RelatedInsightChips.RelatedInsight = {
56+
insightLabel: 'Some fake insight',
57+
activateInsight: () => {},
58+
messages: [
59+
'Message 1',
60+
'Message 2',
61+
],
62+
};
63+
const relatedMap: Components.RelatedInsightChips.EventToRelatedInsightsMap = new Map();
64+
relatedMap.set(FAKE_EVENT, [relatedInsight]);
65+
66+
const component = new Components.RelatedInsightChips.RelatedInsightChips();
67+
renderElementIntoDOM(component);
68+
component.activeEvent = FAKE_EVENT;
69+
component.eventToRelatedInsightsMap = relatedMap;
70+
await coordinator.done();
71+
assert.isOk(component.shadowRoot);
72+
73+
const regularChips = component.shadowRoot.querySelectorAll<HTMLElement>('li.insight-chip');
74+
assert.lengthOf(regularChips, 1);
75+
76+
const optimizationChips = component.shadowRoot.querySelectorAll<HTMLElement>('li.insight-message-box');
77+
assert.lengthOf(optimizationChips, 2);
78+
79+
const text1 = getCleanTextContentFromElements(optimizationChips[0], 'button');
80+
assert.deepEqual(text1, ['Insight: Some fake insight Message 1']);
81+
const text2 = getCleanTextContentFromElements(optimizationChips[1], 'button');
82+
assert.deepEqual(text2, ['Insight: Some fake insight Message 2']);
83+
});
84+
5385
it('calls the activateInsight function when the insight is clicked', async () => {
5486
const activateStub = sinon.stub();
5587
const relatedInsight: Components.RelatedInsightChips.RelatedInsight = {
5688
insightLabel: 'Some fake insight',
5789
activateInsight: () => activateStub(),
90+
messages: [],
5891
};
5992
const relatedMap: Components.RelatedInsightChips.EventToRelatedInsightsMap = new Map();
6093
relatedMap.set(FAKE_EVENT, [relatedInsight]);

‎front_end/panels/timeline/components/RelatedInsightChips.ts‎

Lines changed: 34 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,18 @@ const UIStrings = {
1616
*@description prefix shown next to related insight chips
1717
*/
1818
insightKeyword: 'Insight',
19+
/**
20+
* @description Prefix shown next to related insight chips and containing the insight name.
21+
* @example {Improve image delivery} PH1
22+
*/
23+
insightWithName: 'Insight: {PH1}',
1924
};
2025
const str_ = i18n.i18n.registerUIStrings('panels/timeline/components/RelatedInsightChips.ts', UIStrings);
2126
const i18nString = i18n.i18n.getLocalizedString.bind(undefined, str_);
2227

2328
export interface RelatedInsight {
2429
insightLabel: string;
30+
messages: string[];
2531
activateInsight: () => void;
2632
}
2733
export type EventToRelatedInsightsMap = Map<Trace.Types.Events.Event, RelatedInsight[]>;
@@ -70,21 +76,39 @@ export class RelatedInsightChips extends HTMLElement {
7076
return;
7177
}
7278

73-
const insightChips = relatedInsights.map(insight => {
79+
// TODO: Render insight messages in a separate UX
80+
// Right before insight chips is acceptable for now
81+
const insightMessages = relatedInsights.flatMap(insight => {
82+
return insight.messages.map(message => html`
83+
<li class="insight-message-box">
84+
<button type="button" @click=${this.#insightClick(insight)}>
85+
<div class="insight-label">${i18nString(UIStrings.insightWithName, {
86+
PH1: insight.insightLabel,
87+
})}</div>
88+
<div class="insight-message">${message}</div>
89+
</button>
90+
</li>
91+
`);
92+
});
93+
94+
const insightChips = relatedInsights.flatMap(insight => {
7495
// clang-format off
75-
return html`
76-
<li class="insight-chip">
77-
<button type="button" @click=${this.#insightClick(insight)}>
78-
<span class="keyword">${i18nString(UIStrings.insightKeyword)}</span>
79-
<span class="insight-label">${insight.insightLabel}</span>
80-
</button>
81-
</li>
82-
`;
96+
return [html`
97+
<li class="insight-chip">
98+
<button type="button" @click=${this.#insightClick(insight)}>
99+
<span class="keyword">${i18nString(UIStrings.insightKeyword)}</span>
100+
<span class="insight-label">${insight.insightLabel}</span>
101+
</button>
102+
</li>
103+
`];
83104
// clang-format on
84105
});
85106

86107
// clang-format off
87-
LitHtml.render(html`<ul>${insightChips}</ul>`, this.#shadow, {host: this});
108+
LitHtml.render(html`
109+
<ul>${insightMessages}</ul>
110+
<ul>${insightChips}</ul>
111+
`, this.#shadow, {host: this});
88112
// clang-format on
89113
}
90114
}

0 commit comments

Comments
 (0)