core(manifest-parser): handle blob manifests #9088
Conversation
| @@ -151,7 +151,7 @@ function parseStartUrl(jsonInput, manifestUrl, documentUrl) { | |||
| return { | |||
| raw, | |||
| value: documentUrl, | |||
| warning: 'ERROR: invalid start_url relative to ${manifestUrl}', | |||
| warning: `ERROR: invalid start_url relative to ${manifestUrl}`, | |||
There was a problem hiding this comment.
lol, that's certainly one big problem with testing via assert.ok(parsedUrl.warning)...
| manifestUrlWarning = `WARNING: manifest URL not available over a valid network protocol`; | ||
| } | ||
| } catch (_) { | ||
| manifestUrlWarning = `ERROR: invalid manifest URL ${manifestUrl}`; |
There was a problem hiding this comment.
if the url was completely invalid, it seems like this should be throwing, a la the undefined check at the top?
(in theory it really should never happen since something was fetched in order to get here)
There was a problem hiding this comment.
hm, yeah I agree in principle and it should never happen but seeing as it tanks the entire LH run, that seems like a bad outcome and it was reasonable to warn when the rest of the manifest can still be parsed?
There was a problem hiding this comment.
hm, yeah I agree in principle and it should never happen but seeing as it tanks the entire LH run, that seems like a bad outcome and it was reasonable to warn when the rest of the manifest can still be parsed?
arg, very good point. I forgot it's not in a gatherer anymore.
I can't find anywhere that we surface the warning if value is defined, but we can think about that with the other manifest error stuff.
|
|
||
| it('should warn on valid but non-HTTPS manifest parser URL', function() { | ||
| const parsedManifest = manifestParser('{}', EXAMPLE_MANIFEST_BLOB_URL, EXAMPLE_DOC_URL); | ||
| assert.ok(parsedManifest.warning); |
There was a problem hiding this comment.
we should probably learn our lesson and do some kind of real assertion :)
There was a problem hiding this comment.
heh, alright alright
| const parsedManifest = manifestParser(manifestSrc, EXAMPLE_MANIFEST_BLOB_URL, | ||
| EXAMPLE_DOC_URL); | ||
| const icons = parsedManifest.value.icons; | ||
| assert.equal(icons.value.length, 0); |
Co-Authored-By: Brendan Kenny <[email protected]>
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
Summary
A manifest can be fetched over a
blob:URL but all of its URLs will be invalid. Handle the case where we have invalid URLs and add a toplevel manifest warning when it's blobyRelated Issues/PRs
closes #9087