core(installable-manifest): check for fetchable icon #10168

connorjclark · 2020-01-03T01:33:08Z

I think how Lighthouse deals with manifests could be improved, but that's a pretty big project for small gain. This PR does the minimal necessary to fix #789.

patrickhulce

my biggest concern about this is that we could be fetching lots of files very slowing for not a ton of benefit... (especially in WPT-like environments that are throttled outside of our control)

what are thoughts on just fetching the largest icon to check and/or whatever one chrome is most likely to actually use?

patrickhulce · 2020-01-03T03:02:23Z

lighthouse-core/gather/gather-runner.js

+    // 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);


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?

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.

patrickhulce · 2020-01-03T03:03:25Z

lighthouse-core/gather/gather-runner.js

+          ${JSON.stringify(icon.value.src.value)}
+        )`;
+        if (!await passContext.driver.evaluateAsync(expression, {useIsolation: true})) {
+          icon.warning = 'Error fetching icon';


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

patrickhulce · 2020-01-03T03:03:29Z

lighthouse-core/gather/gather-runner.js

+        const expression = `(${pageFunctions.fetchIsOkString})(
+          ${JSON.stringify(icon.value.src.value)}
+        )`;
+        if (!await passContext.driver.evaluateAsync(expression, {useIsolation: true})) {


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

patrickhulce · 2020-01-03T03:07:09Z

lighthouse-core/test/lib/icons-test.js

@@ -36,6 +36,17 @@ describe('Icons helper', () => {
      assert.equal(icons.doExist(manifest.value), false);
    });

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


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

connorjclark · 2020-01-03T21:13:12Z

what are thoughts on just fetching the largest icon to check and/or whatever one chrome is most likely to actually use?

Only attempt the largest, or attempt all above 512px until one passes?

patrickhulce · 2020-01-03T23:52:19Z

Only attempt the largest, or attempt all above 512px until one passes?

Ideally, we'd do whatever browsers are most likely to do across all users. i.e. if Chrome will try the 512 icon first and use it if it works we do the same, try to fetch 512 and stop if it worked

connorjclark · 2020-01-09T21:42:46Z

Here is the installation check. Just looks for any icon >= 144px (with a caveat for maskable icons that #10197 will no doubt need to consider). No fetch here.

More complicated logic is done to determine the best fitting icon. Rather than replicating that, we should just let Chrome do the fetching and sniff for the error it may create. Page.getAppManifest response's .errors should have that signal. (speaking of which .... why do we replicate all of these checks when this errors object exists?)

connorjclark · 2020-01-09T22:57:42Z

oh right, I forgot about this #8995

should be fine to use this protocol for just the signal for failure to fetch an icon.

connorjclark · 2020-01-09T23:07:22Z

unfortunately we just can't tie it back to a specific icon :(

paulirish · 2020-01-27T23:32:03Z

fyi: part 2 of the getInstallabilityErrors -- tweaks to frontend: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2016235

Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>

brendankenny · 2020-01-28T04:51:35Z

I don't have anything fleshed out. I mostly mean that the manual checks we have are not great (we may be missing full coverage). The new protocol work on Page.getInstallabilityErrors will help us here in due time. Ideally we'll be able to lean on that 100% and replace the WebappManifest artifact with just the literal manifest, not all this parsing junk.

The parser code style is different because we forked it from an existing library, and at the time we didn't know what we'd actually need out of it, so the output is just the direct tree instead of linearized errors or whatever. If there are actual bugs to report we should fix them, we should rely on the protocol when we can do so, but in the meantime let's not unnecessarily antagonize those of us that had to do the library forking :)

connorjclark · 2020-01-30T21:17:22Z

lighthouse-core/test/report/html/renderer/report-renderer-test.js

@@ -115,15 +115,18 @@ describe('ReportRenderer', () => {
      const container = renderer._dom._document.body;
      const output = renderer.renderReport(sampleResultsCopy, container);

+      function isPWAGauge(el) {


pwa category is failing for the sample report json now, which means the pwa__wrapper class isn't set. Changed the test to look at text content instead.

ah well I had to update the sample artifacts anyways, but I'm gonna keep this test change.

patrickhulce · 2020-01-31T20:47:33Z

proto/sample_v2_round_trip.json

@@ -2028,7 +2028,7 @@
                        "vulnCount": 2.0
                    }
                ],
-                "summary": {},


why have these changed?

because i forgot to edit them back

i havent update proto ..

patrickhulce · 2020-01-31T20:48:24Z

@connorjclark the assignment was just to reflect our new policy right, you're not waiting on me? I don't see anything objectionable that's new so my approval still stands

core(manifest-values): only consider icons that are fetchable

a1967d2

connorjclark requested review from brendankenny, patrickhulce and paulirish as code owners January 3, 2020 01:33

googlebot added the cla: yes label Jan 3, 2020

pagefns

9286bac

vercel bot deployed to Preview January 3, 2020 01:36 View deployment

icons test

8e23bff

vercel bot deployed to Preview January 3, 2020 01:40 View deployment

icon

a29fd77

vercel bot deployed to Preview January 3, 2020 01:42 View deployment

patrickhulce reviewed Jan 3, 2020

View reviewed changes

trivial

dd3785c

vercel bot deployed to Preview January 3, 2020 21:14 View deployment

better

cb119aa

vercel bot deployed to Preview January 9, 2020 23:23 View deployment

fix tst

86da15f

vercel bot deployed to Preview January 9, 2020 23:28 View deployment

more tests

d52a99c

vercel bot requested a deployment to Preview January 9, 2020 23:35 Pending

vercel bot deployed to Preview January 9, 2020 23:35 View deployment

connorjclark changed the title ~~core(manifest-values): only consider icons that are fetchable~~ core(installable-manifest): check for fetchable icon Jan 9, 2020

connorjclark added 2 commits January 9, 2020 15:46

better test

cff5ce3

fix more tests

dbc1f42

vercel bot deployed to Preview January 9, 2020 23:54 View deployment

patrickhulce assigned connorjclark Jan 27, 2020

patrickhulce added waiting4committer and removed waiting4reviewer labels Jan 27, 2020

paulirish removed request for paulirish and brendankenny January 27, 2020 23:32

Update lighthouse-core/test/gather/gather-runner-test.js

8ed9b9e

Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>

vercel bot deployed to Preview January 27, 2020 23:36 View deployment

connorjclark added 2 commits January 30, 2020 12:56

Merge remote-tracking branch 'origin/master' into manifest-icon-fetch

533e6c1

fix

5078cab

connorjclark requested a review from a team as a code owner January 30, 2020 20:58

vercel bot deployed to Preview January 30, 2020 20:58 View deployment

fix render test

9d63999

vercel bot deployed to Preview January 30, 2020 21:15 View deployment

connorjclark commented Jan 30, 2020

View reviewed changes

connorjclark added 2 commits January 31, 2020 12:09

Merge remote-tracking branch 'origin/master' into manifest-icon-fetch

5b2ca74

fix

88fbc8d

vercel bot deployed to Preview January 31, 2020 20:20 View deployment

connorjclark assigned patrickhulce and unassigned connorjclark Jan 31, 2020

patrickhulce reviewed Jan 31, 2020

View reviewed changes

oops

f36ef02

vercel bot deployed to Preview January 31, 2020 21:03 View deployment

connorjclark added the land-when-ci-is-green label Jan 31, 2020

devtools-bot merged commit 1b52a82 into master Jan 31, 2020

devtools-bot deleted the manifest-icon-fetch branch January 31, 2020 21:39

devtools-bot removed the land-when-ci-is-green label Jan 31, 2020

patrickhulce mentioned this pull request Feb 10, 2020

InstallabilityErrors protocol format changed #10318

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(installable-manifest): check for fetchable icon #10168

core(installable-manifest): check for fetchable icon #10168

connorjclark commented Jan 3, 2020

patrickhulce left a comment

patrickhulce Jan 3, 2020

connorjclark Jan 3, 2020

patrickhulce Jan 3, 2020

patrickhulce Jan 3, 2020

patrickhulce Jan 3, 2020

patrickhulce Jan 3, 2020

connorjclark commented Jan 3, 2020

patrickhulce commented Jan 3, 2020

connorjclark commented Jan 9, 2020 •

edited

Loading

connorjclark commented Jan 9, 2020

connorjclark commented Jan 9, 2020

paulirish commented Jan 27, 2020

brendankenny commented Jan 28, 2020

connorjclark Jan 30, 2020

connorjclark Jan 31, 2020

patrickhulce Jan 31, 2020

connorjclark Jan 31, 2020

connorjclark Jan 31, 2020

patrickhulce commented Jan 31, 2020

core(installable-manifest): check for fetchable icon #10168

core(installable-manifest): check for fetchable icon #10168

Conversation

connorjclark commented Jan 3, 2020

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Jan 3, 2020

patrickhulce commented Jan 3, 2020

connorjclark commented Jan 9, 2020 • edited Loading

connorjclark commented Jan 9, 2020

connorjclark commented Jan 9, 2020

paulirish commented Jan 27, 2020

brendankenny commented Jan 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Jan 31, 2020

connorjclark commented Jan 9, 2020 •

edited

Loading