-
Notifications
You must be signed in to change notification settings - Fork 175
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
Protocol Handler support #910
base: main
Are you sure you want to change the base?
Conversation
4e24ed5
to
6b02d3c
Compare
@@ -57,6 +66,15 @@ protected Uri getLaunchingUrl() { | |||
<%= code %> | |||
<% } %> | |||
|
|||
/* Protocol Handler support */ | |||
String scheme = uri.getScheme(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about moving these to android browser helper to allow PWAs that are not generated with bubblewrap to have protocol handlers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this, but I'm not sure how to pass the map of handlers and the launchUrl to ABH in a way that will work with apps that use ABH but are not generated with bubblewrap... Any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checked package manager and seems like we can't query Activity's intent filters, so the only way to get them is probably protected Map<String, String> getProtocolHandlers()
that is exposed from ABH and change getLaunchingUrl()
implementation to handle protocols.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to do slightly better since that's a library... I think I have an idea but let's keep this here for now while we're still working on the TS side.
const protocolHandlersMap = new Map<string, string>(); | ||
for (const handler of oldTwaManifest.protocolHandlers ?? []) { | ||
protocolHandlersMap.set(handler.protocol, handler.url); | ||
} | ||
if (!(fieldsToIgnore.includes('protocol_handlers'))) { | ||
for (const handler of webManifest.protocol_handlers ?? []) { | ||
protocolHandlersMap.set(handler.protocol, handler.url); | ||
}; | ||
} | ||
const protocolHandlers = Array.from(protocolHandlersMap.entries()).map(([protocol, url]) => { | ||
return {protocol, url} as ProtocolHandler; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to add the list of allowed protocols in the protocol_handlers entry. Also the format of the url should be verified:
https://developer.chrome.com/docs/web-platform/best-practices/url-protocol-handler#how_to_use_url_protocol_handler_registration_for_pwas
6b02d3c
to
79eef5b
Compare
if (mProtocolHandlers.containsKey(scheme)) { | ||
String target = uri.toString().replace(scheme + "://", ""); | ||
String format = mProtocolHandlers.get(scheme); | ||
String baseUrl = getString(R.string.launchUrl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
baseUrl should already be prepended to format during manifest processing
No description provided.