818987 - Links to media files should have an "Open in app" option

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Wesley Johnston (:wesj)]

+++ This bug was initially created as a clone of Bug #791654 +++

Steps to reproduce:
1. Open Firefox for Android
2. Go to http://searchmp3.mobi/ and tap any Top Rated Tracks
3. Long press on Download button (potentially a captcha step in here too)

Expected result:
Context menu should have an "Open in app" option.
Actual result:
No such option

I need to build a reduced test case for this to confirm that the link does point to an MP3 file here.

Comment 1

[Daniel Desira]

I believe we should also add a download button to the menu since it does not show in the Nightlies. I would like to work on this bug. It my first time. How can I get in contact with the mentor please? :-)

Comment 2

[Wesley Johnston (:wesj)]

I can provide you with help right in here! Or you can grab me on mozilla irc in #mobile or email me if you want. There are a few starting steps here to figure out exactly what the bug is and what we should do.

We have some code in Fennec that should be doing this already. The first thing you might want to check here is exactly whats going on on searchmp3.mobi. There desktop site actually has links to an html page that redirect to the mp3 file. Based on that I guess the problem is that we just don't know these are MP3's until its too late.

If that's the case, it would be nice to test this on a link that is more what we think. i.e. you can put together a quick test page with a link to an mp3 and make sure that we show the "Open in App" option in the Context menu.

If that works (it should...) we should probably add this option ("Open in App") to the video long tap context menu (you're welcome to add the download button here if you want too, although that's bug 791654).

First steps are to get a Fennec build up and running. There are pretty in depth instructions here: https://wiki.mozilla.org/Mobile/Fennec/Android but don't hesitate to ask if you run into problems.

Context menu options for Fennec are added using our context menu api:

https://developer.mozilla.org/en-US/docs/Extensions/Mobile/API/NativeWindow/contextmenus

This particular context menu item is added here in browser.js:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#7625

The first callback there is used to get the title for the context menu item. The second parameter (this.filter) is a function to determine if this context menu item should be shown. They both use NativeWindow.contextmenus._getLink(node); to determine if we're looking at a link. We want that to work for elements other than links, but I don't want to change NativeWindow.contextmenus._getLink since its used by other guys.

We could probably move that a new function on ExternalApps (_getLink works) that can call NativeWindow.contextmenus._getLink, but also check if this node is a media element and if so get its url. Something similar to what we do here for the title of a context menu should work:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1518

I'll add some notes in the download bug about how/where to fix it as well. Hope that helps! Don't be afraid to ask if you run into issues.

Comment 3

[Wesley Johnston (:wesj)]

Hey Daniel, Just checking in to see if you've made any progress here or if there's anything I can do to help? A good first step (once you can build) with this kinda stuff is just changing something and seeing what breaks. For instance, you can change the two calls to:

uri = NativeWindow.contextmenus._getLink(node);

in ExternalApps (in browser.js) to something like:

uri = "http://www.youtube.com";

and everything in the world should start showing "Open in YouTube" in the context menu.

Comment 4

[Sagar J P]

Hi,
Is it okay if I get working on this too? This is my first time with Mozilla. I saw your comments and I can start to work on it if you say so.
Thanks!

Comment 5

[Wesley Johnston (:wesj)]

Hmm.. maybe you'd like to look at bug 791654? Its very similar.

Comment 6

[heyjinsu]

Hi Wesley,

Can I take on this issue?
I've been playing around with this issue for awhile and I'm having trouble retrieving external apps from HelperApps.getAppsForUri function. http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#8033

Am I passing in the arguments correctly into the getAppsForUri function?
http://www.pastebin.mozilla.org/2134308

Comment 7

[Wesley Johnston (:wesj)]

Awesome. The code looks generally right to me. What is the spec of the url you end up passing in? I know we've found bugs in our code to find third party apps before, so its possible the error is there. Also, note that we throw out any handlers that look like other browsers in HelperApps:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/HelperApps.js#38

So if the system isn't returning anything that can open the url except browsers, we'd return []. You're welcome to try commenting out that filtering and see if it helps. A WIP patch might help also me help you.

Comment 8

[heyjinsu]

