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(fr): convert image-elements gatherer #12474

Merged
merged 17 commits into from
May 17, 2021
3 changes: 3 additions & 0 deletions lighthouse-core/fraggle-rock/config/default-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const artifacts = {
FormElements: '',
GlobalListeners: '',
IFrameElements: '',
ImageElements: '',
InstallabilityErrors: '',
JsUsage: '',
LinkElements: '',
Expand Down Expand Up @@ -68,6 +69,7 @@ const defaultConfig = {
{id: artifacts.FormElements, gatherer: 'form-elements'},
{id: artifacts.GlobalListeners, gatherer: 'global-listeners'},
{id: artifacts.IFrameElements, gatherer: 'iframe-elements'},
{id: artifacts.ImageElements, gatherer: 'image-elements'},
{id: artifacts.InstallabilityErrors, gatherer: 'installability-errors'},
{id: artifacts.JsUsage, gatherer: 'js-usage'},
{id: artifacts.LinkElements, gatherer: 'link-elements'},
Expand Down Expand Up @@ -109,6 +111,7 @@ const defaultConfig = {
artifacts.FormElements,
artifacts.GlobalListeners,
artifacts.IFrameElements,
artifacts.ImageElements,
artifacts.InstallabilityErrors,
artifacts.JsUsage,
artifacts.LinkElements,
Expand Down
139 changes: 92 additions & 47 deletions lighthouse-core/gather/gatherers/image-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,11 @@
'use strict';

const log = require('lighthouse-logger');
const Gatherer = require('./gatherer.js');
const FRGatherer = require('../../fraggle-rock/gather/base-gatherer.js');
const pageFunctions = require('../../lib/page-functions.js');
const Driver = require('../driver.js'); // eslint-disable-line no-unused-vars
const DevtoolsLog = require('./devtools-log.js');
const FontSize = require('./seo/font-size.js');
const NetworkRecords = require('../../computed/network-records.js');

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

Expand Down Expand Up @@ -221,26 +222,33 @@ function getEffectiveSizingRule({attributesStyle, inlineStyle, matchedCSSRules},
return null;
}

class ImageElements extends Gatherer {
class ImageElements extends FRGatherer {
/** @type {LH.Gatherer.GathererMeta<'DevtoolsLog'>} */
meta = {
supportedModes: ['navigation'],
dependencies: {DevtoolsLog: DevtoolsLog.symbol},
};

constructor() {
super();
/** @type {Map<string, {naturalWidth: number, naturalHeight: number}>} */
this._naturalSizeCache = new Map();
}

/**
* @param {Driver} driver
* @param {LH.Gatherer.FRTransitionalDriver} driver
* @param {LH.Artifacts.ImageElement} element
*/
async fetchElementWithSizeInformation(driver, element) {
const url = element.src;
if (this._naturalSizeCache.has(url)) {
Object.assign(element, this._naturalSizeCache.get(url));
return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

👀

}

try {
// We don't want this to take forever, 250ms should be enough for images that are cached
driver.setNextProtocolTimeout(250);
driver.defaultSession.setNextProtocolTimeout(250);
const size = await driver.executionContext.evaluate(determineNaturalSize, {
args: [url],
});
Expand All @@ -256,18 +264,18 @@ class ImageElements extends Gatherer {
* Images might be sized via CSS. In order to compute unsized-images failures, we need to collect
* matched CSS rules to see if this is the case.
* @url http://go/dwoqq (googlers only)
* @param {Driver} driver
* @param {LH.Gatherer.FRProtocolSession} session
* @param {string} devtoolsNodePath
* @param {LH.Artifacts.ImageElement} element
*/
async fetchSourceRules(driver, devtoolsNodePath, element) {
async fetchSourceRules(session, devtoolsNodePath, element) {
try {
const {nodeId} = await driver.sendCommand('DOM.pushNodeByPathToFrontend', {
const {nodeId} = await session.sendCommand('DOM.pushNodeByPathToFrontend', {
path: devtoolsNodePath,
});
if (!nodeId) return;

const matchedRules = await driver.sendCommand('CSS.getMatchedStylesForNode', {
const matchedRules = await session.sendCommand('CSS.getMatchedStylesForNode', {
nodeId: nodeId,
});
const width = getEffectiveSizingRule(matchedRules, 'width');
Expand All @@ -284,13 +292,10 @@ class ImageElements extends Gatherer {
}

/**
* @param {LH.Gatherer.PassContext} passContext
* @param {LH.Gatherer.LoadData} loadData
* @return {Promise<LH.Artifacts['ImageElements']>}
* @param {LH.Artifacts.NetworkRequest[]} networkRecords
*/
async afterPass(passContext, loadData) {
const driver = passContext.driver;
const indexedNetworkRecords = loadData.networkRecords.reduce((map, record) => {
indexNetworkRecords(networkRecords) {
return networkRecords.reduce((map, record) => {
// An image response in newer formats is sometimes incorrectly marked as "application/octet-stream",
// so respect the extension too.
const isImage = /^image/.test(record.mimeType) || /\.(avif|webp)$/i.test(record.url);
Expand All @@ -301,43 +306,25 @@ class ImageElements extends Gatherer {
}

return map;
}, /** @type {Object<string, LH.Artifacts.NetworkRequest>} */ ({}));

const elements = await driver.executionContext.evaluate(collectImageElementInfo, {
args: [],
deps: [
pageFunctions.getElementsInDocumentString,
pageFunctions.getBoundingClientRectString,
pageFunctions.getNodeDetailsString,
getClientRect,
getPosition,
getHTMLImages,
getCSSImages,
],
});

await Promise.all([
driver.sendCommand('DOM.enable'),
driver.sendCommand('CSS.enable'),
driver.sendCommand('DOM.getDocument', {depth: -1, pierce: true}),
]);

// Sort (in-place) as largest images descending
elements.sort((a, b) => {
const aRecord = indexedNetworkRecords[a.src] || {};
const bRecord = indexedNetworkRecords[b.src] || {};
return bRecord.resourceSize - aRecord.resourceSize;
});
}, /** @type {Record<string, LH.Artifacts.NetworkRequest>} */ ({}));
}

/**
*
* @param {LH.Gatherer.FRTransitionalDriver} driver
* @param {LH.Artifacts.ImageElement[]} elements
* @param {Record<string, LH.Artifacts.NetworkRequest>} indexedNetworkRecords
*/
async collectExtraDetails(driver, elements, indexedNetworkRecords) {
// Don't do more than 5s of this expensive devtools protocol work. See #11289
let reachedGatheringBudget = false;
setTimeout(_ => (reachedGatheringBudget = true), 5000);
let skippedCount = 0;

for (const element of elements) {
// Pull some of our information directly off the network record.
const networkRecord = indexedNetworkRecords[element.src] || {};
element.mimeType = networkRecord.mimeType;
const networkRecord = indexedNetworkRecords[element.src];
element.mimeType = networkRecord && networkRecord.mimeType;

if (reachedGatheringBudget) {
skippedCount++;
Expand All @@ -346,7 +333,7 @@ class ImageElements extends Gatherer {

// Need source rules to determine if sized via CSS (for unsized-images).
if (!element.isInShadowDOM && !element.isCss) {
await this.fetchSourceRules(driver, element.node.devtoolsNodePath, element);
await this.fetchSourceRules(driver.defaultSession, element.node.devtoolsNodePath, element);
}
// Images within `picture` behave strangely and natural size information isn't accurate,
// CSS images have no natural size information at all. Try to get the actual size if we can.
Expand All @@ -359,14 +346,72 @@ class ImageElements extends Gatherer {
if (reachedGatheringBudget) {
log.warn('ImageElements', `Reached gathering budget of 5s. Skipped extra details for ${skippedCount}/${elements.length}`); // eslint-disable-line max-len
}
}

/**
* @param {LH.Gatherer.FRTransitionalContext} context
* @param {LH.Artifacts.NetworkRequest[]} networkRecords
* @return {Promise<LH.Artifacts['ImageElements']>}
*/
async _getArtifact(context, networkRecords) {
const session = context.driver.defaultSession;
const executionContext = context.driver.executionContext;
const indexedNetworkRecords = this.indexNetworkRecords(networkRecords);

const elements = await executionContext.evaluate(collectImageElementInfo, {
args: [],
deps: [
pageFunctions.getElementsInDocumentString,
pageFunctions.getBoundingClientRectString,
pageFunctions.getNodeDetailsString,
getClientRect,
getPosition,
getHTMLImages,
getCSSImages,
],
});

await Promise.all([
driver.sendCommand('DOM.disable'),
driver.sendCommand('CSS.disable'),
session.sendCommand('DOM.enable'),
session.sendCommand('CSS.enable'),
session.sendCommand('DOM.getDocument', {depth: -1, pierce: true}),
]);

// Sort (in-place) as largest images descending.
elements.sort((a, b) => {
const aRecord = indexedNetworkRecords[a.src] || {};
const bRecord = indexedNetworkRecords[b.src] || {};
return bRecord.resourceSize - aRecord.resourceSize;
});

await this.collectExtraDetails(context.driver, elements, indexedNetworkRecords);

await Promise.all([
session.sendCommand('DOM.disable'),
session.sendCommand('CSS.disable'),
]);

return elements;
}

/**
* @param {LH.Gatherer.FRTransitionalContext<'DevtoolsLog'>} context
* @return {Promise<LH.Artifacts['ImageElements']>}
*/
async getArtifact(context) {
const devtoolsLog = context.dependencies.DevtoolsLog;
const networkRecords = await NetworkRecords.request(devtoolsLog, context);
return this._getArtifact(context, networkRecords);
}

/**
* @param {LH.Gatherer.PassContext} passContext
* @param {LH.Gatherer.LoadData} loadData
* @return {Promise<LH.Artifacts['ImageElements']>}
*/
async afterPass(passContext, loadData) {
return this._getArtifact({...passContext, dependencies: {}}, loadData.networkRecords);
}
}

module.exports = ImageElements;
2 changes: 1 addition & 1 deletion lighthouse-core/test/fraggle-rock/api-test-pptr.js
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ describe('Fraggle Rock API', () => {
const {lhr} = result;
const {auditResults, failedAudits, erroredAudits} = getAuditsBreakdown(lhr);
// TODO(FR-COMPAT): This assertion can be removed when full compatibility is reached.
expect(auditResults.length).toMatchInlineSnapshot(`107`);
expect(auditResults.length).toMatchInlineSnapshot(`112`);
expect(erroredAudits).toHaveLength(0);

const failedAuditIds = failedAudits.map(audit => audit.id);
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/test/fraggle-rock/config/config-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ describe('Fraggle Rock Config', () => {
});

it('should throw on invalid artifact definitions', () => {
const configJson = {artifacts: [{id: 'ImageElements', gatherer: 'image-elements'}]};
expect(() => initializeConfig(configJson, {gatherMode})).toThrow(/ImageElements gatherer/);
const configJson = {artifacts: [{id: 'FullPageScreenshot', gatherer: 'full-page-screenshot'}]};
expect(() => initializeConfig(configJson, {gatherMode})).toThrow(/FullPageScreenshot gatherer/);
});

it('should resolve navigation definitions', () => {
Expand Down
Loading