1182140 - Split out Intent handling code flow, or add flag, for links from web content (if possible :d)

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

We need to add Category.BROWSABLE to all links from web content, but not our other intents. Considering how the code is currently, we add BROWSABLE to all of our intents and hackily exclude certain schemes (e.g. bug 1100100).

It'd be great to split out this code path, or add a flag, so we can properly add the BROWSABLE category to the appropriate intents.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Actually, let's keep all the Intent handling non-sense in this bug:

(via Michael Comella (:mcomella) from bug 1175532 comment #12)
So here's a fun one [1]:

1215     private static Intent getOpenURIIntentInner(final Context context,  final String targetURI,
1216             final String mimeType, final String action, final String title) {
1217 
1218         if (action.equalsIgnoreCase(Intent.ACTION_SEND)) {
1219             Intent shareIntent = getShareIntent(context, targetURI, mimeType, title);
1220             return Intent.createChooser(shareIntent,
1221                                         context.getResources().getString(R.string.share_title)); 
1222         }

getShareIntent sets the intent's data field to the mimeType argument. However, without certain flags specified, Intent.createChooser does not copy that field into the new intent. To be correct, I manually inserted the mimeType ("text/plain") into the returned Intent and we don't match any applications.

Wes, it seems the use of explicit mimeTypes is very inconsistent - do you know when they're used explicitly and implicitly? So far it seems "file" schemes have used mimeTypes and nothing else has.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java?rev=3527e6013de9#1219

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Note: bug 1175532 will add another work-around to the Category.BROWSABLE problem.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #1)
> getShareIntent sets the intent's data field to the mimeType argument.
> However, without certain flags specified, Intent.createChooser does not copy
> that field into the new intent. To be correct, I manually inserted the
> mimeType ("text/plain") into the returned Intent and we don't match any
> applications.

Wes pointed out in bug 1173200 comment 18 that we can use multiple Intents (and thus an implicit and explicit mimeType) to form the request:

Alternatively, you might run getHandlersForIntent and then check the one with the best results. If they both match different things, you could even explicitly show a chooser with both intents: http://developer.android.com/reference/android/content/Intent.html#EXTRA_INITIAL_INTENTS

We do that in the FilePicker code for instance: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/FilePicker.java#201

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

After realizing the menu bar does not have CATEGORY_BROWSABLE [1], adding it and finding out that nothing was matched anymore, I re-read into the documentation on CATEGORY_BROWSABLE [2]:

Activities that can be safely invoked from a browser must support this category. For example, if the user is viewing a web page or an e-mail and clicks on a link in the text, the Intent generated execute that link will require the BROWSABLE category,

---
From this reading, it seems CATEGORY_BROWSABLE is only necessary when clicking links from web content, not when passing around urls.

However, I don't understand how opening a url in the browser than sharing that url is better than just clicking the url...

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=6932c3a8a0fc#3299
[2]: http://developer.android.com/reference/android/content/Intent.html#CATEGORY_BROWSABLE

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #4)
> However, I don't understand how opening a url in the browser than sharing
> that url is better than just clicking the url...

Perhaps it's only for clicking links that directly open an android application? i.e. intent://, android-app://, and customSchemes://.

Comment 6

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #5)
> (In reply to Michael Comella (:mcomella) from comment #4)
> > However, I don't understand how opening a url in the browser than sharing
> > that url is better than just clicking the url...
> 
> Perhaps it's only for clicking links that directly open an android
> application? i.e. intent://, android-app://, and customSchemes://.

Yeah, I think the confusing thing here is that the "invoked from a browser" bit really means "invoked from a web view", not from an app that is a browser.

If I'm understanding the history here correctly, the main issue is that we want to add CATEGORY_BROWSERABLE now that we support these URIs that directly open android applications, so maybe we should just add it in this case.

Or alternately, should we find a way to only add CATEGORY_BROWSABLE when a link is clicked from web content? We can't actually load intent:// URIs in the browser, so you should never be able to choose to share this URL from the browser through the share menu, right?

I'm not sure I have a clear story of all the things at play here, maybe we should sit down together and you can tell me a story of all the different ways we interact with intents. If CATEGORY_BROWSABLE is what's causing all our regressions, and we only added that because we now support intent:// URIs, it seems to me like it should be fine to special-case these kinds of intents and only add that category for them.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to :Margaret Leibovic from comment #6)
> If I'm understanding the history here correctly, the main issue is that we
> want to add CATEGORY_BROWSERABLE now that we support these URIs that
> directly open android applications, so maybe we should just add it in this
> case.

Sounds like a good v1: filed bug 1182328.

> Or alternately, should we find a way to only add CATEGORY_BROWSABLE when a
> link is clicked from web content?

v2: this bug.

> We can't actually load intent:// URIs in
> the browser, so you should never be able to choose to share this URL from
> the browser through the share menu, right?

The long-press share menu will share the link as plain-text which should be harmless. I guess the issue occurs when these links get turned into Intents.

> I'm not sure I have a clear story of all the things at play here, maybe we
> should sit down together and you can tell me a story of all the different
> ways we interact with intents.

FWIW, I think I'm starting to understand with the exception of mimeTypes, but the code is messy and makes it difficult to manage the handling (hence this bug).

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

41 is doing it's own thing in bug 1182328.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Wes recommended writing some unit tests before moving forward with this - sounds good to me.

Note that the build in 42 still has the flaws bug 1182328 intends to fix in 41.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Whiteboarding of call hierarchies

Margaret and I discovered that (more or less) all web content links go through GeckoAppShell.getOpenURIIntent so if we add an argument, shouldAddBrowsable, we'll break the compile, and we can chase it up the tree to only add Browsable when we need to.

One exception to this may be file:// urls, but we'll get to that when we get there.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

As per comment 10, getOpenURIIntent would also be a good place to unit test.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Spoke with Snorp: he suggested we move all of the Java stuff to our own Java methods (e.g. IntentUtils) and leave the GeckoAppShell code alone, with the exception of adding BROWSABLE - apparently most of that is planned to move into C++ land now that there are reasonable bindings.

There is a bit of a disconnect where the Java calls into Gecko to share (e.g. JS links) but we can separate as much as possible.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

No one is actively working on this and the Intent handling code is roughly stable, so there's no need to burn the oceans with this change (yet) – removing tracking flag.

Comment 16

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.