at the bottom of the pastebin, you can see the spec of the url.
E/GeckoConsole(18299): extApp.filter > uri = [xpconnect wrapped nsIURI]
E/GeckoConsole(18299): extApp.fiter > uri.spec = http://html5demos.com/assets/dizzy.mp4
E/GeckoConsole(18299): extApp.filter > calling getAppsForUri
E/GeckoConsole(18299): happs > extApp debug: urlHanlders = [xpconnect wrapped nsIMutableArray]
E/GeckoConsole(18299): happs > extApp debug: urlApp.name = Android chooser
E/GeckoConsole(18299): happs > extApp debug: urlApp.name = Internet
E/GeckoConsole(18299): happs > extApp debug: urlApp.name = Firefox
E/GeckoConsole(18299): happs > extApp debug: urlApp.name = Fennec jinsu
E/GeckoConsole(18299): extApp.filter > apps.length = 0

Is that how it's supposed to be passed in? I'm confused because I was expecting to see some apps but I'm only seeing browsers.

Comment 9

[Wesley Johnston (:wesj)]

Sorry I missed that bit in the pastebin. Yeah, looks like everything is correct, we're just screwing up finding apps that will work with it. I'm going to guess that we need to use the mimetype here rather than url when querying Android.

You mind trying something else? Lets try to guess the file mimetype/extension from the url. You'll need to do something like:

let mimeService = Cc["@mozilla.org/mime;1"].getService(Ci.nsIMIMEService);
let type = mimeService.getTypeFromURI(uri);
let mimeInfo = mimeService.getFromTypeAndExtension(type, ); // would be good to guess at the extension if we can here
let handlers = mimeInfo.possibleLocalHandlers;

Comment 10

[heyjinsu]

Oo ok. I'll give it a try tonight, Wesley.

Comment 11

[heyjinsu]

I tried guessing mimetype
http://www.pastebin.mozilla.org/2135494

seems like it should work, but I'm getting Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIMIMEInfo.possibleLocalHandlers]

I'll wrap it up here for now...how do i start debugging this error?

Comment 12

[heyjinsu]

A quick update and a question, Wesley.

As suggested by Mark(:mfinkle), I used nsIExternalSharingAppService.getSharingApps(mimeType) and got some applications such as gmail, email, facebook... But these applications are more like sharing applications than media players, so I wanted clarify what applications are expected.

Sorry, this is exactly the reduced test case you suggested in comment 2. lol...

For example, a page with a html5 audio element:
http://demos.w3avenue.com/html5-unleashed-tips-tricks-and-techniques/sample-04-audio-demo.html

1. long press on the audio element(open a "Context Menu")
2. "Open with an app"
3. what are the expected applications? could you name me some? so I can download them and test out my code.

http://www.pastebin.mozilla.org/2164052
at the end is the log output, which shows what applications got returned by getSharingApps()

Comment 13

[Wesley Johnston (:wesj)]

I'm expecting media player apps, which will probably vary a lot by phone. On my S3 I would expect "Music Player" and "Play Music" to appear for Audio. "Video Player" for video.

Comment 14

[heyjinsu]

Attachment: works for html5 audio and video elements

"Open With an App" context menu will show for html5 audio and video elements.
mostly tested on http://acolor.herokuapp.com/about

Some shortcomings in this version:
1. Before: When "Open with an App" is selected, the possible applications to choose from do not include browser applications(Chrome, default Internet, fennec...)
   After: browser applications do not get filtered...

2. Before: if there is only one non-browser app in possible external applications to open with, the context menu will customize to "Open with APP_NAME app".
   After: no such customization...only "Open With an App"

3. links that do not end with a file extension such as the searchmp3.mobi download button are not recognized.

I wanted to get some feedbacks at this point so I know I'm on the right track.

Comment 15

[Wesley Johnston (:wesj)]

Comment on attachment 720571 [details] [diff] [review]
works for html5 audio and video elements

Sorry for the delay. I like this a lot. Much simpler and we can side step around that ugly MimeInfo stuff. Plus, it exposes some nice functionality for addons that we get frequent requests for. Splinter review seems to be broken for me.

-                extends GeckoActivity 
+                extends GeckoActivity

Don't bother with white space changes here. There a bunch of them throughout, so I'll just say this once.

