Closed Bug 1064152 Opened 6 years ago Closed 5 years ago

Properly handle intent:// URIs

Categories

(Firefox for Android Graveyard :: Search Activity, defect, P1)

All
Android
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(1 file)

For example, if you get a google maps result, you can click a link for directions, which launches an intent:// URI. Right now this doesn't even load properly in Fennec, so the experience is busted.
Wes - Were we working on Intent URIs at one point? Might have been security issues, I forget.
Flags: needinfo?(wjohnston)
Yeah. We decided we needed a prompt of some sort before handling them (even if they only match one installed app). We have more machinery in place for something like that now.
Flags: needinfo?(wjohnston)
Well, for the search activity, I think it makes sense to just launch the app chooser (or default to whatever the user set to handle that intent). It makes sense to default to Fennec for regular URLs, but there's no point opening Fennec just to handle an intent URI.

Also, you two should both be asleep.
Looking into this, just launching an intent with these intent:// URIs isn't working. Looking more closely at these URIs, it looks like they include some meta data about launching the intent:

intent://maps.google.com/maps?um=1&ie=UTF-8&fb=1&sll=52.54114,13.44009&sspn=5.146615,11.2856986&q=Prenzlauer+Berg,+Germany&entry=s#Intent;scheme=http;package=com.google.android.apps.maps;end

I think this is actually something that's just handled by the WebView, so if we just avoid loading an intent at all in this case, the WebView will take care of it for us.

I found that if I get rid of the WebViewClient, this is the case. However, if change our link-intercepting code to not launch a new intent for an intent:// URI, it creates a page load error in the WebView. So maybe we do need to add special code to handle this ourselves.

I have a WIP here: https://github.com/mozilla/fennec-search/pull/84

I took this opportunity to do some refactoring to use shouldOverrideUrlLoading instead of canceling the page load in onPageStarted, which seems like the more correct thing to do. I was hoping that would fix this problem, but it didnt :(
wesj, do you have any ideas what we should do here? Do you know if there's a simple way to parse these URIs to actually launch the right intent?
Flags: needinfo?(wjohnston)
You github link shows nothing to me. The Intent class has a built in way to parse these.
http://developer.android.com/reference/android/content/Intent.html#parseUri%28java.lang.String,%20int%29
Flags: needinfo?(wjohnston)
So there are really two issues here.

The first, more critical issue, is that we aren't properly parsing intent URIs, so Fennec is just failing to do anything useful when we try to load them. Using Intent.parseUri solves this problem.

The second, less important issue, is that I think we should honor the package name set in the intent URI, rather than forcing everything to open in Fennec. The maps URL I liked to above, for example, will open fine in Fennec if we force it to, but I think it's nicer to open it in the maps app since that's what it requested.

I also decided to add a different telemetry probe for this case, since this is different from us loading a URL in the browser. It would also be interesting to know how often users actually hit this. It might be a lot if they're using Google.

Asking mfinkle for feedback on the probe.
Attachment #8492366 - Flags: review?(wjohnston)
Attachment #8492366 - Flags: feedback?(mark.finkle)
Comment on attachment 8492366 [details] [diff] [review]
Properly handle intent:// URIs

Probe looks fine. (LAUNCH, INTENT, "search-result") is unique enough for us to know where it originated from even though the Session might not be set to SEARCH at the time the probe fires.
Attachment #8492366 - Flags: feedback?(mark.finkle) → feedback+
Summary: Don't open intent:// URIs in Fennec → Properly handle intent:// URIs
Comment on attachment 8492366 [details] [diff] [review]
Properly handle intent:// URIs

Review of attachment 8492366 [details] [diff] [review]:
-----------------------------------------------------------------

Seems neat. A couple nits.

::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
@@ +95,3 @@
>  
> +        @Override
> +        public boolean shouldOverrideUrlLoading(WebView view, String url) {

Nothing you can do, but I kinda hate this method name.

@@ +108,5 @@
> +                // If the intent URI didn't specify a package, open this in Fennec.
> +                if (i.getPackage() == null) {
> +                    i.setClassName(AppConstants.ANDROID_PACKAGE_NAME, AppConstants.BROWSER_INTENT_CLASS_NAME);
> +                    Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL,
> +                            TelemetryContract.Method.CONTENT, "search-result");

Just curious, if you call startActivity in here and return true only in this if block (i.e. if we're not forcing this to open in Fennec, just let the WebView handle it), do things work. Is there any reason to do this over that?

@@ +117,5 @@
>  
> +                startActivity(i);
> +                return true;
> +            } catch (URISyntaxException e) {
> +                Log.e(LOG_TAG, "Error parsing intent URI: " + url, e);

Careful showing urls in logcat. Can be a security breach. Does this catch need to wrap so much code? I guess the exception is specific so it won't leak out. Up to you I guess :)

@@ +118,5 @@
> +                startActivity(i);
> +                return true;
> +            } catch (URISyntaxException e) {
> +                Log.e(LOG_TAG, "Error parsing intent URI: " + url, e);
> +                return false;

I'd move this return outside the catch.
Attachment #8492366 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #9)

> @@ +108,5 @@
> > +                // If the intent URI didn't specify a package, open this in Fennec.
> > +                if (i.getPackage() == null) {
> > +                    i.setClassName(AppConstants.ANDROID_PACKAGE_NAME, AppConstants.BROWSER_INTENT_CLASS_NAME);
> > +                    Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL,
> > +                            TelemetryContract.Method.CONTENT, "search-result");
> 
> Just curious, if you call startActivity in here and return true only in this
> if block (i.e. if we're not forcing this to open in Fennec, just let the
> WebView handle it), do things work. Is there any reason to do this over that?

Just tried, and unfortunately no, the WebView gives an error about how it can't load the webpage at intent://...

> @@ +117,5 @@
> >  
> > +                startActivity(i);
> > +                return true;
> > +            } catch (URISyntaxException e) {
> > +                Log.e(LOG_TAG, "Error parsing intent URI: " + url, e);
> 
> Careful showing urls in logcat. Can be a security breach. Does this catch
> need to wrap so much code? I guess the exception is specific so it won't
> leak out. Up to you I guess :)

Oh, good point, I'll remove the URL. I thought it would help with debugging if we ever hit problems, but it's not worth it for the privacy implications.

And the catch wraps that much code since the Intent.parseUri call is what throws the exception. I suppose I could declare the variable above the try/catch, then return early if it doesn't get set, but I feel like I like keeping the logic all together like this.
https://hg.mozilla.org/mozilla-central/rev/05645d479d89
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.