769454 - Move HelperApps to context menus

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Our current helper apps implementation is a bit heavy weight. As a first step to undo that, let's not show a doorhanger, but instead just add a context menu item. When the user long taps on a link that can open in a (non browser) HelperApp, we'll add something like "Open in YouTube App" or "Open in App" when there is more than one choice.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

And back in the YouTube hack goes. This isn't perfect. For instance, on YouTube we end up redirecting (twice) from a link. So long tapping on the link to the m.youtube.com won't give the "Open in App" option. But it works for Maps links on Yelp, which is my other use case.

Changing the context menu option to include the App name would be nice follow up. I could hack it here, but would rather fix up the contextmenu API to allow dynamic names instead.

Comment 2

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

Comment on attachment 637723 [details] [diff] [review]
Patch

This isn't too bad, but I think we need the name of the helper app. Can we remove/re-add the context menu? Or maybe only add the menu when we need it?

Comment 3

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

Also, maybe we should backout/disable the current behavior while this is addressed.

Comment 4

[Wesley Johnston (:wesj)]

I'll split this up into two parts to remove the current behavior.

Adding dynamically isn't exactly possible with the current behavior.... maybe we can listen for the context menu event sent to the page and add the listener there. We'd have might also have to fire some notification when the prompt goes away...


Thats just too hacky. I'll add dynamic context menu titles instead I think. How nervous are you about changing the current NativeWindow API's?

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Backout patch

Backout the doorhanger. Leave in HelperApps.js for later.

Assuming this is ok with you, I'm PTO today and someone else will have to land.

Comment 6

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

(In reply to Wesley Johnston (:wesj) from comment #4)
> I'll split this up into two parts to remove the current behavior.
> 
> Adding dynamically isn't exactly possible with the current behavior....
> maybe we can listen for the context menu event sent to the page and add the
> listener there. We'd have might also have to fire some notification when the
> prompt goes away...
> 
> 
> Thats just too hacky. I'll add dynamic context menu titles instead I think.
> How nervous are you about changing the current NativeWindow API's?

Adding methods is fine. What other changes are you thinking about

Comment 7

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1502bb46634

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch

This uses an object with a toString() method to create a dynamic title for this entry. It depends on contextmenu code calling us in a particular order (call filter, if that works, get the name).

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch

Alternative approach, allows passing a function for the contextmenuitem's (is that one word or three?) name.

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Patch

Whoops. Tiny tweak I forgot to qref

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

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

Comment on attachment 640464 [details] [diff] [review]
Patch

>diff --git a/mobile/android/locales/en-US/chrome/browser.properties b/mobile/android/locales/en-US/chrome/browser.properties

>+helperapps.openWithList2=Open With App

is "Open With an App" a little better?

And we should update the document here to add support for the "function" version of "name"
https://developer.mozilla.org/en/Extensions/Mobile/API/NativeWindow/contextmenus

Comment 13

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/391f4f60d810

Updated the string. Updating the docs...

Comment 14

[Wesley Johnston (:wesj)]

Added docs at:
https://developer.mozilla.org/en/Extensions/Mobile/API/NativeWindow/contextmenus/add

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/391f4f60d810

Comment 16

[Aaron Train [:aaronmt]]

Is this going to Aurora?

Verified Fixed on trunk (07/11)

Discoverability is kind of tough on this. Who would long-tap on a link?

Comment 17

[Wesley Johnston (:wesj)]

(In reply to Aaron Train [:aaronmt] from comment #16)
> Discoverability is kind of tough on this. Who would long-tap on a link?

I don't think long tap on a link is that uncommon. Every mobile browser I know of has it. BUT I think the goal here was to have the option when its needed, but overall we want to keep people in the browser. i.e. I don't think we want this to be TOO discoverable.