PageAction for helper apps

RESOLVED FIXED in Firefox 26

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: wesj, Assigned: shilpanbhagat)

Tracking

(Depends on 1 bug, Blocks 1 bug, {user-doc-needed})

Trunk
Firefox 26
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

Reporter

Description

6 years ago
We should show a page action for helper apps that can open a url (ignoring http ones). If there's one app, we could show the icon for the app in the urlbar (or maybe that's confusing? Google Maps with a Google maps icon on the right?). If there's more than one, we can show a custom icon of some sort?
Reporter

Updated

6 years ago
Assignee: nobody → sbhagat
Reporter

Comment 1

6 years ago
Ian, just wanted to get you looped into this early. You ok with doing something like this?
Flags: needinfo?(ibarlow)
Yep, this sounds good to me (I included something like this in the designs I posted a while back http://cl.ly/image/1O1a2U201a27)

As for if it's confusing to have a google maps icon on the right, let's try it and see. We might consider showing a tooltip the first couple of times to explain, something like "TIP: tap here to open this in the ___ app". That's work outside of this bug though, to be clear :)
Flags: needinfo?(ibarlow)
Assignee

Comment 3

6 years ago
Rough patch, displays page action for helper apps. What do you think about the direction I am going here?
Attachment #788830 - Flags: feedback?(wjohnston)
Reporter

Comment 4

6 years ago
Comment on attachment 788830 [details] [diff] [review]
[WIP] PageAction for helper apps

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

I think this might be an ok first draft of this feature (if we get an icon). I notice that this doesn't always launch the app. For instance if I go to YouTube (Desktop site) and tap it it gives me a chooser with all the browsers listed in it. If the list is only browsers + 1 app, we should just launch with the app. That's probably a bug in helper apps, but you should fix it here.

There are a few features I'd like this to have in the end, but I wonder if we file them as follow ups to having the basic feature in place:

1.) In order to do the icons like I had suggested before, we'll need to somehow get a reference to the icon drawable in js. I think we can maybe use:

http://developer.android.com/reference/android/content/pm/ResolveInfo.html#getIconResource%28%29

to get the resourceID though, and pass it to JS as a drawable:// uri, which JS can then pass to the pageactions service.

2.) We also might need a way to detect redirects and continue to show the page action across them. i.e. if typing youtube.com takes me to vnd.youtube.com which takes me to m.youtube.com, and the Youtube app is registered for vnd.youtube.com, we should show the icon on m.youtube.com as well...

3.) I think we might also want to icon support to prompts and replace the default Android chooser with a prompt (so that we can filter browsers from the list when we do show it).

