From 87dd64edbd93dfd6237e2fef7617ae79e569803b Mon Sep 17 00:00:00 2001 From: Shane Exterkamp Date: Fri, 12 Oct 2018 11:46:02 -0700 Subject: [PATCH 1/2] Made pngSizedAtLeast match the chrome icon checking. --- lighthouse-core/lib/icons.js | 6 +++--- lighthouse-core/test/lib/icons-test.js | 30 ++++---------------------- 2 files changed, 7 insertions(+), 29 deletions(-) diff --git a/lighthouse-core/lib/icons.js b/lighthouse-core/lib/icons.js index 4687ccf80378..8a9d08e9c13b 100644 --- a/lighthouse-core/lib/icons.js +++ b/lighthouse-core/lib/icons.js @@ -33,13 +33,13 @@ function pngSizedAtLeast(sizeRequirement, manifest) { /** @type {Array} */ const flattenedSizes = []; iconValues + // filter out icons with no typehint + extension is not .png // filter out icons with a typehint that is not 'image/png' - .filter(icon => (!icon.value.type.value) || + .filter(icon => (!icon.value.type.value && (icon.value.src.value && + new URL(icon.value.src.value).pathname.endsWith('.png'))) || (icon.value.type.value && icon.value.type.value === 'image/png')) // filter out icons that are not png - .filter(icon => icon.value.src.value && - new URL(icon.value.src.value).pathname.endsWith('.png')) .forEach(icon => { // check that the icon has a size if (icon.value.sizes.value) { diff --git a/lighthouse-core/test/lib/icons-test.js b/lighthouse-core/test/lib/icons-test.js index 71b862c1b2fc..8a515babb62d 100644 --- a/lighthouse-core/test/lib/icons-test.js +++ b/lighthouse-core/test/lib/icons-test.js @@ -278,7 +278,9 @@ describe('Icons helper', () => { assert.equal(icons.pngSizedAtLeast(192, manifest.value).length, 0); }); - it('fails with an icon that has a png typehint but is not png', () => { + it('succeeds with an icon that has a png typehint but is not png', () => { + // We will believe your typehints until such time that we can fetch the image and decode it + // TODO: fetch images and decode them to check real filetype like in Chrome const manifestSrc = JSON.stringify({ icons: [{ src: 'path/to/image.jpg', @@ -287,7 +289,7 @@ describe('Icons helper', () => { }], }); const manifest = manifestParser(manifestSrc, EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL); - assert.equal(icons.pngSizedAtLeast(192, manifest.value).length, 0); + assert.equal(icons.pngSizedAtLeast(192, manifest.value).length, 1); }); it('succeeds with a png icon that has query params in url', () => { @@ -301,29 +303,5 @@ describe('Icons helper', () => { const manifest = manifestParser(manifestSrc, EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL); assert.equal(icons.pngSizedAtLeast(192, manifest.value).length, 1); }); - - it('fails with a non-png icon that has query params in url', () => { - const manifestSrc = JSON.stringify({ - icons: [{ - src: 'path/to/image.jpg?param=true', - sizes: '200x200', - type: 'image/png', - }], - }); - const manifest = manifestParser(manifestSrc, EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL); - assert.equal(icons.pngSizedAtLeast(192, manifest.value).length, 0); - }); - - it('fails with a non-png icon that has a .png extension in the middle', () => { - const manifestSrc = JSON.stringify({ - icons: [{ - src: 'path/to/image.png.jpg', - sizes: '200x200', - type: 'image/png', - }], - }); - const manifest = manifestParser(manifestSrc, EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL); - assert.equal(icons.pngSizedAtLeast(192, manifest.value).length, 0); - }); }); }); From dccc67e4acf414022b27149ecd7a8061a72b2ca4 Mon Sep 17 00:00:00 2001 From: Shane Exterkamp Date: Fri, 12 Oct 2018 17:50:48 -0700 Subject: [PATCH 2/2] Broke filter into block and re-added tests w/note. --- lighthouse-core/lib/icons.js | 17 ++++++++------ lighthouse-core/test/lib/icons-test.js | 32 ++++++++++++++++++++++---- 2 files changed, 38 insertions(+), 11 deletions(-) diff --git a/lighthouse-core/lib/icons.js b/lighthouse-core/lib/icons.js index 8a9d08e9c13b..548af0d3fd33 100644 --- a/lighthouse-core/lib/icons.js +++ b/lighthouse-core/lib/icons.js @@ -33,13 +33,16 @@ function pngSizedAtLeast(sizeRequirement, manifest) { /** @type {Array} */ const flattenedSizes = []; iconValues - // filter out icons with no typehint + extension is not .png - // filter out icons with a typehint that is not 'image/png' - .filter(icon => (!icon.value.type.value && (icon.value.src.value && - new URL(icon.value.src.value).pathname.endsWith('.png'))) || - (icon.value.type.value && - icon.value.type.value === 'image/png')) - // filter out icons that are not png + .filter(icon => { + const typeHint = icon.value.type.value; + if (typeHint) { + // If a type hint is present, filter out icons that are not 'image/png'. + return typeHint === 'image/png'; + } + // Otherwise, fall back to filtering on the icons' extension. + const src = icon.value.src.value; + return src && new URL(src).pathname.endsWith('.png'); + }) .forEach(icon => { // check that the icon has a size if (icon.value.sizes.value) { diff --git a/lighthouse-core/test/lib/icons-test.js b/lighthouse-core/test/lib/icons-test.js index 8a515babb62d..90a3cfaa876e 100644 --- a/lighthouse-core/test/lib/icons-test.js +++ b/lighthouse-core/test/lib/icons-test.js @@ -278,9 +278,21 @@ describe('Icons helper', () => { assert.equal(icons.pngSizedAtLeast(192, manifest.value).length, 0); }); + it('succeeds with a png icon that has query params in url', () => { + const manifestSrc = JSON.stringify({ + icons: [{ + src: 'path/to/image.png?param=true', + sizes: '200x200', + type: 'image/png', + }], + }); + const manifest = manifestParser(manifestSrc, EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL); + assert.equal(icons.pngSizedAtLeast(192, manifest.value).length, 1); + }); + + // Note on tests below: we will believe your typehints until we can fetch the image and decode it. + // See https://github.com/GoogleChrome/lighthouse/issues/789 it('succeeds with an icon that has a png typehint but is not png', () => { - // We will believe your typehints until such time that we can fetch the image and decode it - // TODO: fetch images and decode them to check real filetype like in Chrome const manifestSrc = JSON.stringify({ icons: [{ src: 'path/to/image.jpg', @@ -292,10 +304,22 @@ describe('Icons helper', () => { assert.equal(icons.pngSizedAtLeast(192, manifest.value).length, 1); }); - it('succeeds with a png icon that has query params in url', () => { + it('succeeds with a non-png icon that has query params in url', () => { const manifestSrc = JSON.stringify({ icons: [{ - src: 'path/to/image.png?param=true', + src: 'path/to/image.jpg?param=true', + sizes: '200x200', + type: 'image/png', + }], + }); + const manifest = manifestParser(manifestSrc, EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL); + assert.equal(icons.pngSizedAtLeast(192, manifest.value).length, 1); + }); + + it('succeeds with a non-png icon that has a .png extension in the middle', () => { + const manifestSrc = JSON.stringify({ + icons: [{ + src: 'path/to/image.png.jpg', sizes: '200x200', type: 'image/png', }],