+            } else if (event.equals("Apps:Open")) {
+              GeckoAppShell.openUriExternal(message.getString("url"), message.getString("mime"),
+                  "", "", message.getString("action"), "");

Lets take this a bit further if we're going to do it. The promptService has a method getSafeString that we could move to someplace more universal (org.mozilla.fennec.utils.JSONUtils?), and we can then allow messages to optionally pass up any of the arguments openUriExternal takes.

+            } else if (event.equals("Apps:Get")) {
+              Intent intent = GeckoAppShell.getOpenURIIntent(mAppContext, message.getString("url"),
+                  message.getString("mime"), message.getString("action"), "");
+              String[] handlers = GeckoAppShell.getHandlersForIntent(intent);
+              ArrayList<String> appLabelList = new ArrayList<String>(handlers.length);

Just appList is fine.

+              int numAttr = 4;
+              for(int i = 0; i < handlers.length; i += numAttr) {

At first glance, I didn't really understand why you're stepping by 4 here (I see now the results from getHandlersForIntent are designed to make parsing easy, not to make sense). Can you add a comment explaining why? Also a better variable name for numAttr might help? It would also be useful to pass down the "default" piece here. Its not handled very well in HelperApps yet, but I'd like to fix that soon.

+                  appLabelList.add(handlers[i]);
+              }
+              JSONObject handlers_JSON = new JSONObject();

We don't use _ in variable names ever. But I'm not sure we really need this. Can you just do something like:

mCurrentResponse = new JSONArray(appLabelList).toString();

+  parseAppsJSON: function parseApps(aJSON) {
+    // apps -> {apps: [app1, app2, app3, ...]}
+    let handlers_obj = JSON && JSON.parse(aJSON) || null;
+    let appsList = handlers_obj ? handlers_obj.apps : [];

We aren't usually this defensive, since we're not interacting with 3rd party code. Basically, if you somehow get here with a message that doesn't match, we'd rather throw an error than cover it up.

+        // apps = HelperApps.getAppsForUri(uri);

Remove the commented out code

+      // return apps.length == 1 ? Strings.browser.formatStringFromName("helperapps.openWithApp2", [apps[0]], 1) :
+      //                           Strings.browser.GetStringFromName("helperapps.openWithList2");
+      return Strings.browser.GetStringFromName("helperapps.openWithList2");

Why did you remove this totally great string?

Comment 16

[Wesley Johnston (:wesj)]

"As suggested by Mark(:mfinkle), I used nsIExternalSharingAppService.getSharingApps(mimeType) and got some applications such as gmail, email, facebook... But these applications are more like sharing applications than media players, so I wanted clarify what applications are expected."

I talked to mfinkle about this today. I'm not familiar with nsIExternalSharingAppService, but were we using the right mimeType here? From looking at the code, it really seems like this should work.

Comment 17

[heyjinsu]

Haha, sorry about the whitespace issues. My vim configuration doesn't want to play nicely with mercurial, but I won't let it slip through me again.

For comment 16:
nsIExternalSharingAppService.getSharingApps hard-codes an intent with 'SEND' action when we want is an intent with 'VIEW' action. So the returned apps are not so relevant. I chose to skip over the AppService classes until I can figure out MimeInfo c++ files.

For comment 15:
I'm working on putting out "Open with YouTube App" in the context menu(Currently it's only "Open with Another App" for all cases). I'll put in your suggestions. As for the getSafeString method, I'll have to read up on it first and update you.

Thanks Wesley!

Comment 18

[heyjinsu]

Attachment: clean up the code

It's been awhile!
-Open with "NAME" App menu put in.
-clean up the code.

I like your suggestion about using getSafeString method, but I'm afraid to create a JSONUtils file with just one function...haha, so I didn't use it.

Let me know what you think.

Comment 19

[Wesley Johnston (:wesj)]

Comment on attachment 728851 [details] [diff] [review]
clean up the code

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

Sorry for the delay. I like this approach, but after talking to mfinkle, I think it might be better if we used a more strictly defined API. Since HelperApps is around and should probably do this, I think we should use it! Something like what I've outlined below is just basically taking the current code and moving things around... Lets get mfinkle's feedback before you do all this though....

::: mobile/android/chrome/content/browser.js
@@ +8053,5 @@
>        }
>        let apps = [];
> +      if (uri) {
> +        let mimeType = ContentAreaUtils.getMIMETypeForURI(uri) || "";
> +        let handlersJSON = sendMessageToJava({type: "Apps:Get", mime: mimeType, action: "", url: uri.spec});

So instead, we can do something like apps = HelperApps.getAppsForUri(uri), and move this message passing logic into there.

@@ +8083,5 @@
>        let apps = [];
>        if (uri) {
> +        let mimeType = ContentAreaUtils.getMIMETypeForURI(uri) || "";
> +        let handlersJSON = sendMessageToJava({type: "Apps:Get", mime: mimeType, action: "", url: uri.spec});
> +        apps = HelperApps.parseAppsJSON(handlersJSON);

Same here.

@@ +8095,5 @@
> +    let mimeType = ContentAreaUtils.getMIMETypeForURI(uri) || "";
> +    let handlersJSON = sendMessageToJava({type: "Apps:Get", mime: mimeType, action: "", url: uri.spec});
> +    let apps = HelperApps.parseAppsJSON(handlersJSON);
> +    let msg = {type: "Apps:Open", mime: mimeType, action: "", url: uri.spec, packageName: "", className: ""};
> +    sendMessageToJava(msg);

Here, ideally we could also move this to HelperApps. I'm not sure why this Apps:Get is here actually, but we can make this a simple HelperApps.launchUri(uri);. Alternatively, we could do some work to make the apps returned above implement nsIHandlerApp [1] and you could do:

apps = HelperApps.getAppForUri(uri);
apps[0].launchWithUri(uri);

but for our simple case here that doesn't seem necessary :)

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/mime/nsIMIMEInfo.idl#212

Comment 20

[heyjinsu]

thanks for the feedback, wes. time to make the code right. after hearing back from mark.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 728851 [details] [diff] [review]
clean up the code

Looking good, but I'd really like to not need App:Open and App:Get messages. I really want to be able to use the nsIExternalSharingAppService.idl

Comment 22

[Josh Matthews [:jdm]]

Any update here? It sounds like the patch was coming along nicely.

Comment 23

[heyjinsu]

Sorry folk, i've been lazy about this ever since i started a new job about 3 months ago. I'll try to set up the dev environment this weekend and give another crack at it.

Comment 24

[heyjinsu]

I looked at the code more to see if i can use other classes. Please correct me if I'm wrong, with the current implementation lack a getHandler method to that uses both uri and mime-type. The only methods I could use are in GeckoAppShell.java class...

I can't use nsIExternalSharingAppService because the intent action is hard-coded to 'send' when we need 'view'. https://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/android/nsExternalSharingAppService.cpp#48

I have to use both uri and mime-type because uri will be used for media links that don't have file extension(like youtube link) mime-type will be used for media links with file extension.

I see two options at this point.
1) go with message passing to Java, more or less the current work

2) modify nsExternalUrlHandler to fetch handlers using url and mime-type
https://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/android/nsExternalURLHandlerService.cpp#20
this would also change nsMimeInfoAndroid, AndroidBridge, and GeckoAppShell I believe.

