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 all 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,26 @@ module.exports = [
},
},
{
artifacts: {
WebAppManifest: {
value: {
icons: {
value: [
{value: {src: {value: 'http://localhost:10503/launcher-icon-0-75x.png'}}},
{value: {src: {value: 'http://localhost:10503/launcher-icon-1x.png'}}},
{value: {src: {value: 'http://localhost:10503/launcher-icon-1-5x.png'}}},
{value: {src: {value: 'http://localhost:10503/launcher-icon-2x.png'}}},
{value: {src: {value: 'http://localhost:10503/launcher-icon-3x.png'}}},
],
},
},
},
InstallabilityErrors: {
errors: [
'Downloaded icon was empty or corrupted',
],
},
},
lhr: {
requestedUrl: 'http://localhost:10503/offline-ready.html',
finalUrl: 'http://localhost:10503/offline-ready.html',
Expand Down Expand Up @@ -130,7 +150,8 @@ module.exports = [
scoreDisplayMode: 'notApplicable',
},
'installable-manifest': {
score: 1,
score: 0,
explanation: 'Failures: Manifest icon failed to be fetched.',
},
'splash-screen': {
score: 0,
Expand Down
5 changes: 3 additions & 2 deletions lighthouse-core/audits/installable-manifest.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class InstallableManifest extends MultiCheckAudit {
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
requiredArtifacts: ['URL', 'WebAppManifest'],
requiredArtifacts: ['URL', 'WebAppManifest', 'InstallabilityErrors'],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just got bit by forgetting to add this. #9946 would have saved me time.

};
}

Expand All @@ -73,6 +73,7 @@ class InstallableManifest extends MultiCheckAudit {
'hasStartUrl',
'hasPWADisplayValue',
'hasIconsAtLeast144px',
'fetchesIcon',
];
manifestValues.allChecks
.filter(item => bannerCheckIds.includes(item.id))
Expand All @@ -91,7 +92,7 @@ class InstallableManifest extends MultiCheckAudit {
* @return {Promise<{failures: Array<string>, manifestValues: LH.Artifacts.ManifestValues}>}
*/
static async audit_(artifacts, context) {
const manifestValues = await ManifestValues.request(artifacts.WebAppManifest, context);
const manifestValues = await ManifestValues.request(artifacts, context);
const manifestFailures = InstallableManifest.assessManifest(manifestValues);

return {
Expand Down
10 changes: 5 additions & 5 deletions lighthouse-core/audits/service-worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,19 +95,19 @@ class ServiceWorker extends Audit {
/**
* Returns a failure message if there is no start_url or if the start_url isn't
* contolled by the scopeUrl.
* @param {LH.Artifacts['WebAppManifest']} manifest
* @param {LH.Artifacts['WebAppManifest']} WebAppManifest
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not changing anything else here now, but this naming convention is better imo.

* @param {string} scopeUrl
* @return {string|undefined}
*/
static checkStartUrl(manifest, scopeUrl) {
if (!manifest) {
static checkStartUrl(WebAppManifest, scopeUrl) {
if (!WebAppManifest) {
return str_(UIStrings.explanationNoManifest);
}
if (!manifest.value) {
if (!WebAppManifest.value) {
return str_(UIStrings.explanationBadManifest);
}

const startUrl = manifest.value.start_url.value;
const startUrl = WebAppManifest.value.start_url.value;
if (!startUrl.startsWith(scopeUrl)) {
return str_(UIStrings.explanationBadStartUrl, {startUrl, scopeUrl});
}
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/audits/splash-screen.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class SplashScreen extends MultiCheckAudit {
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
requiredArtifacts: ['WebAppManifest'],
requiredArtifacts: ['WebAppManifest', 'InstallabilityErrors'],
};
}

Expand Down Expand Up @@ -84,7 +84,7 @@ class SplashScreen extends MultiCheckAudit {
/** @type {Array<string>} */
const failures = [];

const manifestValues = await ManifestValues.request(artifacts.WebAppManifest, context);
const manifestValues = await ManifestValues.request(artifacts, context);
SplashScreen.assessManifest(manifestValues, failures);

return {
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/audits/themed-omnibox.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class ThemedOmnibox extends MultiCheckAudit {
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
requiredArtifacts: ['WebAppManifest', 'MetaElements'],
requiredArtifacts: ['WebAppManifest', 'InstallabilityErrors', 'MetaElements'],
};
}

Expand Down Expand Up @@ -93,7 +93,7 @@ class ThemedOmnibox extends MultiCheckAudit {
const failures = [];

const themeColorMeta = artifacts.MetaElements.find(meta => meta.name === 'theme-color');
const manifestValues = await ManifestValues.request(artifacts.WebAppManifest, context);
const manifestValues = await ManifestValues.request(artifacts, context);
ThemedOmnibox.assessManifest(manifestValues, failures);
ThemedOmnibox.assessMetaThemecolor(themeColorMeta, failures);

Expand Down
26 changes: 20 additions & 6 deletions lighthouse-core/computed/manifest-values.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const PWA_DISPLAY_VALUES = ['minimal-ui', 'fullscreen', 'standalone'];
const SUGGESTED_SHORTNAME_LENGTH = 12;

class ManifestValues {
/** @typedef {(val: NonNullable<LH.Artifacts.Manifest['value']>) => boolean} Validator */
/** @typedef {(val: NonNullable<LH.Artifacts.Manifest['value']>, errors: string[]) => boolean} Validator */

/**
* @return {Array<{id: LH.Artifacts.ManifestValueCheckID, failureText: string, validate: Validator}>}
Expand All @@ -39,6 +39,20 @@ class ManifestValues {
validate: manifestValue => icons.doExist(manifestValue) &&
icons.pngSizedAtLeast(512, manifestValue).length > 0,
},
{
id: 'fetchesIcon',
failureText: 'Manifest icon failed to be fetched',
validate: (manifestValue, errors) => {
const failedToFetchIconErrors = [
// kCannotDownloadIconMessage
'Could not download a required icon from the manifest',
// kNoIconAvailableMessage
'Downloaded icon was empty or corrupted',
];
return icons.doExist(manifestValue) &&
!failedToFetchIconErrors.some(error => errors.includes(error));
},
},
{
id: 'hasPWADisplayValue',
failureText: 'Manifest\'s `display` value is not one of: ' + PWA_DISPLAY_VALUES.join(' | '),
Expand Down Expand Up @@ -77,19 +91,19 @@ class ManifestValues {

/**
* Returns results of all manifest checks
* @param {LH.Artifacts['WebAppManifest']} manifest
* @param {Pick<LH.Artifacts, 'WebAppManifest'|'InstallabilityErrors'>} Manifest
* @return {Promise<LH.Artifacts.ManifestValues>}
*/
static async compute_(manifest) {
static async compute_({WebAppManifest, InstallabilityErrors}) {
// if the manifest isn't there or is invalid json, we report that and bail
if (manifest === null) {
if (WebAppManifest === null) {
return {
isParseFailure: true,
parseFailureReason: 'No manifest was fetched',
allChecks: [],
};
}
const manifestValue = manifest.value;
const manifestValue = WebAppManifest.value;
if (manifestValue === undefined) {
return {
isParseFailure: true,
Expand All @@ -103,7 +117,7 @@ class ManifestValues {
return {
id: item.id,
failureText: item.failureText,
passing: item.validate(manifestValue),
passing: item.validate(manifestValue, InstallabilityErrors.errors),
};
});

Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/config/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ const BASE_ARTIFACT_BLANKS = {
NetworkUserAgent: '',
BenchmarkIndex: '',
WebAppManifest: '',
InstallabilityErrors: '',
Stacks: '',
traces: '',
devtoolsLogs: '',
Expand Down
6 changes: 6 additions & 0 deletions lighthouse-core/gather/gather-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,7 @@ class GatherRunner {
NetworkUserAgent: '', // updated later
BenchmarkIndex: 0, // updated later
WebAppManifest: null, // updated later
InstallabilityErrors: {errors: []}, // updated later
Stacks: [], // updated later
traces: {},
devtoolsLogs: {},
Expand Down Expand Up @@ -515,6 +516,11 @@ class GatherRunner {
// Fetch the manifest, if it exists.
baseArtifacts.WebAppManifest = await GatherRunner.getWebAppManifest(passContext);

if (baseArtifacts.WebAppManifest) {
baseArtifacts.InstallabilityErrors =
await passContext.driver.sendCommand('Page.getInstallabilityErrors');
}

baseArtifacts.Stacks = await stacksGatherer(passContext);

// Find the NetworkUserAgent actually used in the devtoolsLogs.
Expand Down
14 changes: 7 additions & 7 deletions lighthouse-core/gather/gatherers/start-url.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ class StartUrl extends Gatherer {
* @return {Promise<LH.Artifacts['StartUrl']>}
*/
async _determineStartUrlAvailability(passContext) {
const manifest = passContext.baseArtifacts.WebAppManifest;
const startUrlInfo = this._readManifestStartUrl(manifest);
const WebAppManifest = passContext.baseArtifacts.WebAppManifest;
const startUrlInfo = this._readManifestStartUrl(WebAppManifest);
if (startUrlInfo.isReadFailure) {
return {statusCode: -1, explanation: startUrlInfo.reason};
}
Expand All @@ -52,12 +52,12 @@ class StartUrl extends Gatherer {

/**
* Read the parsed manifest and return failure reasons or the startUrl
* @param {LH.Artifacts.Manifest|null} manifest
* @param {LH.Artifacts.Manifest|null} WebAppManifest
* @return {{isReadFailure: true, reason: string}|{isReadFailure: false, startUrl: string}}
*/
_readManifestStartUrl(manifest) {
if (!manifest || !manifest.value) {
const detailedMsg = manifest && manifest.warning;
_readManifestStartUrl(WebAppManifest) {
if (!WebAppManifest || !WebAppManifest.value) {
const detailedMsg = WebAppManifest && WebAppManifest.warning;

if (detailedMsg) {
return {isReadFailure: true, reason: `Error fetching web app manifest: ${detailedMsg}.`};
Expand All @@ -67,7 +67,7 @@ class StartUrl extends Gatherer {
}

// Even if the start URL had a parser warning, the browser will still supply a fallback URL.
return {isReadFailure: false, startUrl: manifest.value.start_url.value};
return {isReadFailure: false, startUrl: WebAppManifest.value.start_url.value};
}

/**
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/scripts/build-report-for-autodeployment.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ async function generateErrorLHR() {
NetworkUserAgent: 'Mozilla/5.0 ErrorUserAgent Chrome/66',
BenchmarkIndex: 1000,
WebAppManifest: null,
InstallabilityErrors: {errors: []},
Stacks: [],
settings: defaultSettings,
URL: {
Expand Down
27 changes: 21 additions & 6 deletions lighthouse-core/test/audits/installable-manifest-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ function generateMockArtifacts(src = manifestSrc) {

const clonedArtifacts = JSON.parse(JSON.stringify({
WebAppManifest: exampleManifest,
InstallabilityErrors: {errors: []},
URL: {finalUrl: 'https://example.com'},
}));
return clonedArtifacts;
Expand All @@ -44,8 +45,7 @@ describe('PWA: webapp install banner audit', () => {
});

it('fails with a non-parsable manifest', () => {
const artifacts = generateMockArtifacts();
artifacts.WebAppManifest = manifestParser('{,:}', EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL);
const artifacts = generateMockArtifacts('{,:}');
const context = generateMockAuditContext();
return InstallableManifestAudit.audit(artifacts, context).then(result => {
assert.strictEqual(result.score, 0);
Expand All @@ -54,13 +54,12 @@ describe('PWA: webapp install banner audit', () => {
});

it('fails when an empty manifest is present', () => {
const artifacts = generateMockArtifacts();
artifacts.WebAppManifest = manifestParser('{}', EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL);
const artifacts = generateMockArtifacts('{}');
const context = generateMockAuditContext();
return InstallableManifestAudit.audit(artifacts, context).then(result => {
assert.strictEqual(result.score, 0);
assert.ok(result.explanation);
assert.strictEqual(result.details.items[0].failures.length, 4);
assert.strictEqual(result.details.items[0].failures.length, 5);
});
});

Expand Down Expand Up @@ -132,11 +131,27 @@ describe('PWA: webapp install banner audit', () => {
assert.ok(result.explanation.includes('PNG icon'), result.explanation);

const details = result.details.items[0];
assert.strictEqual(details.failures.length, 1, details.failures);
assert.strictEqual(details.failures.length, 2, details.failures);
assert.strictEqual(details.hasStartUrl, true);
assert.strictEqual(details.hasIconsAtLeast144px, false);
});
});

it('fails if page had no fetchable icons in the manifest', () => {
const artifacts = generateMockArtifacts();
artifacts.InstallabilityErrors.errors.push('Downloaded icon was empty or corrupted');
const context = generateMockAuditContext();

return InstallableManifestAudit.audit(artifacts, context).then(result => {
assert.strictEqual(result.score, 0);
assert.ok(result.explanation.includes('failed to be fetched'), result.explanation);

const details = result.details.items[0];
assert.strictEqual(details.failures.length, 1, details.failures);
assert.strictEqual(details.hasStartUrl, true);
assert.strictEqual(details.hasIconsAtLeast144px, true);
});
});
});

it('fails if icons were present, but no valid PNG present', () => {
Expand Down
16 changes: 7 additions & 9 deletions lighthouse-core/test/audits/service-worker-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,17 +47,17 @@ function createSWArtifact(swOpts) {
* Create a set of artifacts for the ServiceWorker audit.
* @param {Array<{scriptURL: string, status: string, scopeURL?: string}>} swOpts
* @param {string} finalUrl
* @param {{}}} manifestJson WebAppManifest object or null if no manifest desired.
* @param {{}|string|null} manifestJsonOrObject WebAppManifest object or string or null if no manifest desired.
*/
function createArtifacts(swOpts, finalUrl, manifestJson) {
function createArtifacts(swOpts, finalUrl, manifestJsonOrObject) {
const manifestUrl = getBaseDirectory(finalUrl) + 'manifest.json';
let WebAppManifest;
if (manifestJson === null) {
if (manifestJsonOrObject === null) {
WebAppManifest = null;
} else if (typeof manifestJson === 'object') {
WebAppManifest = manifestParser(JSON.stringify(manifestJson), manifestUrl, finalUrl);
} else {
throw new Error('unsupported test manifest format');
const manifestJson = typeof manifestJsonOrObject === 'object' ?
JSON.stringify(manifestJsonOrObject) : manifestJsonOrObject;
WebAppManifest = manifestParser(manifestJson, manifestUrl, finalUrl);
}

return {
Expand Down Expand Up @@ -281,9 +281,7 @@ describe('Offline: service worker audit', () => {
status: 'activated',
scriptURL: 'https://example.com/sw.js',
}];

const artifacts = createArtifacts(swOpts, finalUrl, {});
artifacts.WebAppManifest = manifestParser('{,;}', finalUrl, finalUrl);
const artifacts = createArtifacts(swOpts, finalUrl, '{,;}');

const output = ServiceWorker.audit(artifacts);
assert.strictEqual(output.score, 0);
Expand Down
7 changes: 3 additions & 4 deletions lighthouse-core/test/audits/splash-screen-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ function generateMockArtifacts(src = manifestSrc) {

return {
WebAppManifest: exampleManifest,
InstallabilityErrors: {errors: []},
};
}
function generateMockAuditContext() {
Expand All @@ -45,8 +46,7 @@ describe('PWA: splash screen audit', () => {
});

it('fails with a non-parsable manifest', () => {
const artifacts = generateMockArtifacts();
artifacts.WebAppManifest = manifestParser('{,:}', EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL);
const artifacts = generateMockArtifacts('{,:}');
const context = generateMockAuditContext();
return SplashScreenAudit.audit(artifacts, context).then(result => {
assert.strictEqual(result.score, 0);
Expand All @@ -55,8 +55,7 @@ describe('PWA: splash screen audit', () => {
});

it('fails when an empty manifest is present', () => {
const artifacts = generateMockArtifacts();
artifacts.WebAppManifest = manifestParser('{}', EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL);
const artifacts = generateMockArtifacts('{}');
const context = generateMockAuditContext();
return SplashScreenAudit.audit(artifacts, context).then(result => {
assert.strictEqual(result.score, 0);
Expand Down
Loading