::: mobile/android/chrome/content/HelperApps.js
@@ +81,5 @@
> +  _setPageActionFor: function setPageActionFor(uri) {
> +    this._pageActionUri = uri;
> +    if (this._pageActionId == undefined) {
> +      this._pageActionId = NativeWindow.pageactions.add({
> +        title: "Open in App",

You'll need to localize these.
Attachment #788830 - Flags: feedback?(wjohnston) → feedback+
Watch out for scope creep! Use new bugs for different bugs, including bugs in helper apps.
Wes, Shilpan here is an "Open in app" icon you can use, for cases where we either can't get the app icon we want or if there are multiple options to choose from. 

http://cl.ly/3f0D263c0X0u
Assignee

Comment 7

6 years ago
Posted patch PageAction for helper apps (obsolete) — Splinter Review
The pageAction now launches the app if there is only one app that can handle it (except for browsers) otherwise the user gets a chooser.
Attachment #788830 - Attachment is obsolete: true
Attachment #790562 - Flags: review?(wjohnston)
Assignee

Comment 8

6 years ago
Posted patch PageAction for helper apps (obsolete) — Splinter Review
Oops, forgot about a minute change
Attachment #790562 - Attachment is obsolete: true
Attachment #790562 - Flags: review?(wjohnston)
Attachment #790566 - Flags: review?(wjohnston)
Reporter

Comment 9

6 years ago
Comment on attachment 790566 [details] [diff] [review]
PageAction for helper apps

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

I think I want to change this up a bit, and I have some concern about querying java (in a blocking manner) every time a page is loaded/tab is changed.

::: mobile/android/chrome/content/HelperApps.js
@@ +68,5 @@
>      }
>      return found;
>    },
>  
> +  setPageAction: function setPageAction(uri) {

Lets call this updatePageAction.

@@ +69,5 @@
>      return found;
>    },
>  
> +  setPageAction: function setPageAction(uri) {
> +    let urlData = this.getAppsForUri(uri);

urlData -> apps

@@ +73,5 @@
> +    let urlData = this.getAppsForUri(uri);
> +    if (urlData.length != 0) {
> +      this._setPageActionFor(uri, urlData);
> +    } else if (this._pageActionId != undefined) {
> +      this._removePageAction();

I think I would rather we make this

if (apps.length > 0) this.setPageAction(url, apps);
else this.removePageAction();

and then:

_setPageAction = function(url, apps) {
  if (this._pageActionId)
    this.removePageAction();
  // do the real work to add
}

removePageAction = function() {
  if (!this._pageActionId)
    return;
  // do the real work to remove
}

Also, it scares me a bit that we're redoing this query on every tab selected event. I wonder if we can cache the app list on the tab and invalidate it in onLocationChange?
Attachment #790566 - Flags: review?(wjohnston)
Assignee

Comment 10

6 years ago
Part 2 will cover the caching.
Attachment #790566 - Attachment is obsolete: true
Attachment #793165 - Flags: review?(wjohnston)
Assignee

Comment 11

6 years ago
Posted patch Part 2: App cache (obsolete) — Splinter Review
Attachment #793210 - Flags: review?(wjohnston)
Assignee

Comment 12

6 years ago
Posted patch Part 2: App cache (obsolete) — Splinter Review
Revised a few things in the algorithm based on how objects behave.
Attachment #793210 - Attachment is obsolete: true
Attachment #793210 - Flags: review?(wjohnston)
Attachment #793226 - Flags: review?(wjohnston)
Reporter

Comment 13

6 years ago
Comment on attachment 793165 [details] [diff] [review]
Part 1: PageAction for helperApps

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

::: mobile/android/chrome/content/HelperApps.js
@@ +79,5 @@
> +
> +  _setPageActionFor: function setPageActionFor(uri, apps) {
> +    this._pageActionUri = uri;
> +    if (this._pageActionId != undefined)
> +      return;

I think we want to remove the page action here? Also, I think I'd just do

if (this._pageActionId)
  this._removePageAction();

like you do in removePageAction. Are there cases where the id is falsey?
Attachment #793165 - Flags: review?(wjohnston) → review+
Reporter

Comment 14

6 years ago
Comment on attachment 793226 [details] [diff] [review]
Part 2: App cache

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

I think I'd rather we just create a little generic LRU object here, maybe backed by an array?. These lists are small enough that I don't think lookup is really an issue. I wrote up a dumb little one:

http://pastebin.mozilla.org/2894548

(testing this, I had to use window.performance.now() in order to detect any difference in time when adding a series of things).

::: mobile/android/chrome/content/HelperApps.js
@@ +78,5 @@
>  
> +  _appCache: {},
> +
> +  addToAppCache: function addToAppCache(uri, apps) {
> +    this._appCache[uri.spec] = {apps: apps, count: 0};

Adding this so early, won't it just always be deleted?

@@ +79,5 @@
> +  _appCache: {},
> +
> +  addToAppCache: function addToAppCache(uri, apps) {
> +    this._appCache[uri.spec] = {apps: apps, count: 0};
> +    if (Object.keys(this._appCache).length <= 5)

Make this '5' a const somewhere.
Attachment #793226 - Flags: review?(wjohnston)
Reporter

Comment 15

6 years ago
whoops. Wront link: http://pastebin.mozilla.org/2894549
Assignee

Comment 16

6 years ago
Do you think we really need time? I thought we were removing from the array based on how frequently we used the element in the array. Yours seem to simply remove the oldest?
I was trying to be efficient by using objects I think we can still use an Object for this : http://pastebin.mozilla.org/2895082

Or, if we do not want to use the keys at all and simply use an array: http://pastebin.mozilla.org/2895021

I can clean up to make this more readable.
Assignee

Comment 17

6 years ago
Carry forward from the previous one.
Attachment #793165 - Attachment is obsolete: true
Attachment #793226 - Attachment is obsolete: true
Attachment #794935 - Flags: review+
Reporter

Comment 19

6 years ago
(In reply to Wesley Johnston (:wesj) from comment #4)
> 2.) We also might need a way to detect redirects and continue to show the
> page action across them. i.e. if typing youtube.com takes me to
> vnd.youtube.com which takes me to m.youtube.com, and the Youtube app is
> registered for vnd.youtube.com, we should show the icon on m.youtube.com as
> well...

> 3.) I think we might also want to icon support to prompts and replace the
> default Android chooser with a prompt (so that we can filter browsers from
> the list when we do show it).

Can you file bugs for both of these (and maybe one for supporting icons in prompts as well :) ). #2 might be better filed as something like "Make YouTube helper app show up on YouTube mobile site" (that may involve a redirect fix, or maybe a "check with a few different subdomains" thing). #3 is probably a less necessary change, but I think it would be good to track.
https://hg.mozilla.org/mozilla-central/rev/e883ed151dc9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Depends on: 909400
Depends on: 909446
Reporter

Updated

6 years ago
Depends on: 911904
Duplicate of this bug: 919433
Just had a user find us on IRC to say "why do I have an Android icon in my URL bar when I load Boing Boing?". When he tapped it, it opened the video player, which threw an error.

Sounds like:

1. We probably need an answer to this question in SUMO, based on the Iceberg Principle of Confused Users.
2. There's a bug mis-recognizing a page as a video -- though I was unable to trigger the icon in Aurora or Nightly on my HTC One.
2.b) ... and displaying a small android in the URL bar probably isn't intended behavior.
3. We do need to iterate on the UI in a follow-up -- neither the user nor myself had any idea why the icon appeared (I only ended up here because mfinkle happened to be around to explain what was going on).
Keywords: user-doc-needed
OS: Linux → Android
Hardware: x86 → All
Reporter

Comment 23

6 years ago
2 is bug 911905. Im not sure how 2b and 3 differ but please file a new bug for ui changes (with ideas!)
Reporter

Comment 24

6 years ago
whoops. bug 911904 (stupid phone keyboard...)
Blocks: 921926
Depends on: 928439
Depends on: 929680

Updated

5 years ago
Depends on: 1115503
You need to log in before you can comment on or make changes to this bug.