Option 2 seems like the right way...but we'll edit working codes.

Let me know what your thoughts are. :mfinkle and :wesj

Comment 25

[Wesley Johnston (:wesj)]

I vote for both. I'd rather land this similar to as-is, and fix the urlHandlerService in a different patch.

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Wesley Johnston (:wesj) from comment #25)
> I vote for both. I'd rather land this similar to as-is, and fix the
> urlHandlerService in a different patch.

I can agree with this idea.

I would like the crazy array sent back by "App:Get" to be more sane. Also, change "App:Get" and "App:Open" to "Intent:Get" and "Intent:Open". Lastly, it might be nice to use HelperApps.js as a JS front-end to "Intent:Get" and "Intent:Open" instead of calling them directly.

In any case, let's move forward.

Comment 27

[heyjinsu]

"Alternatively, we could do some work to make the apps returned above implement nsIHandlerApp [1] and you could do:"

is there any similar example in the javascript codebase that creates a nsIHandlerApp instance in javascript?

Comment 28

[heyjinsu]

Attachment: structure the array returned from GeckoAppShell.getHandlersForIntent

I thought that a hash format is less expensive than making a nsIHandlerApp for each app info returned from the GeckoAppShell.getHandlersForIntent because all we can do is look at the app's name so far.

Comment 29

