Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(image-elements): remove resourceSize property #11733

Merged
merged 7 commits into from
Dec 7, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions lighthouse-core/audits/byte-efficiency/offscreen-images.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ class OffscreenImages extends ByteEfficiencyAudit {
static computeWaste(image, viewportDimensions, networkRecords) {
const networkRecord = networkRecords.find(record => record.url === image.src);
// If we don't know how big it was, we can't really report savings, treat it as passed.
if (!image.resourceSize || !networkRecord) return null;
if (!networkRecord) return null;
// If the image had its loading behavior explicitly controlled already, treat it as passed.
if (image.loading === 'lazy' || image.loading === 'eager') return null;

Expand All @@ -89,7 +89,13 @@ class OffscreenImages extends ByteEfficiencyAudit {
const visiblePixels = this.computeVisiblePixels(image.clientRect, viewportDimensions);
// Treat images with 0 area as if they're offscreen. See https://github.com/GoogleChrome/lighthouse/issues/1914
const wastedRatio = totalPixels === 0 ? 1 : 1 - visiblePixels / totalPixels;
const totalBytes = image.resourceSize;
// Resource size is almost always the right one to be using because of the below:
// transferSize = resourceSize + headers.length
// HOWEVER, there are some cases where an image is compressed again over the network and transfer size
// is smaller (see https://github.com/GoogleChrome/lighthouse/pull/4968).
// Use the min of the two numbers to be safe.
const {resourceSize = 0, transferSize = 0} = networkRecord;
const totalBytes = Math.min(resourceSize, transferSize);
const wastedBytes = Math.round(totalBytes * wastedRatio);

if (!Number.isFinite(wastedRatio)) {
Expand Down
25 changes: 17 additions & 8 deletions lighthouse-core/audits/byte-efficiency/uses-responsive-images.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,13 @@ class UsesResponsiveImages extends ByteEfficiencyAudit {
/**
* @param {LH.Artifacts.ImageElement} image
* @param {LH.Artifacts.ViewportDimensions} ViewportDimensions
* @param {Array<LH.Artifacts.NetworkRequest>} networkRecords
* @return {null|Error|LH.Audit.ByteEfficiencyItem};
*/
static computeWaste(image, ViewportDimensions) {
// Nothing can be done without network info.
if (!image.resourceSize) {
static computeWaste(image, ViewportDimensions, networkRecords) {
const networkRecord = networkRecords.find(record => record.url === image.src);
// Nothing can be done without network info, ignore images without resource size information.
if (!networkRecord) {
return null;
}

Expand Down Expand Up @@ -81,7 +83,13 @@ class UsesResponsiveImages extends ByteEfficiencyAudit {
const url = URL.elideDataURI(image.src);
const actualPixels = image.naturalWidth * image.naturalHeight;
const wastedRatio = 1 - (usedPixels / actualPixels);
const totalBytes = image.resourceSize;
// Resource size is almost always the right one to be using because of the below:
// transferSize = resourceSize + headers.length
// HOWEVER, there are some cases where an image is compressed again over the network and transfer size
// is smaller (see https://github.com/GoogleChrome/lighthouse/pull/4968).
// Use the min of the two numbers to be safe.
const {resourceSize = 0, transferSize = 0} = networkRecord;
const totalBytes = Math.min(resourceSize, transferSize);
const wastedBytes = Math.round(totalBytes * wastedRatio);

if (!Number.isFinite(wastedRatio)) {
Expand All @@ -98,9 +106,10 @@ class UsesResponsiveImages extends ByteEfficiencyAudit {

/**
* @param {LH.Artifacts} artifacts
* @param {Array<LH.Artifacts.NetworkRequest>} networkRecords
* @return {ByteEfficiencyAudit.ByteEfficiencyProduct}
*/
static audit_(artifacts) {
static audit_(artifacts, networkRecords) {
const images = artifacts.ImageElements;
const ViewportDimensions = artifacts.ViewportDimensions;

Expand All @@ -109,15 +118,15 @@ class UsesResponsiveImages extends ByteEfficiencyAudit {
/** @type {Map<string, LH.Audit.ByteEfficiencyItem>} */
const resultsMap = new Map();
for (const image of images) {
// Ignore images without resource size information.
// Give SVG a free pass because creating a "responsive" SVG is of questionable value.
// Ignore CSS images because it's difficult to determine what is a spritesheet,
// and the reward-to-effort ratio for responsive CSS images is quite low https://css-tricks.com/responsive-images-css/.
if (!image.resourceSize || image.mimeType === 'image/svg+xml' || image.isCss) {
if (image.mimeType === 'image/svg+xml' || image.isCss) {
continue;
}

const processed = UsesResponsiveImages.computeWaste(image, ViewportDimensions);
/* eslint-disable max-len */
const processed = UsesResponsiveImages.computeWaste(image, ViewportDimensions, networkRecords);
if (!processed) continue;

if (processed instanceof Error) {
Expand Down
10 changes: 0 additions & 10 deletions lighthouse-core/gather/gatherers/image-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ const FontSize = require('./seo/font-size.js');

/* global window, getElementsInDocument, Image, getNodeDetails, ShadowRoot */


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes like this make me unreasonably happy.

/** @param {Element} element */
/* istanbul ignore next */
function getClientRect(element) {
Expand Down Expand Up @@ -77,7 +76,6 @@ function getHTMLImages(allElements) {
isCss: false,
isPicture,
loading: element.loading,
resourceSize: 0, // this will get overwritten below
usesObjectFit: ['cover', 'contain', 'scale-down', 'none'].includes(
computedStyle.getPropertyValue('object-fit')
),
Expand Down Expand Up @@ -137,7 +135,6 @@ function getCSSImages(allElements) {
style.getPropertyValue('image-rendering')
),
usesSrcSetDensityDescriptor: false,
resourceSize: 0, // this will get overwritten below
// @ts-expect-error - getNodeDetails put into scope via stringification
...getNodeDetails(element),
});
Expand Down Expand Up @@ -332,13 +329,6 @@ class ImageElements extends Gatherer {
// Pull some of our information directly off the network record.
const networkRecord = indexedNetworkRecords[element.src] || {};
element.mimeType = networkRecord.mimeType;
// Resource size is almost always the right one to be using because of the below:
// transferSize = resourceSize + headers.length
// HOWEVER, there are some cases where an image is compressed again over the network and transfer size
// is smaller (see https://github.com/GoogleChrome/lighthouse/pull/4968).
// Use the min of the two numbers to be safe.
const {resourceSize = 0, transferSize = 0} = networkRecord;
Copy link
Collaborator

@connorjclark connorjclark Dec 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@patrickhulce this is still of concern ya?

I'd expect this test to fail in this PR (but it isn't).. https://github.com/GoogleChrome/lighthouse/pull/4968/files#diff-b15e1d5e110701eeaa3cf353d4108a5e61e699ae3afbcdb272c7816b978784f1R137 (although it has since been modified 5674d61#diff-b15e1d5e110701eeaa3cf353d4108a5e61e699ae3afbcdb272c7816b978784f1 ). Seems this case was missed when that test was refactored.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes we should still account for it in the optimization cases Math.min(transferSize, resourceSize) is mostly what we want.

element.resourceSize = Math.min(resourceSize, transferSize);

if (!element.isInShadowDOM && !element.isCss) {
await this.fetchSourceRules(driver, element.devtoolsNodePath, element);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ function generateRecord({
mimeType,
startTime, // DevTools timestamp which is in seconds.
resourceSize: resourceSizeInKb * 1024,
transferSize: resourceSizeInKb * 1024,
};
}

Expand Down Expand Up @@ -408,6 +409,7 @@ describe('OffscreenImages audit', () => {
const recordA = {
url: 'https://example.com/a',
resourceSize: wastedSize,
transferSize: wastedSize,
requestId: 'a',
startTime: 1,
priority: 'High',
Expand All @@ -416,6 +418,7 @@ describe('OffscreenImages audit', () => {
const recordB = {
url: 'https://example.com/b',
resourceSize: wastedSize,
transferSize: wastedSize,
requestId: 'b',
startTime: 2.25,
priority: 'High',
Expand Down Expand Up @@ -461,6 +464,7 @@ describe('OffscreenImages audit', () => {
const recordA = {
url: 'https://example.com/a',
resourceSize: wastedSize,
transferSize: wastedSize,
requestId: 'a',
startTime: 1,
priority: 'High',
Expand All @@ -469,6 +473,7 @@ describe('OffscreenImages audit', () => {
const recordB = {
url: 'https://example.com/b',
resourceSize: wastedSize,
transferSize: wastedSize,
requestId: 'b',
startTime: 1.25,
priority: 'High',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,14 @@ const UsesResponsiveImagesAudit =
const assert = require('assert').strict;

/* eslint-env jest */
function generateRecord(resourceSizeInKb, durationInMs, mimeType = 'image/png') {
function generateRecord(resourceSizeInKb, durationInMs, url = 'https://google.com/logo.png', mimeType = 'image/png') {
return {
mimeType,
resourceSize: resourceSizeInKb * 1024,
transferSize: resourceSizeInKb * 1024,
endTime: durationInMs / 1000,
responseReceivedTime: 0,
url,
};
}

Expand All @@ -26,32 +28,29 @@ function generateSize(width, height, prefix = 'displayed') {
return size;
}

function generateImage(clientSize, naturalSize, networkRecord, src = 'https://google.com/logo.png') {
Object.assign(networkRecord || {}, {url: src});
const image = {src, ...networkRecord};
Object.assign(image, clientSize, naturalSize);
return image;
function generateImage(clientSize, naturalSize, src = 'https://google.com/logo.png') {
return {src, ...clientSize, ...naturalSize};
}

describe('Page uses responsive images', () => {
function testImage(condition, data) {
const description = `identifies when an image is ${condition}`;
const artifacts = {
ViewportDimensions: {
innerWidth: 1000,
innerHeight: 1000,
devicePixelRatio: data.devicePixelRatio || 1,
},
ImageElements: [
generateImage(
generateSize(...data.clientSize),
generateSize(...data.naturalSize, 'natural')
),
],
};
it(description, () => {
const result = UsesResponsiveImagesAudit.audit_({
ViewportDimensions: {
innerWidth: 1000,
innerHeight: 1000,
devicePixelRatio: data.devicePixelRatio || 1,
},
ImageElements: [
generateImage(
generateSize(...data.clientSize),
generateSize(...data.naturalSize, 'natural'),
generateRecord(data.sizeInKb, data.durationInMs || 200)
),
],
});

// eslint-disable-next-line max-len
const result = UsesResponsiveImagesAudit.audit_(artifacts, [generateRecord(data.sizeInKb, data.durationInMs || 200)]);
assert.equal(result.items.length, data.listed ? 1 : 0);
if (data.listed) {
assert.equal(Math.round(result.items[0].wastedBytes / 1024), data.expectedWaste);
Expand Down Expand Up @@ -120,80 +119,84 @@ describe('Page uses responsive images', () => {
null
),
],
});
},
[]
);

assert.equal(auditResult.items.length, 0);
});

it('identifies when images are not wasteful', () => {
const networkRecords = [generateRecord(100, 300, 'https://google.com/logo.png'), generateRecord(90, 500, 'https://google.com/logo2.png'), generateRecord(20, 100, 'data:image/jpeg;base64,foobar')];
const auditResult = UsesResponsiveImagesAudit.audit_({
ViewportDimensions: {innerWidth: 1000, innerHeight: 1000, devicePixelRatio: 2},
ImageElements: [
generateImage(
generateSize(200, 200),
generateSize(450, 450, 'natural'),
generateRecord(100, 300),
'https://google.com/logo.png'
),
generateImage(
generateSize(100, 100),
generateSize(210, 210, 'natural'),
generateRecord(90, 500),
'https://google.com/logo2.png'
),
generateImage(
generateSize(100, 100),
generateSize(80, 80, 'natural'),
generateRecord(20, 100),
'data:image/jpeg;base64,foobar'
),
],
});
},
networkRecords
);

assert.equal(auditResult.items.length, 2);
});

it('ignores vectors', () => {
const urlA = 'https://google.com/logo.svg';
const naturalSizeA = generateSize(450, 450, 'natural');
const recordA = generateRecord(100, 300, 'image/svg+xml');

const image =
{...generateImage(generateSize(10, 10), naturalSizeA, urlA), mimeType: 'image/svg+xml'};
const auditResult = UsesResponsiveImagesAudit.audit_({
ViewportDimensions: {innerWidth: 1000, innerHeight: 1000, devicePixelRatio: 1},
ImageElements: [
generateImage(generateSize(10, 10), naturalSizeA, recordA, urlA),
image,
],
});

},
[generateRecord(100, 300, urlA, 'image/svg+xml')]
);
assert.equal(auditResult.items.length, 0);
});

it('ignores CSS', () => {
const urlA = 'https://google.com/logo.png';
const naturalSizeA = generateSize(450, 450, 'natural');
const recordA = generateRecord(100, 300);

const auditResult = UsesResponsiveImagesAudit.audit_({
ViewportDimensions: {innerWidth: 1000, innerHeight: 1000, devicePixelRatio: 1},
ImageElements: [
{...generateImage(generateSize(10, 10), naturalSizeA, recordA, urlA), isCss: true},
{...generateImage(generateSize(10, 10), naturalSizeA, urlA), isCss: true},
],
});
},
[generateRecord(100, 300, urlA)]
);

assert.equal(auditResult.items.length, 0);
});

it('handles failure', () => {
const urlA = 'https://google.com/logo.png';
const naturalSizeA = generateSize(NaN, 450, 'natural');
const recordA = generateRecord(100, 300);

const auditResult = UsesResponsiveImagesAudit.audit_({
ViewportDimensions: {innerWidth: 1000, innerHeight: 1000, devicePixelRatio: 1},
ImageElements: [
generateImage(generateSize(10, 10), naturalSizeA, recordA, urlA),
generateImage(generateSize(10, 10), naturalSizeA, urlA),
],
});
},
[generateRecord(100, 300, urlA)]
);

assert.equal(auditResult.items.length, 0);
assert.equal(auditResult.warnings.length, 1);
Expand All @@ -202,21 +205,24 @@ describe('Page uses responsive images', () => {
it('de-dupes images', () => {
const urlA = 'https://google.com/logo.png';
const naturalSizeA = generateSize(450, 450, 'natural');
const recordA = generateRecord(100, 300);
const recordA = generateRecord(100, 300, urlA);
const urlB = 'https://google.com/logoB.png';
const naturalSizeB = generateSize(1000, 1000, 'natural');
const recordB = generateRecord(10, 20); // make it small to still test passing
const recordB = generateRecord(10, 20, urlB); // make it small to keep test passing
const networkRecords = [recordA, recordB];

const auditResult = UsesResponsiveImagesAudit.audit_({
ViewportDimensions: {innerWidth: 1000, innerHeight: 1000, devicePixelRatio: 1},
ImageElements: [
generateImage(generateSize(10, 10), naturalSizeA, recordA, urlA),
generateImage(generateSize(450, 450), naturalSizeA, recordA, urlA),
generateImage(generateSize(30, 30), naturalSizeA, recordA, urlA),
generateImage(generateSize(500, 500), naturalSizeB, recordB, urlB),
generateImage(generateSize(100, 100), naturalSizeB, recordB, urlB),
generateImage(generateSize(10, 10), naturalSizeA, urlA),
generateImage(generateSize(450, 450), naturalSizeA, urlA),
generateImage(generateSize(30, 30), naturalSizeA, urlA),
generateImage(generateSize(500, 500), naturalSizeB, urlB),
generateImage(generateSize(100, 100), naturalSizeB, urlB),
],
});
},
networkRecords
);

assert.equal(auditResult.items.length, 1);
assert.equal(auditResult.items[0].wastedPercent, 75, 'correctly computes wastedPercent');
Expand Down
Loading