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

NEW
Unassigned

Status

()

3 years ago
3 years ago

People

(Reporter: mcomella, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 unaffected)

Details

Attachments

(1 attachment)

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.
Summary: Split out code flow, or add flag, for links from web content (if possible :d) → Split out Intent handling code flow, or add flag, for links from web content (if possible :d)
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
Flags: needinfo?(wjohnston)
Note: bug 1175532 will add another work-around to the Category.BROWSABLE problem.
(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
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
(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

3 years ago
(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.
(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).
41 is doing it's own thing in bug 1182328.
status-firefox41: --- → unaffected
Duplicate of this bug: 1162182
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.
Created attachment 8633178 [details]
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.
As per comment 10, getOpenURIIntent would also be a good place to unit test.
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.
Duplicate of this bug: 1173625
Flags: needinfo?(wjohnston)
tracking-fennec: --- → 43+
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.
tracking-fennec: 43+ → ---
You need to log in before you can comment on or make changes to this bug.