[Wesley Johnston (:wesj)]

Comment on attachment 779294 [details] [diff] [review]
structure the array returned from GeckoAppShell.getHandlersForIntent

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

I think this is a good first step. Mostly some nitpicky things. I think we'll eventually need to do some more work to broaden the abilities of HelperApps (and move it into a jsm), but I'd take this as a first step (and a nice new feature!).

I'd like mark to give it a glance too.

::: mobile/android/base/GeckoApp.java
@@ +712,5 @@
>                      Log.e(LOGTAG, "Received Contact:Add message with no email nor phone number");
>                  }                
> +            } else if (event.equals("Intent:Get")) {
> +                Intent intent = GeckoAppShell.getOpenURIIntent(sAppContext, message.getString("url"),
> +                    message.getString("mime"), message.getString("action"), "");

Might be better to use message.optString("something") here. That way callers can only specify the pieces they know/want.

@@ +724,5 @@
> +                mCurrentResponse = handlersJSON.toString();
> +            } else if (event.equals("Intent:Open")) {
> +                GeckoAppShell.openUriExternal(message.getString("url"),
> +                    message.getString("mime"), message.getString("packageName"),
> +                    message.getString("className"), message.getString("action"), "");

Same here.

::: mobile/android/chrome/content/HelperApps.js
@@ +75,5 @@
> +               action: "", url: uri.spec, packageName: "", className: ""};
> +    sendMessageToJava(msg);
> +  },
> +
> +  _constructAppsHash: function _constructAppsHash(aJSON) {

Lets name this _parseApps

@@ +78,5 @@
> +
> +  _constructAppsHash: function _constructAppsHash(aJSON) {
> +    // aJSON -> {apps: [app1Label, app1Default, app1PackageName, app1ActivityName, app2Label, app2Defaut, ...]}
> +    // see GeckoAppShell.java getHandlersForIntent function for details
> +    let resolveInfoFlatArray = aJSON.apps;

lets just call this 'appInfo'

@@ +79,5 @@
> +  _constructAppsHash: function _constructAppsHash(aJSON) {
> +    // aJSON -> {apps: [app1Label, app1Default, app1PackageName, app1ActivityName, app2Label, app2Defaut, ...]}
> +    // see GeckoAppShell.java getHandlersForIntent function for details
> +    let resolveInfoFlatArray = aJSON.apps;
> +    let numAttr = 4; // 4 elements per ResolveInfo: label, default, package name, activity name.

You might as well use const here. const numAttr = 4;

::: mobile/android/chrome/content/browser.js
@@ +7480,5 @@
>  
>  var ExternalApps = {
>    _contextMenuId: -1,
>  
> +  // contextmenu._getLink doesn't pick up on html5 media links yet.

Hmm... this comment isn't quite right (or I'd say just add this to _getLink. But I don't think we want things like "Open in new tab" showing on video elements yet, so I do think we want to keep this separate (or add a a parameter to contextmenu._getLink)

@@ +7497,5 @@
>      this._contextMenuId = NativeWindow.contextmenus.add(function(aElement) {
>        let uri = null;
>        var node = aElement;
>        while (node && !uri) {
> +        uri = NativeWindow.contextmenus._getLink(node) || ExternalApps._getMediaLink(node);

Since we're doing this all over, maybe ExternalApps._getMediaLink(node) should just call NativeWindow.contextmenus._getLink and fast path out if it finds something.

Comment 30

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 779294 [details] [diff] [review]
structure the array returned from GeckoAppShell.getHandlersForIntent

>diff --git a/mobile/android/chrome/content/HelperApps.js b/mobile/android/chrome/content/HelperApps.js

>   getAppsForUri: function getAppsFor(uri) {

>+    let mimeType = ContentAreaUtils.getMIMETypeForURI(uri) || "";
>+    // empty action string defaults to android.intent.action.VIEW
>+    let msg = {type: "Intent:Get", mime: mimeType,
>+               action: "", url: uri.spec, packageName: "", className: ""};

nit: we like to format this a bit

let msg = {
  type: "Intent:Get",
  mime: mimeType,
  action: "",
  url: uri.spec,
  packageName: "",
  className: ""
};

>+    for (var i = 0; i < apps.length; i++) {

var -> let

(this file does use a lot of "var", but let's not add more

>   openUriInApp: function openUriInApp(uri) {

>+    let msg = {type: "Intent:Open", mime: mimeType,
>+               action: "", url: uri.spec, packageName: "", className: ""};

nit: format

>+    for (var i = 0; i < resolveInfoFlatArray.length; i += numAttr) {

var -> let

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+  _getMediaLink: function(aElement) {

>+    if (aElement.nodeType == Ci.nsIDOMNode.ELEMENT_NODE &&
>+        (aElement instanceof Ci.nsIDOMHTMLMediaElement && mediaSrc)) {

no need to wrap this in front-end JS

>   filter: {
>     matches: function(aElement) {
>-      let uri = NativeWindow.contextmenus._getLink(aElement);
>+      let uri = NativeWindow.contextmenus._getLink(aElement) ||
>+                ExternalApps._getMediaLink(aElement);

No need to wrap this in front-end JS

>   openExternal: function(aElement) {
>-    let uri = NativeWindow.contextmenus._getLink(aElement);
>+    let uri = NativeWindow.contextmenus._getLink(aElement) ||
>+              ExternalApps._getMediaLink(aElement);

Same

Comment 31

[Mark Finkle (:mfinkle) (use needinfo?)]

My f+ is in addition to Wes' suggested changes

Comment 32

[heyjinsu]

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+  _getMediaLink: function(aElement) {

>+    if (aElement.nodeType == Ci.nsIDOMNode.ELEMENT_NODE &&
>+        (aElement instanceof Ci.nsIDOMHTMLMediaElement && mediaSrc)) {

no need to wrap this in front-end JS <-??

>   filter: {
>     matches: function(aElement) {
>-      let uri = NativeWindow.contextmenus._getLink(aElement);
>+      let uri = NativeWindow.contextmenus._getLink(aElement) ||
>+                ExternalApps._getMediaLink(aElement);

No need to wrap this in front-end JS <-??

>   openExternal: function(aElement) {
>-    let uri = NativeWindow.contextmenus._getLink(aElement);
>+    let uri = NativeWindow.contextmenus._getLink(aElement) ||
>+              ExternalApps._getMediaLink(aElement);

Same <-??

I'm not sure what you mean by those, Mark. Would you clarify for me?

Comment 33

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to heyjinsu from comment #32)

> I'm not sure what you mean by those, Mark. Would you clarify for me?

Keep the lines of code on one line. Do not break them up to two lines.

Comment 34

[heyjinsu]

Attachment: fastout with _getLink url in _getMediaLink and formatting changes

As suggested, I changed _getMediaLink to call _getLink and fastout if a url is found.
One shortfall with this patch is that when clicking 'Open with Youtube App', it doesn't actually launch with Youtube App but launches an activity chooser or default app.

Comment 35

[Wesley Johnston (:wesj)]

Comment on attachment 786750 [details] [diff] [review]
fastout with _getLink url in _getMediaLink and formatting changes

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

This looks great. I'll make these changes and land it! Thanks for the hard work here Jinsu.

::: mobile/android/base/GeckoApp.java
@@ +1575,5 @@
>          registerEventListener("Update:Install");
>          registerEventListener("PrivateBrowsing:Data");
>          registerEventListener("Contact:Add");
> +        registerEventListener("Intent:Open");
> +        registerEventListener("Intent:Get");

Lets name this Intent:GetHandlers to be specific. Also, some intents can return a value, so Intent:Get seems like something we might want to save.

::: mobile/android/chrome/content/browser.js
@@ +7484,5 @@
> +  // extend _getLink to pickup html5 media links.
> +  _getMediaLink: function(aElement) {
> +    let uri = NativeWindow.contextmenus._getLink(aElement);
> +    if (uri == null) {
> +      let mediaSrc = aElement.currentSrc || aElement.src;

Lets move this inside the if statement.

Comment 36

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/a0670c5cac39

Comment 37

[heyjinsu]

I appreciate your patience in helping me with this patch, Wesley and Mark. I hope I can do a better job next time!

Comment 38

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/a0670c5cac39