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(installability-errors): add url scheme error #13846

Merged
merged 3 commits into from
Apr 13, 2022
Merged

Conversation

adamraine
Copy link
Member

No description provided.

@adamraine adamraine requested a review from a team as a code owner April 12, 2022 16:00
@adamraine adamraine requested review from connorjclark and removed request for a team April 12, 2022 16:00
@brendankenny brendankenny mentioned this pull request Apr 12, 2022
3 tasks
@@ -97,6 +97,8 @@ const UIStrings = {
'protocol-timeout': `Lighthouse could not determine if there was a service worker. Please try with a newer version of Chrome.`,
/** Message logged when the web app has been uninstalled o desktop, signalling that the install banner state is being reset. */
'pipeline-restarted': 'PWA has been uninstalled and installability checks resetting.',
/** Error message explaining that URL of the manifest uses a scheme that is not supported on Android. */
'scheme-not-supported-for-webapk': 'The manifest URL scheme is not supported on Android.',
Copy link
Member

Choose a reason for hiding this comment

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

it would be kind of nice to have `The manifest URL scheme (${'urlScheme}') is not supported on Android.` though, and replace it with http/blob/data or whatever. The audit already uses the manifest url, so maybe not too annoying to include?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we would need to handle the case where the manifest URL is null. It would probably be confusing if we use null as the URL scheme in the string, and I don't want to maintain two different versions.

We could suggest including the URL scheme as an argument on the installability error for value0.

Copy link
Member

Choose a reason for hiding this comment

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

from my reading of app_banner_manager_android.cc that shouldn't be possible, so it seems ok to just fall back to null to keep the type checker happy?

We could suggest including the URL scheme as an argument on the installability error for value0

I think the issue is how likely is that to happen?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry I was referring to our local var:

const manifestUrl = artifacts.WebAppManifest ? artifacts.WebAppManifest.url : null;

The WebAppManifest will be null if there was no manifest data or a PROTOCOL_TIMEOUT occurred.

@brendankenny
Copy link
Member

we can ignore the DevTools smoke 2 and land this first

@adamraine adamraine merged commit 9ca3d1c into master Apr 13, 2022
@adamraine adamraine deleted the url-scheme-error branch April 13, 2022 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants