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(installable-manifest): check for fetchable icon #10168

Merged
merged 31 commits into from
Jan 31, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
a1967d2
core(manifest-values): only consider icons that are fetchable
connorjclark Jan 3, 2020
9286bac
pagefns
connorjclark Jan 3, 2020
8e23bff
icons test
connorjclark Jan 3, 2020
a29fd77
icon
connorjclark Jan 3, 2020
dd3785c
trivial
connorjclark Jan 3, 2020
cb119aa
better
connorjclark Jan 9, 2020
86da15f
fix tst
connorjclark Jan 9, 2020
d52a99c
more tests
connorjclark Jan 9, 2020
cff5ce3
better test
connorjclark Jan 9, 2020
dbc1f42
fix more tests
connorjclark Jan 9, 2020
ae67065
fix smoke test
connorjclark Jan 10, 2020
292a3eb
fix tests
connorjclark Jan 10, 2020
c3e6459
Merge remote-tracking branch 'origin/master' into manifest-icon-fetch
connorjclark Jan 15, 2020
3c36dc4
fix
connorjclark Jan 15, 2020
f0467f9
pr
connorjclark Jan 22, 2020
77b155d
Merge remote-tracking branch 'origin/master' into manifest-icon-fetch
connorjclark Jan 22, 2020
50cc495
mock
connorjclark Jan 22, 2020
2351fc6
test
connorjclark Jan 22, 2020
a5db32d
Merge remote-tracking branch 'origin/master' into manifest-icon-fetch
connorjclark Jan 23, 2020
276fa39
fixtest
connorjclark Jan 23, 2020
691ab34
Merge remote-tracking branch 'origin/master' into manifest-icon-fetch
connorjclark Jan 27, 2020
1f9b1d1
InstallabilityErrors artifact
connorjclark Jan 27, 2020
e86fc0b
fix
connorjclark Jan 27, 2020
aaedac3
revert
connorjclark Jan 27, 2020
8ed9b9e
Update lighthouse-core/test/gather/gather-runner-test.js
connorjclark Jan 27, 2020
533e6c1
Merge remote-tracking branch 'origin/master' into manifest-icon-fetch
connorjclark Jan 30, 2020
5078cab
fix
connorjclark Jan 30, 2020
9d63999
fix render test
connorjclark Jan 30, 2020
5b2ca74
Merge remote-tracking branch 'origin/master' into manifest-icon-fetch
connorjclark Jan 31, 2020
88fbc8d
fix
connorjclark Jan 31, 2020
f36ef02
oops
connorjclark Jan 31, 2020
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
Binary file added lighthouse-cli/test/fixtures/launcher-icon-4x.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,22 @@ module.exports = [
},
},
{
artifacts: {
WebAppManifest: {
value: {
icons: {
value: [
{value: {src: {value: 'http://localhost:10503/launcher-icon-0-75x.png'}}, warning: 'Error fetching icon'},
{value: {src: {value: 'http://localhost:10503/launcher-icon-1x.png'}}, warning: 'Error fetching icon'},
{value: {src: {value: 'http://localhost:10503/launcher-icon-1-5x.png'}}, warning: 'Error fetching icon'},
{value: {src: {value: 'http://localhost:10503/launcher-icon-2x.png'}}, warning: 'Error fetching icon'},
{value: {src: {value: 'http://localhost:10503/launcher-icon-3x.png'}}, warning: 'Error fetching icon'},
{value: {src: {value: 'http://localhost:10503/launcher-icon-4x.png'}}, warning: undefined},
],
},
},
},
},
lhr: {
requestedUrl: 'http://localhost:10503/offline-ready.html',
finalUrl: 'http://localhost:10503/offline-ready.html',
Expand Down
20 changes: 19 additions & 1 deletion lighthouse-core/gather/gather-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const NetworkAnalyzer = require('../lib/dependency-graph/simulator/network-analy
const NetworkRecorder = require('../lib/network-recorder.js');
const constants = require('../config/constants.js');
const i18n = require('../lib/i18n/i18n.js');
const pageFunctions = require('../lib/page-functions.js');

/** @typedef {import('../gather/driver.js')} Driver */

Expand Down Expand Up @@ -536,7 +537,24 @@ class GatherRunner {
static async getWebAppManifest(passContext) {
const response = await passContext.driver.getAppManifest();
if (!response) return null;
return manifestParser(response.data, response.url, passContext.url);
const manifest = manifestParser(response.data, response.url, passContext.url);

// Add a warning for icons that aren't fetchable.
if (manifest.value && Array.isArray(manifest.value.icons.value)) {
const iconsToFetch = manifest.value.icons.value
.filter(icon => icon.value.src && icon.value.src.value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

were empty string icons treated as acceptable before but now they won't be? or I suppose they will be valid because we won't try to fetch and put any warning on them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

at this point empty strings were converted to undefined (see parser). later, the icons check will ignore icons whose src does not end in .png.

... however I've just noticed there's a bug if typeHint === 'image/png' and src is undefined. this is so dirty. Can fix by tweaking getFetchableIcons to also check for a nonempty string.

Copy link
Collaborator

Choose a reason for hiding this comment

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

aight sgtm

const fetchPromises = iconsToFetch.map(async icon => {
const expression = `(${pageFunctions.fetchIsOkString})(
${JSON.stringify(icon.value.src.value)}
)`;
if (!await passContext.driver.evaluateAsync(expression, {useIsolation: true})) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Feels like we should make this timeout very short and .catch them to not be a fatal protocol timeout

icon.warning = 'Error fetching icon';
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't love the Promise.all mutating an object, can we make this a map of the icon results?

}
});
await Promise.all(fetchPromises);
}

return manifest;
}

/**
Expand Down
12 changes: 10 additions & 2 deletions lighthouse-core/lib/icons.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,14 @@

const URL = require('./url-shim.js');

/**
* Filter out icons that could not be fetched in the page.
* @param {NonNullable<LH.Artifacts.Manifest['value']>} manifest
*/
function getFetchableIcons(manifest) {
return manifest.icons.value.filter(icon => !icon.warning);
}

/**
* @param {NonNullable<LH.Artifacts.Manifest['value']>} manifest
* @return {boolean} Does the manifest have any icons?
Expand All @@ -15,7 +23,7 @@ function doExist(manifest) {
if (!manifest || !manifest.icons) {
return false;
}
if (manifest.icons.value.length === 0) {
if (getFetchableIcons(manifest).length === 0) {
return false;
}
return true;
Expand All @@ -29,7 +37,7 @@ function doExist(manifest) {
function pngSizedAtLeast(sizeRequirement, manifest) {
// An icon can be provided for a single size, or for multiple sizes.
// To handle both, we flatten all found sizes into a single array.
const iconValues = manifest.icons.value;
const iconValues = getFetchableIcons(manifest);
/** @type {Array<string>} */
const flattenedSizes = [];
iconValues
Expand Down
6 changes: 5 additions & 1 deletion lighthouse-core/lib/manifest-parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ function parseIcon(raw, manifestUrl) {
sizes = {...parsedSizes, value: undefined};
}

return {
const result = {
raw,
value: {
src,
Expand All @@ -281,6 +281,10 @@ function parseIcon(raw, manifestUrl) {
},
warning: undefined,
};
// Hack to make warning be `string|undefined`, even though this function will always
// return undefined. Can be removed if Manifest is explicitly typed (see artifacts.d.ts).
const warning = /** @type {string=} */(undefined);
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
return {...result, warning};
}

/**
Expand Down
9 changes: 8 additions & 1 deletion lighthouse-core/lib/page-functions.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
// @ts-nocheck
'use strict';

/* global window document Node ShadowRoot */
/* global window document Node ShadowRoot fetch */

/**
* Helper functions that are passed by `toString()` by Driver to be evaluated in target page.
Expand Down Expand Up @@ -303,6 +303,12 @@ function getNodeLabel(node) {
return tagName;
}

/** @param {string} url */
async function fetchIsOk(url) {
const response = await fetch(url);
return response.ok;
}

module.exports = {
wrapRuntimeEvalErrorInBrowserString: wrapRuntimeEvalErrorInBrowser.toString(),
registerPerformanceObserverInPageString: registerPerformanceObserverInPage.toString(),
Expand All @@ -318,4 +324,5 @@ module.exports = {
getNodeLabel: getNodeLabel,
getNodeLabelString: getNodeLabel.toString(),
isPositionFixedString: isPositionFixed.toString(),
fetchIsOkString: fetchIsOk.toString(),
};
11 changes: 11 additions & 0 deletions lighthouse-core/test/lib/icons-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,17 @@ describe('Icons helper', () => {
assert.equal(icons.doExist(manifest.value), false);
});

it('fails when a manifest contains no fetchable icons', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

ooo ooo can we get an empty string test in here before we forget about it again :D

const manifestSrc = JSON.stringify({
icons: [{
src: 'icon.png',
}],
});
const manifest = manifestParser(manifestSrc, EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL);
manifest.value.icons.value[0].warning = 'Error fetching icon';
assert.equal(icons.doExist(manifest.value), false);
});

it('succeed when a manifest contains icons', () => {
const manifestSrc = JSON.stringify({
icons: [{
Expand Down