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 4 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
44 changes: 43 additions & 1 deletion lighthouse-core/lib/i18n/locales/en-US.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading