942270 - Add Quickshare buttons to Context menu

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Deb Richardson [:dria] (plz NEEDINFO)]

Just wanted to make sure there was a bug on file for getting the quickshare buttons added to the context menu.  Not sure if work has started on this or not.

Ian's design flows have them mocked up.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP: QuickShare

This uses the same MenuItemActionView on the context menus.

Things done:
1. Added a "shareType" to the json object from Gecko. This can be link or image or text, based on the context.
2. The Share item occupies first row in the menu.
3. Click listeners for the app icons work.

Things to be done:
1. Casting a mShareActionProvider feels ugly. This needs to be a get() method instead.
   a. In the future, if there are going to be more ActionProviders, its better to have different get() methods for each.
   b. Having the ActionProviders in BrowserApp doesn't feel good. Instead GeckoApplication can hold it.
   c. There might be a cost involved in initializing ActionProviders. So, it's better to do it after startup.
2. There was a patch from wesj that cleans up Prompts. It's better to apply this patch over that, as it simplifies the logical entities uses in the context menu list by using MenuItems instead of json objects.
3. The click listener on actual share item doesn't work. This needs to be updated.
4. The dialog won't be dismissed on clicking app icons -- needs fixing.
5. Since the Share row is added to the top, the item position on the list are offset by one. May be, Gecko should return the list with share as first item to fix it.

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP: Multiple Share

Comment 3

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 8355618 [details] [diff] [review]
WIP: Multiple Share

This shows an example of how to use multiple action providers. By using a different history file, and extending GeckoActionProvider, this can be achieved. Some commented portion shows using "default" applications too.

Comment 5

[Brian Nicholson (:bnicholson)]

I think Wes is taking a look at this.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: WIP 1

Updating the WIP. This is mostly working, just needs lots of cleanup and a lot of commenting. It keeps srirams' work to make context menu items register themselves. i.e. the api for these particular items looks like:

NativeWindow.contextmenus.add({
  order: 1,       // sort order, higher is sorted higher in the menu
  label: "Share",
  selector: ...

  // I want to change this naming, but works for now
  shareType: function(aElement) { // Can be a function or an object
    title: aElement.textContent || aElement.title,
    uri: NativeWindow.contextmenus._getLinkURL(aElement),
    type: "text/plain",
  },
  icon: "drawable://ic_menu_share", // This is new, but we've wanted it for a bit
  menu: true, // This can probably be inferred from 'shareType'
  callback:   // not used if shareType is set
});

In Java I split off the menu showing code that we previously used for the File picker into an IntentChooserPrompt class. It can either be filled by handing it a list of Intents to query for activities, or using the GeckoActionProvider (since the list provided by it is sorted differently). For these special rows, all of the click handling is done in Java though. i.e. Clicking the "Share" button will show an IntentChooserPrompt. Clicking on a quick share icon will fire the intent directly in java.

This also creates a different GeckoActionProvider for for front of each mimetype. i.e. "text" from "text/plain", or "image" from "image/png". We need some better UI for discerning the difference between the two rows. Since Ian's in town, maybe we can iterate on some ideas :)

http://dl.dropboxusercontent.com/u/72157/quickshare2.png

Comment 7

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

One other thing that comes to my mind when looking at the screenshots is we could move "copy" and "save" to Share actions too. Thoughts?

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch 1

This grew a bit on me. We need an Intent picker for this, and we already had code to make one in the ActivityHandlerHelper class. I moved it out into its own IntentChooserClass and refactored the ActivityHandlerHelper. That also made it obvious that the ActivityHandlerHelper was pulling double duty, being both a FilePicker and a generic Activity dispatcher, so I split it in two.

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch 2

This is the real quick share stuff. Mostly the same as what sriram had written, and what I outlined above.

This forces us to bail out of the context menu generation when we hit Images (or other media type things that might be inside a link.

Comment 10

[Wesley Johnston (:wesj)]

Follow ups I want to work on somewhere else:

1.) Make these history files profile aware. We can probably set up some distribution stuff to initialize them at the same time

2.) Add Bookmark to the share menu. Force it to the left. Not sure how I'll do that....

Comment 11

[Wesley Johnston (:wesj)]

Comment on attachment 8368363 [details] [diff] [review]
Patch 2

Talked to ian this morning and realized this isn't quite what he wants. Moving to feedback if you want to start reading brian, but no hurry :)

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Patch 3

This mostly goes on top of the other patches to provide a tabbed interface for these dialogs. Still hooking things like clicking back up though.

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Screenshot

Comment 14

[Brian Nicholson (:bnicholson)]

Comment on attachment 8368363 [details] [diff] [review]
Patch 2

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

::: mobile/android/base/prompts/Prompt.java
@@ +505,1 @@
>              } catch(Exception ex) { }

This looks like a nasty place for any JSON bugs to hide. Can you catch the more specific JSONException here rather than just use a generic Exception catch-all? Can you also add a Log.e() in this catch statement with an error message and the exception to make it easier to debug?

@@ +505,4 @@
>              } catch(Exception ex) { }
>          }
> +
> +        PromptListItem[] arrays = new PromptListItem[length];

Why not just use a PromptListItem[] directly like we had before? Is it to prevent an array with holes in case an exception is thrown in the for loop above? Some comments would be good here to explain why this indirection is necessary.

@@ +541,5 @@
> +                    intent = GeckoAppShell.getShareIntent(GeckoAppShell.getContext(), uri, type, title);
> +                    showAsActions = true;
> +                    isParent = true;
> +                }
> +            } catch(Exception ex) {

Exception => JSONException. If some unrelated exception happens (e.g., in getShareIntent()), we don't want to swallow that exception since it could hide bugs.

@@ +542,5 @@
> +                    showAsActions = true;
> +                    isParent = true;
> +                }
> +            } catch(Exception ex) {
> +                Log.i(LOGTAG, "Exception parsing intent", ex);

Nit: Log.i => Log.e

@@ +545,5 @@
> +            } catch(Exception ex) {
> +                Log.i(LOGTAG, "Exception parsing intent", ex);
> +            }
> +
> +            BitmapUtils.getDrawable(GeckoAppShell.getContext(), aObject.optString("icon"), new BitmapUtils.BitmapLoader() {

GeckoAppShell.getContext() => mContext

Also IIRC, there's a bug with JSONObject optString() where it will return the *string* "null" if the value is null. I'd change this to `aObject.optString("icon", null)`.

@@ +680,5 @@
> +            view.setSubMenuIndicator(item.isParent);
> +            view.setMenuItemClickListener(new View.OnClickListener() {
> +                @Override
> +                public void onClick(View v) {
> +                    final GeckoActionProvider provider = GeckoActionProvider.getForType(mContext, item.intent.getType()); 

Nit: whitespace

@@ +684,5 @@
> +                    final GeckoActionProvider provider = GeckoActionProvider.getForType(mContext, item.intent.getType()); 
> +                    IntentChooserPrompt prompt = new IntentChooserPrompt(provider);
> +
> +                    prompt.show(item.label, mContext, new IntentHandler() {
> +                        public void gotIntent(Intent i, int position) {

Nit: @Override

::: mobile/android/base/widget/GeckoActionProvider.java
@@ +53,5 @@
> +    private static HashMap<String, GeckoActionProvider> mProviders = new HashMap<String, GeckoActionProvider>();
> +    public static GeckoActionProvider getForType(Context context, String type) {
> +        if (!mProviders.containsKey(type)) {
> +            GeckoActionProvider provider = new GeckoActionProvider(context);
> +            provider.setFilename("history" + type.split("/")[0] + ".xml");

I take it these history files aren't profile-specific, just like bug 940575? I guess it's not a big deal for now, but something to keep in mind if we ever allow true multi-profile support.

::: mobile/android/chrome/content/browser.js
@@ +1991,5 @@
>          matches: function(aElt, aX, aY) {
>            return this.context.matches(aElt, aX, aY);
>          },
>          getValue: function(aElt) {
> +          let obj = item;

This uses the same object reference for both item and obj, so when you change obj.label or obj.shareData below, you're permanently changing item. I think you'll want to return a cloned object like the previous patch did.

@@ +1992,5 @@
>            return this.context.matches(aElt, aX, aY);
>          },
>          getValue: function(aElt) {
> +          let obj = item;
> +          if (typeof this.label == "function")

Nit: `this.label instanceof Function` is a bit cleaner as it avoids string comparisons.

@@ +1993,5 @@
>          },
>          getValue: function(aElt) {
> +          let obj = item;
> +          if (typeof this.label == "function")
> +            obj.label = this.label(aElt); 

Nit: whitespace

@@ +1994,5 @@
>          getValue: function(aElt) {
> +          let obj = item;
> +          if (typeof this.label == "function")
> +            obj.label = this.label(aElt); 
> +          if (typeof this.shareData == "function")

Nit: instanceof

Comment 15

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

(In reply to Brian Nicholson (:bnicholson) from comment #14)

> > +    private static HashMap<String, GeckoActionProvider> mProviders = new HashMap<String, GeckoActionProvider>();
> > +    public static GeckoActionProvider getForType(Context context, String type) {
> > +        if (!mProviders.containsKey(type)) {
> > +            GeckoActionProvider provider = new GeckoActionProvider(context);
> > +            provider.setFilename("history" + type.split("/")[0] + ".xml");
> 
> I take it these history files aren't profile-specific, just like bug 940575?
> I guess it's not a big deal for now, but something to keep in mind if we
> ever allow true multi-profile support.

I am hoping we tackle the multi-profile support now. Also "history-xxx.xml" please.

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Patch 1 - Move promptlistitem to its own class

There are going to be a lot of these. I may move them to separate bugs in the end, but I want to save my place. I wound up making this into a bigger refactoring of this code that I've wanted to do for a long time. Its still not perfect, but its getting better.

Part 1 Just moves promptlistitems to their own class since they're becoming used more frequently in other contexts. I'd like them to die and be replaced with GeckoMenuItems, but that's phase 2.

Comment 17

[Wesley Johnston (:wesj)]

Attachment: Patch 2 - Add intent abilities to promptlistitem

Patch 2 - This gives promptlistitems an intent field that can be populated from JS or Java. It also hooks up the icon property partly to Javascript.

Comment 18

[Wesley Johnston (:wesj)]

Attachment: Patch 3 - Move promptlistadapter to its own class

We need to be able to show Intent prompts here, and that means we need better access to the PromptListAdapter. This just moves it out of Prompt.java (and keeps the world running in the mean time).

Comment 19

[Wesley Johnston (:wesj)]

Attachment: Patch 3 - Cleanup GeckoActionProvider

We'll also need GeckoActionProvider and MenuItemActionView to support a few new functions. This is that work. GeckoActionProvider holds a hash table of mimeType->providers to manage different providers for different menu items. MenuItemActionView is given some utility methods so that icons/titles can be set from code.

Comment 20

[Wesley Johnston (:wesj)]

Attachment: Patch 5 - Make promptlistadapter show Action views

And this hooks up the promptlistadapter to show action views if the promptlistitem asks for it.

Comment 21

[Wesley Johnston (:wesj)]

Attachment: Patch 6 - IntentChooser

This provides an intentChooserPrompt class to wrap showing lists of intent handlers in a prompt. We already did this in ActivityHandlerHelper. This is basically that code pulled into something reusable.

Comment 22

[Wesley Johnston (:wesj)]

Attachment: Patch 7 - Use the intent chooser

And this makes the ActivityHandlerHelper use the new code. ActivityHandlerHelper was actually pulling double duty, managing callbacks for intents and working as a file picker. I also pulled the file picker code into its own class here (and cleaned it up a lot).

Comment 23

[Wesley Johnston (:wesj)]

Attachment: Patch 8 - Add a tab input widget for prompts

The last of the java code. This adds a Tab box widget for prompts to use. In my dream world you could add arbitrary widgets to the tabs. In the real world this just supports lists (see, I did hold myself back in some places).

Comment 24

[Wesley Johnston (:wesj)]

Attachment: Patch 9 - Add listeners to PromptInputs

This lets the Prompt listen fro when widgets change. We need this to know that we can close the dialog when a row in the new tab widget is selected. Its hooked up to (some) other widgets as well though, and will close any time there are no buttons and an item is "selected" (for instance from a grid).

Comment 25

[Wesley Johnston (:wesj)]

Attachment: Patch 10 - Menu.jsm

This creates a new Menu.jsm module and does a direct port of the context menu code into it. No changes at all. This will not work, but its a big change so I'm trying to split it up so its readable.

Comment 26

[Wesley Johnston (:wesj)]

Attachment: Patch 11 - Make menu.jsm work

And this makes the changes that I think are needed to make menu.jsm work. Note, I haven't tested this, but saving my place here. It exposes some cross-dependecies we have in our JS code that would be nice to remove. May be hard though :(

Comment 27

[Wesley Johnston (:wesj)]

Attachment: Patch 12 - Menu.jsm refactor

This is what I was really trying to do here. Refactor this contextmenu code. This is a first attempt. Its better, but not great. It basically pulls out

Menus.context -> The normal code that addons interact with. This also holds overriden versions of some of the other code for backwards compat

Menus.selectors -> All the selectors that were formally in NativeWindow.contextmenus

Menus.utils -> We have tons of utility methods in here. I wanted to clean them up somewhere, but I think these might be better in a real Utils class...

ContextMenu() -> These are generated when a context menu is shown. Saves us having to do a lot of work clearing state in the Menu.context object, and encapsulates some more specific functions for showing a particular menu.

Like I said, still cleaning things up....

Comment 28

[Wesley Johnston (:wesj)]

Attachment: Patch 13 - Show quickshare menu items

This turns on two quick share rows in the menus.

Comment 29

[Wesley Johnston (:wesj)]

Attachment: Patch 14 - Tabs support

And this shows things in tabs based on node/scheme types.

Comment 30

[Wesley Johnston (:wesj)]

I'm splitting these patches into separate bugs. Will try to mark depencies. Note, I'm not making this a metabug. Just doing setup work to make this bug a little simpler.

Comment 31

[Wesley Johnston (:wesj)]

Attachment: Patch 2 - Add intent abilities to promptlistitem

Updated to trunk

Comment 32

[Wesley Johnston (:wesj)]

Attachment: Patch 3 - Cleanup GeckoActionProvider

Comment 33

[Wesley Johnston (:wesj)]

Attachment: Patch 5 - Make promptlistadapter show Action views

Updated to recent changes in the adapter.

Comment 34

[Wesley Johnston (:wesj)]

Attachment: Patch 12 - contextmenu refactor

Talked to mark and I'm putting off some of this refactor. This cleans up the contextmenu code though to make the code readable. Its basically splitting things up into a lot of functions.

Comment 35

[Wesley Johnston (:wesj)]

Comment on attachment 8378859 [details] [diff] [review]
Patch 2 - Add intent abilities to promptlistitem

I found some lost bits here. Pausing until I've got things cleaned up.

Comment 36

[Wesley Johnston (:wesj)]

Attachment: Patch 1/6 - Fixed for GeckoActionProvider

This provides some basic fixup for GeckoActionProvider of MenuItemActionView that are needed by the API.

Comment 37

[Wesley Johnston (:wesj)]

Attachment: Patch 2/6

This updates the PromptListAdapter to handle action views.

Comment 38

[Wesley Johnston (:wesj)]

Attachment: Patch 3/6 - Refactor JS context menu code

This is kinda a big guy. I hate our contextmenu code. This refactors it to be more readable. Its mostly moving things around into utility functions and then calling them instead.

Comment 39

[Wesley Johnston (:wesj)]

Attachment: Patch 4/6 - Turn on quicksahre in context menus

Comment 40

[Wesley Johnston (:wesj)]

Attachment: Patch 6/6 - Use tabs in context menus

Last piece (apparently there are only 5), shows tabs in context menus if they apply to multiple contexts.

Comment 41

[Brian Nicholson (:bnicholson)]

Comment on attachment 8379989 [details] [diff] [review]
Patch 1/6 - Fixed for GeckoActionProvider

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

r- for cleanup and questions.

::: mobile/android/base/GeckoAppShell.java
@@ +1190,3 @@
>                                   final String targetURI,
>                                   final String mimeType,
>                                   final String title) {

Alignment

::: mobile/android/base/prompts/PromptListItem.java
@@ +27,3 @@
>      public boolean isParent;
>      public Drawable icon;
> +    public boolean showAsActions = false;

These public fields should really all be final; having public mutable fields breaks encapsulation. Hard to do cleanly with the optional JSON bits below, so this looks like another case where bug 976249 will help out.

@@ +40,5 @@
> +            JSONObject obj = aObject.optJSONObject("shareData");
> +            if (obj != null) {
> +                String uri = obj.optString("uri");
> +                String title = obj.optString("title");
> +                String type = obj.optString("type");

Should uri and title really use optString? Will this work correctly if those values are missing?

Also note that obj.optString() returns the string "null" if the value is null. You might want to consider `obj.isNull() ? null : obj.getString(X);`

@@ +46,5 @@
> +                showAsActions = true;
> +                isParent = true;
> +            }
> +        } catch(Exception ex) {
> +            Log.i(LOGTAG, "Exception parsing intent", ex);

s/Log.i/Log.e/

@@ +50,5 @@
> +            Log.i(LOGTAG, "Exception parsing intent", ex);
> +        }
> +
> +        BitmapUtils.getDrawable(GeckoAppShell.getContext(), aObject.optString("icon"), new BitmapUtils.BitmapLoader() {
> +            public void onBitmapFound(Drawable d) {

Nit: @Override

Comment 42

[Brian Nicholson (:bnicholson)]

Comment on attachment 8379990 [details] [diff] [review]
Patch 2/6

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

::: mobile/android/base/prompts/PromptListAdapter.java
@@ +1,3 @@
>  package org.mozilla.gecko.prompts;
>  
> +import org.mozilla.gecko.menu.MenuItemActionView;

Nit: fix import order

@@ +22,5 @@
>  import android.widget.CheckedTextView;
>  import android.widget.TextView;
>  import android.widget.ListView;
> +import android.widget.ArrayAdapter;
> +import android.widget.AdapterView;

Nit: fix import order

@@ +43,5 @@
>      private static int mTopBottomTextWithIconPadding;
>      private static int mIconSize;
>      private static int mMinRowSize;
>      private static int mIconTextPadding;
> +    private static int mTextSize;

Convention for static vars is to prefix with 's' (see https://source.android.com/source/code-style.html#follow-field-naming-conventions).

@@ +63,5 @@
>              mIconSize = (int) (res.getDimension(R.dimen.prompt_service_icon_size));
>              mMinRowSize = (int) (res.getDimension(R.dimen.prompt_service_min_list_item_height));
> +
> +            float scaledDensity = res.getDisplayMetrics().scaledDensity;
> +            mTextSize = (int) (res.getDimension(R.dimen.menu_item_textsize) / scaledDensity);

Rather than converting to pixels here, can we just use the setTextSize() below that allows you to specifiy a unit (http://developer.android.com/reference/android/widget/TextView.html#setTextSize%28int,%20float%29)?

Comment 43

[Brian Nicholson (:bnicholson)]

Comment on attachment 8379993 [details] [diff] [review]
Patch 3/6 - Refactor JS context menu code

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

To make this easier to review, do you think you could split this into two patches? The first being the refactor of existing code, the second being additions to contextmenu specific to this bug? You may even want to move the refactoring patch to a different bug since it seems only tangentially related.

Comment 44

[Brian Nicholson (:bnicholson)]

Comment on attachment 8379994 [details] [diff] [review]
Patch 4/6 - Turn on quicksahre in context menus

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

Wrong patch? This looks like bug 973045.

Comment 45

[Wesley Johnston (:wesj)]

Attachment: Patch 1/6 - Fixed for GeckoActionProvider

(In reply to Brian Nicholson (:bnicholson) from comment #41)
> These public fields should really all be final; having public mutable fields
> breaks encapsulation. Hard to do cleanly with the optional JSON bits below,
> so this looks like another case where bug 976249 will help out.

I made everything final I could and moved the rest over to using getters and setters. IMO, it makes the code less readable and the encapsulation benefits aren't really worth it. I wish java had better ways to do this stuff...

> Should uri and title really use optString? Will this work correctly if those
> values are missing?

I did this intentionally with the intent of eventually allowing arbitrary Intents to be placed here. Empty strings should be fine.

> Also note that obj.optString() returns the string "null" if the value is
> null. You might want to consider `obj.isNull() ? null : obj.getString(X);`

Makes sense.

Comment 46

[Wesley Johnston (:wesj)]

Attachment: Patch 1

Comment 47

[Wesley Johnston (:wesj)]

Attachment: Patch 4/6 - Turn on quicksahre in context menus

Too much juggling.

Comment 48

[Brian Nicholson (:bnicholson)]

Comment on attachment 8381638 [details] [diff] [review]
Patch 1

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

::: mobile/android/base/menu/MenuItemActionView.java
@@ +118,5 @@
> +    }
> +
> +    public void setSubMenuIndicator(boolean hasSubMenu) {
> +        mMenuItem.setSubMenuIndicator(hasSubMenu);
> +    }

Are these changes related? You don't seem to be using any of these setters.

::: mobile/android/base/prompts/PromptListItem.java
@@ -25,5 @@
> -    public boolean selected;
> -    public Intent intent;
> -    public boolean isParent;
> -    public Drawable icon;
> -    public boolean showAsActions = false;

Confused about this patch -- what is it based on? `public boolean showAsActions = false;` isn't even in the tree now. Does this need to be folded?

@@ +26,5 @@
> +    public final boolean isParent;
> +
> +    public Intent mIntent;
> +    public boolean mSelected;
> +    public Drawable mIcon;

Since you made getters and setters for these guys, these fields should be private.

::: mobile/android/base/widget/GeckoActionProvider.java
@@ +48,5 @@
>  
> +    private static HashMap<String, GeckoActionProvider> mProviders = new HashMap<String, GeckoActionProvider>();
> +
> +    // Gets the action provider for a particular mimetype
> +    public static GeckoActionProvider getForType(String type, Context context) {

Is this supposed to be here? I didn't see it in the last iteration of this patch.

Comment 49

[Wesley Johnston (:wesj)]

Attachment: Patch 1/6 - Add intents to PromptListItem

Grr. You're right. I meant for most of that to be in here.

Comment 50

[Wesley Johnston (:wesj)]

Attachment: Patch 2/6 - Cleans up the GeckoActionProvider

And this just cleans up the GeckoActionProvider so that we can update the stored data when you select something.

Comment 51

[Wesley Johnston (:wesj)]

The getForType stuff doesn't have to be in here I guess, but it seems related enough to adding API's here that we need.

Comment 52

[Brian Nicholson (:bnicholson)]

Comment on attachment 8382455 [details] [diff] [review]
Patch 1/6 - Add intents to PromptListItem

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

::: mobile/android/base/prompts/PromptListItem.java
@@ +27,4 @@
>  
> +    public Intent mIntent;
> +    public boolean mSelected;
> +    public Drawable mIcon;

These should be private now.

Comment 53

[Wesley Johnston (:wesj)]

Attachment: fixupGeckoActionProvider

Brian noticed that I accidently marked the actionView patch obsolete. Since its the only consumer of this fixup patch, I just folded them together.

Comment 54

[Brian Nicholson (:bnicholson)]

Comment on attachment 8383331 [details] [diff] [review]
fixupGeckoActionProvider

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

This looks fine except for the same nits/comments I had in comment 42. Please fix those.

::: mobile/android/base/prompts/PromptListAdapter.java
@@ +1,3 @@
>  package org.mozilla.gecko.prompts;
>  
> +import org.mozilla.gecko.menu.MenuItemActionView;

Nit: these imports are out of order

@@ +22,5 @@
>  import android.widget.CheckedTextView;
>  import android.widget.TextView;
>  import android.widget.ListView;
> +import android.widget.ArrayAdapter;
> +import android.widget.AdapterView;

Nit: these imports are out of order

@@ +43,5 @@
>      private static int mTopBottomTextWithIconPadding;
>      private static int mIconSize;
>      private static int mMinRowSize;
>      private static int mIconTextPadding;
> +    private static int mTextSize;

So what did we decide as the new coding standard? Maybe have a part 0 to changes these from mX to X?

If you want to follow the Android conventions, this should be prefixed with 's' since it's static. With our new convention (if that's what we're going with), remove the prefix. Either way, though, 'm' isn't correct.

@@ +63,5 @@
>              mIconSize = (int) (res.getDimension(R.dimen.prompt_service_icon_size));
>              mMinRowSize = (int) (res.getDimension(R.dimen.prompt_service_min_list_item_height));
> +
> +            float scaledDensity = res.getDisplayMetrics().scaledDensity;
> +            mTextSize = (int) (res.getDimension(R.dimen.menu_item_textsize) / scaledDensity);

Rather than converting to pixels here, can we just use the setTextSize() below that allows you to specifiy a unit (http://developer.android.com/reference/android/widget/TextView.html#setTextSize%28int,%20float%29)?

Comment 55

[Brian Nicholson (:bnicholson)]

Comment on attachment 8383331 [details] [diff] [review]
fixupGeckoActionProvider

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

::: mobile/android/base/widget/GeckoActionProvider.java
@@ +43,5 @@
>      private OnTargetSelectedListener mOnTargetListener;
>  
>      private final Callbacks mCallbacks = new Callbacks();
>  
> +    private static HashMap<String, GeckoActionProvider> mProviders = new HashMap<String, GeckoActionProvider>();

Also here

Comment 56

[Brian Nicholson (:bnicholson)]

Comment on attachment 8379993 [details] [diff] [review]
Patch 3/6 - Refactor JS context menu code

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

r- for some bugs that need fixing.

::: mobile/android/chrome/content/browser.js
@@ +1926,5 @@
>      uninit: function() {
>        Services.obs.removeObserver(this, "Gesture:LongPress");
>      },
>  
> +    add: function() {

So we have some kind of wiki that describes our API for addons, right? Please make sure that gets updated with this change.

@@ +1951,5 @@
> +      item.getValue = function(aElt) {
> +        let obj = item;
> +
> +        if (typeof this.label == "function")
> +          obj.label = this.label(aElt); 

This looks like a bug. `let obj = item` above is a shallow clone, so this assignment is happening directly on item. Once getValue() is called once, the item's label gets clobbered.

If callers only need a result with label and shareData, you could just return a new object with those properties like we did before.

@@ +1958,5 @@
> +          obj.shareData = this.shareData(aElt);
> +
> +        return obj;
> +      };
> +      item.id = uuidgen.generateUUID().toString();

Any reason you're using generateUUID over the incremental _contextId we had previously? Using a UUID is heavier, and I can't think of any obvious advantages to using it.

@@ +2155,5 @@
> +        if (elt instanceof Ci.nsIDOMHTMLMenuElement) {
> +          this.menuitems = {};
> +
> +          this._addHTMLContextMenuItemsForMenu(elt);
> +          this._innerShow();

Have you tested this? I don't think this will work since these methods aren't available to 'this', which is the item itself (see use of selectedItem.callback.call() below).

@@ +2169,5 @@
> +      return {
> +        id: id,
> +        order: this.DEFAULT_HTML5_ORDER,
> +        callback: this._htmlItemClick(elt),
> +        matches: function(aElt) { return true; },

Since we're now creating this item object in multiple places (native context menu items and here), I wonder if it would be beneficial to have an actual Item class that we could instantiate instead of doing this on-the-fly object creation.

@@ +2185,1 @@
>            }

Nit: missing semicolon

@@ +2206,5 @@
> +        return true;
> +      return false;
> +    },
> +
> +    _addNativeContextMenuItems: function(element, aX, aY) {

Nit: aX/aY -> x/y

@@ +2207,5 @@
> +      return false;
> +    },
> +
> +    _addNativeContextMenuItems: function(element, aX, aY) {
> +      // then check for any context menu items registered in the ui

This comment would be better down below, after where you have "First check for any html5 context menus that might exist".

@@ +2267,5 @@
> +      let element = this._target;
> +      this.menuitems = {};
> +
> +      while (element) {
> +        let url = this._getUrl(element);

url is unused

@@ +2330,5 @@
>        // spin through the tree looking for a title for this context menu
> +      let title = this._findTitle(target);
> +
> +      this.menuitems.sort((a,b) => {
> +        if (a.order == b.order) return 0;

Nit: return on its own line

@@ +2331,5 @@
> +      let title = this._findTitle(target);
> +
> +      this.menuitems.sort((a,b) => {
> +        if (a.order == b.order) return 0;
> +        return a.order > b.order ? 1 : -1;

Nit: parens around `a.order > b.order`

@@ +2353,5 @@
> +      let menu = items;
> +      let selectedItemId = menu[data.button].id;
> +      let selectedItem = this._containsItem(selectedItemId);
> +
> +      if (!selectedItem || !selectedItem.callback) {

Nit: include `|| !selectedItem.matches` here so you can remove an indentation level from the code below.

@@ +2362,5 @@
> +        // for menuitems added using the native UI, pass the dom element that matched that item to the callback
> +        while (target) {
> +          if (selectedItem.matches(target, x, y)) {
> +            Services.console.logStringMessage("callback with " + target);
> +            selectedItem.callback.call(selectedItem, target, x, y);

If you really wanted to use selectedItem as your 'this' argument, there's no need to use .call() at all here (i.e., `selectedItem.callback(target, x, y)` would have the same effect). But I don't think you did want to use selectedItem as the 'this' target (see my comment in _htmlItemClick); instead you probably wanted NativeWindow.contextmenus.

But to simplify this, maybe the _htmlItemClick callback should be created with bind() so we don't need to use call() at all here?

Comment 57

[Brian Nicholson (:bnicholson)]

Comment on attachment 8381640 [details] [diff] [review]
Patch 4/6 - Turn on quicksahre in context menus

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

Mostly looks good. r- for the file name issue.

::: mobile/android/base/widget/GeckoActionProvider.java
@@ +52,5 @@
>      public static GeckoActionProvider getForType(String type, Context context) {
>          if (!mProviders.keySet().contains(type)) {
>              GeckoActionProvider provider = new GeckoActionProvider(context);
>  
> +            provider.setHistoryFileName("history-" + type.replace("/", "-") + ".xml");

Won't this break in certain cases? For example: "video/*"?

Rather than doing string replacements on the MIME type string given to us, perhaps we can use a hash of the string instead.

::: mobile/android/chrome/content/browser.js
@@ +526,5 @@
>        });
>  
> +    NativeWindow.contextmenus.add({
> +      label: Strings.browser.GetStringFromName("contextmenu.shareLink"),
> +      order: NativeWindow.contextmenus.DEFAULT_HTML5_ORDER-1, // Show above HTML5 menu items 

Nit: trailing whitespace

@@ +546,5 @@
> +      selector: NativeWindow.contextmenus._disableInGuest(NativeWindow.contextmenus.emailLinkContext),
> +      showAsActions: function(aElement) {
> +        try {
> +          let url = NativeWindow.contextmenus._getLinkURL(aElement);
> +          let [scheme, emailAddr] = NativeWindow.contextmenus._stripScheme(url);

Nit: scheme is unused

@@ +554,5 @@
> +            uri: emailAddr,
> +            type: "text/plain",
> +          }
> +        } catch(ex) {
> +          console.log(ex);

Shouldn't this be using the standard Cu.reportError()?

@@ +577,5 @@
> +            uri: phoneNumber,
> +            type: "text/" + scheme,
> +          }
> +        } catch(ex) {
> +          console.log(ex);

Shouldn't this be using the standard Cu.reportError()?

@@ +681,5 @@
>  
> +    NativeWindow.contextmenus.add({
> +      label: Strings.browser.GetStringFromName("contextmenu.shareImage"),
> +      selector: NativeWindow.contextmenus._disableInGuest(NativeWindow.contextmenus.imageSaveableContext),
> +      order: NativeWindow.contextmenus.DEFAULT_HTML5_ORDER-1, // Show above HTML5 menu items 

Nit: trailing whitespace

@@ -648,5 @@
>          } catch(ex) {
>             type = "";
>          }
> -        sendMessageToJava({
> -          type: "Share:Image",

We should remove the Share:Image listeners from Java since this was the only trigger.

Comment 58

[Wesley Johnston (:wesj)]

Attachment: Patch 3/6 - Refactor JS context menu code

I liked your ContextMenuItem idea. I ran with it here. I actually had a ContextMenu object in the original version of this too, but help off to avoid cluttering browser.js more. Maybe its worth just moving these to a separate file...

Comment 59

[Brian Nicholson (:bnicholson)]

Comment on attachment 8383986 [details] [diff] [review]
Patch 3/6 - Refactor JS context menu code

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

r- for some more cleanup/questions.

::: mobile/android/chrome/content/browser.js
@@ +2147,5 @@
> +    shouldShow: function() {
> +      let menu = this.menuitems;
> +      if (menu.length > 0)
> +        return true;
> +      return false;

This can all be simplified to `return this.menuitems.length > 0`.

@@ +8232,5 @@
>    },
>  };
> +
> +function ContextMenuItem(args) {
> +  this.id = uuidgen.generateUUID().toString();

Any reason to prefer UUIDs over incrementing int IDs like we had before?

@@ +8268,5 @@
> +      inGroup: this.addVal("inGroup", elt, false),
> +      disabled: this.addVal("disabled", elt, false),
> +      selected: this.addVal("selected", elt, false),
> +      isParent: this.addVal("isParent", elt, false),
> +    }

Nit: missing semicolon

@@ +8270,5 @@
> +      selected: this.addVal("selected", elt, false),
> +      isParent: this.addVal("isParent", elt, false),
> +    }
> +  }
> +}

Nit: missing semicolon

@@ +8280,5 @@
> +  this.targetElementRef = Cu.getWeakReference(target);
> +}
> +
> +HTMLContextMenuItem.prototype = {
> +  __proto__: ContextMenuItem.prototype,

Note that use of __proto__ is strongly discouraged now [1]. The proper way to do subclassing is now Object.create [2]:

HTMLContextMenuItem.prototype = Object.create(HTMLMenuItem, {
  values: {
    ...
  }
});

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/proto
[2] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/create

@@ +8291,5 @@
> +
> +  callback: function() {
> +    let elt = this.menuElementRef.get();
> +    let target = this.targetElementRef.get();
> +    if (!elt) {

Should this also check for a null target?

@@ +8297,5 @@
> +    }
> +
> +    // If this is a menu item, show a new context menu with the submenu in it
> +    if (elt instanceof Ci.nsIDOMHTMLMenuElement) {
> +      Services.console.logStringMessage("Show menu...");

Nit: drop this log

@@ +8303,5 @@
> +        NativeWindow.contextmenus.menuitems = [];
> +        NativeWindow.contextmenus._addHTMLContextMenuItemsForMenu(elt, target);
> +        NativeWindow.contextmenus._innerShow(target);
> +      } catch(ex) {
> +        Services.console.logStringMessage(ex);

Any reason you're using this over Cu.reportError?

@@ +8311,5 @@
> +      elt.click();
> +    }
> +  },
> +
> +  getValue: function(target) {

Why does the getValue() above accept an elt that it uses to determine the value, whereas this one ignores the argument altogether and uses the internal menu ref instead? Is there a way to make these more consistent?

@@ +8323,5 @@
> +    return {
> +      id: this.id,
> +      icon: elt.icon,
> +      label: elt.label,
> +      disabled: elt.disabled,

Rather than duplicating all these, could you instead call the parent ContextMenuItem's getValue(elt)?

@@ +8325,5 @@
> +      icon: elt.icon,
> +      label: elt.label,
> +      disabled: elt.disabled,
> +      menu: elt instanceof Ci.nsIDOMHTMLMenuElement
> +    }

Nit: missing semicolon

@@ +8327,5 @@
> +      disabled: elt.disabled,
> +      menu: elt instanceof Ci.nsIDOMHTMLMenuElement
> +    }
> +  }
> +}

Nit: missing semicolon

Comment 60

[Wesley Johnston (:wesj)]

Attachment: four

Updated with nits. I talked to mfinkle today and we decided to "limit" the number of providers a bit, rather than just using the mimetype directly. This still has to pass the mimetype (for the share intent). I split it up in GeckoActionProvider and:

1.) If its text/html we just use the default provider. I think the context menus and Android menu should be tied together there. Happy to undo if we want though.
2.) If its text/tel I use history-tel.xml
3.) If its text/mailto I use history-mailto.xml
4.) For all other things I use the first part of the mimetype. i.e. video/* - history-video.xml, image/* -> history-image.xml, etc.

Comment 61

[Wesley Johnston (:wesj)]

Attachment: Patch 4/6 - Turn on quicksahre in context menus

Updated again :( This also now removes the old Share:Image code. Its quite a bit more advanced that what we have here (i.e. it downloads and saves the image). I'm still trying to think of a good way to handle that here... in the mean time I think this is ok...

Comment 62

[Wesley Johnston (:wesj)]

Attachment: Patch 6/6 - Use tabs in context menus

Updated.

Comment 63

[Brian Nicholson (:bnicholson)]

Comment on attachment 8385111 [details] [diff] [review]
Patch 4/6 - Turn on quicksahre in context menus

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

::: mobile/android/base/widget/GeckoActionProvider.java
@@ +50,5 @@
>  
> +    private static String getFilenameFromMimeType(String mimeType) {
> +        String[] mime = mimeType.split("/");
> +        if (mime.length == 1) {
> +            return "history-" + mime[0] + ".xml";

How confident are we that the mimeType will always be safe to use (i.e., not contain any invalid filename chars in the first section)? Maybe we should have some basic regex check to make sure this contains only alphanumeric characters, unless you're sure that we'll always be given valid strings.

::: mobile/android/chrome/content/browser.js
@@ +525,5 @@
>        });
>  
> +    NativeWindow.contextmenus.add({
> +      label: Strings.browser.GetStringFromName("contextmenu.shareLink"),
> +      order: NativeWindow.contextmenus.DEFAULT_HTML5_ORDER-1, // Show above HTML5 menu items

Nit: space around "-"

@@ +531,5 @@
> +      showAsActions: function(aElement) {
> +        return {
> +          title: aElement.textContent.trim() || aElement.title.trim(),
> +          uri: NativeWindow.contextmenus._getLinkURL(aElement),
> +        }

Nit: missing semicolon (here and other cases below)

@@ +625,5 @@
>        });
>  
> +    NativeWindow.contextmenus.add({
> +      label: Strings.browser.GetStringFromName("contextmenu.shareMedia"),
> +      order: NativeWindow.contextmenus.DEFAULT_HTML5_ORDER-1,

Nit: space around "-"

@@ +2417,5 @@
>      },
>  
>      _stripScheme: function(aString) {
> +      let index = aString.indexOf(":");
> +      return [aString.slice(0, index), aString.slice(index + 1)];

Rather than returning both of these, can you just return the second element? I don't see anywhere where you call _stripScheme where you aren't ignoring the first result.

Comment 64

[Brian Nicholson (:bnicholson)]

Comment on attachment 8385112 [details] [diff] [review]
Patch 6/6 - Use tabs in context menus

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

r- for some cleanup/questions here.

::: mobile/android/chrome/content/browser.js
@@ +2189,4 @@
>        return false;
>      },
>  
> +    _getNodeType: function(element) {

"Type" doesn't feel like the right word here; the thing being returned is a user-facing string. Maybe getNodeLabel?

@@ +2309,5 @@
>      _getItems: function(target) {
> +      let keys = Object.keys(this.menuitems);
> +      if (keys.length == 1) {
> +        return this._getItemsInList(target, this.menuitems[keys[0]]);
> +      }

This seems unnecessarily complicated to have two different paths for N=1 and N>1. Can't we just always go through _getItemsInTabs so that callers can always expect an array of the same format as a result? This may go hand-in-hand with the "To simplify this API..." comment I made below.

@@ +2320,5 @@
> +      let itemArray = [];
> +      for (var i = 0; i < keys.length; i++) {
> +        itemArray[i] = {};
> +        itemArray[i].label = keys[i];
> +        itemArray[i].items = this._getItemsInList(target, this.menuitems[keys[i]]);

Nit: use object literal form, i.e.:

itemArray.push({
  label: ...
  items: ...
});

@@ +2352,5 @@
>  
>        // spin through the tree looking for a title for this context menu
>        let title = this._findTitle(target);
>  
> +      for (var type in this.menuitems) {

s/var/let/

@@ +2354,5 @@
>        let title = this._findTitle(target);
>  
> +      for (var type in this.menuitems) {
> +        this.menuitems[type].sort((a,b) => {
> +          if (a.order == b.order) return 0;

Nit from patch 3: Put return statement on its own line (there isn't a single place in browser.js where return is on the same line as another statement).

@@ +2370,5 @@
> +      if (useTabs) {
> +        prompt.addTabs({ items: items });
> +      } else {
> +        prompt.setSingleChoiceItems(items);
> +      }

To simplify this API, it would be nice if Prompts could account for tabs on its own rather than having to split the logic here. Could you use a single method here for passing in the items and then have Prompt look at the count of items given to it instead? Then Prompt could internally decide whether to display the items as tabs or as a single view.

@@ +2372,5 @@
> +      } else {
> +        prompt.setSingleChoiceItems(items);
> +      }
> +
> +

Nit: remove double spacing

@@ +2384,5 @@
>        }
>  
> +      let menu = items;
> +      if (data.tabs0)
> +        menu = items[data.tabs0.tab];

tabs0?

@@ +2388,5 @@
> +        menu = items[data.tabs0.tab];
> +
> +      let selectedItemId;
> +      if (data.tabs0) {
> +        selectedItemId = menu.items[data.tabs0.item].id;

At first I assumed tabs0 was a typo, but you're using it in multiple places. Why "tab0"?

Comment 65

[Wesley Johnston (:wesj)]

(In reply to Brian Nicholson (:bnicholson) from comment #64)
> "Type" doesn't feel like the right word here; the thing being returned is a
> user-facing string. Maybe getNodeLabel?
Sounds fine.

> Nit from patch 3: Put return statement on its own line (there isn't a single
> place in browser.js where return is on the same line as another statement).
Heh. There's one lambda at least: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#413 but yeah, will fix.

> To simplify this API, it would be nice if Prompts could account for tabs on
> its own rather than having to split the logic here. Could you use a single
> method here for passing in the items and then have Prompt look at the count
> of items given to it instead? Then Prompt could internally decide whether to
> display the items as tabs or as a single view.

I don't know if I like this or not. I still kinda want to expand tabs to hold whatever you inputs you want, and I still kinda want to make lists into generic input types. Then this would feel out of place. Right now though, tabs are just special lists...

> At first I assumed tabs0 was a typo, but you're using it in multiple places.
> Why "tab0"?

The prompt API allows you to put multiple inputs of the same type if you want. You can give them explicit ids too. But if you don't it generates ids (with numbers in case you append multiple ones).

Comment 66

[Wesley Johnston (:wesj)]

> Any reason to prefer UUIDs over incrementing int IDs like we had before?

Sorry, I forgot to respond here. I just always worried about incremental ID's. A programmer might assume that they'd always get the same ID (because they likely locally would all the time) and accidently hardcode. There's a possibility that our counter would be reset (especially since I didn't do anything to protect it from outside modification). Aonther nice feature is that, if we ever move where I want we might have Java creating menu items right alongside our JS ones. We wouldn't have to keep the counter in sync... but it probably does have a performance hit, and we do register a bunch of CM items at startup. Maybe we can delay that initialization...

> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/proto
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/create

I can do this, but we do use proto, even in new code, all over the place. Even Gaia uses it. I assume because its easier. Create requires (AFAIK) that you do something like:

Foo.prototype = Object.create(Bar.prototype);
Foo.prototype.method = function() { },
Foo.prototype.method2 = function() { },

or

Foo.prototype = Object.create(Bar.prototype, {
  method: { value: function() { } },
  method2: { value: function() { } },
});

> Any reason you're using this over Cu.reportError?
Habit.

> Why does the getValue() above accept an elt that it uses to determine the
> value, whereas this one ignores the argument altogether and uses the
> internal menu ref instead? Is there a way to make these more consistent?

Not really. HTML5 context menu items actually fire on a element in the page, but have second <menuitem> node that they're also tied to. The getValue calls walk the normal page DOM to find what element the context menu item is firing for. I feel like walking both chains would just lead to confusion. Is this firing on the element that's being tapped or is it firing on the context menu item associated with that element? Right now in both cases its the element that's being tapped.

Comment 67

[Wesley Johnston (:wesj)]

Attachment: Patch 3/6 - Refactor JS context menu code

(In reply to Brian Nicholson (:bnicholson) from comment #59)
> This can all be simplified to `return this.menuitems.length > 0`.
Yeah, this was just from Patch 5 where it needs to be a bit more complex.

> Should this also check for a null target?
I removed this entirely. We don't need to the use target ref here.

> Rather than duplicating all these, could you instead call the parent
> ContextMenuItem's getValue(elt)?
I don't think we can do this easily. In one case, we're calling values/functions that some add-on passed in. In this case, we're using a DOM element to determine what to show. We could probably write some sort of wrapper around the one or the other, but at that point I think the indirection gets too high.

Comment 68

[Wesley Johnston (:wesj)]

Attachment: Patch 6/6 - Use tabs in context menus

> "Type" doesn't feel like the right word here; the thing being returned is a
> user-facing string. Maybe getNodeLabel?

I swtiched to nodeLabel throughout the patch.

> This seems unnecessarily complicated to have two different paths for N=1 and
> N>1. Can't we just always go through _getItemsInTabs so that callers can
> always expect an array of the same format as a result? This may go
> hand-in-hand with the "To simplify this API..." comment I made below.

Hmm.. I don't really want to change the Prompt.jsm API to do two different things. I'd rather that callers have to be explicit to it. Can we talk this through outside this bug? 

I tried to refactor this so that just a single _setupPrompt(prompt, target) call would care about the number of items. It wound up feeling more complex to me, so I left it as is.

Comment 69

[Brian Nicholson (:bnicholson)]

Comment on attachment 8386349 [details] [diff] [review]
Patch 3/6 - Refactor JS context menu code

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

Looks good to me, just withholding r+ because I don't understand the selectedItemId line.

::: mobile/android/chrome/content/browser.js
@@ +1926,5 @@
>        Services.obs.removeObserver(this, "Gesture:LongPress");
>      },
>  
> +    add: function() {
> +      let item;

Nit: Maybe name this args?

@@ +1944,3 @@
>          throw "Menu items must have a name";
>  
> +      var cmItem = new ContextMenuItem(item);

Nit: let

@@ +2187,5 @@
> +        Services.obs.notifyObservers({target: target, x: x, y: y}, "context-menu-not-shown", "");
> +      }
> +    },
> +
> +    _getUrl: function(node, orTitle = false) {

Can you rename to _getTitle and remove the optional orTitle arg? If there's a place where we need to get the URL without the title, it would be better to make a separate _getUrl method for that which _getTitle could call as a helper.

@@ +2237,5 @@
> +      }
> +      return title;
> +    },
> +
> +    _getItems: function(target) {

Can we drop this method? It's used in only one place, and all it does is wrap _getItemsInList.

@@ +2244,5 @@
> +
> +    _getItemsInList: function(target, list) {
> +      let itemArray = [];
> +      for (let i = 0; i < list.length; i++) {
> +        var t = target;

Nit: let

@@ +2269,5 @@
>        // spin through the tree looking for a title for this context menu
> +      let title = this._findTitle(target);
> +
> +      this.menuitems.sort((a,b) => {
> +        if (a.order == b.order) return 0;

Nit: return on its own line.

@@ +2289,5 @@
> +        // prompt was cancelled
> +        return;
> +      }
> +
> +      let selectedItemId = items[data.list[0]].id;

I'm not sure I understand what's happening here. How does `items[data.list[0]]` end up being the selected item? What is `data.list` even? Why don't you need to use `data.button` anymore?

If this is correct, could you add a comment explaining this line?

@@ +8239,5 @@
>  };
> +
> +function ContextMenuItem(args) {
> +  this.id = uuidgen.generateUUID().toString();
> +  this.item = args;

Nit: Can we name this to "this.args"? "item" is being used in lots of places in this patch, and it's hard to keep track of what it means in certain situations. For example, above you have `this.items[cmItem.id] = cmItem;`, where `this.items` is a list of ContextMenuItems rather than these arg property objects. The name "args" makes it more clear that these are objects passed in directly from an external caller.

@@ +8243,5 @@
> +  this.item = args;
> +}
> +
> +ContextMenuItem.prototype = {
> +  get order() { return this.item.order || 0; },

Nit: New lines for function body

@@ +8285,5 @@
> +  this.menuElementRef = Cu.getWeakReference(elt);
> +  this.targetElementRef = Cu.getWeakReference(target);
> +}
> +
> +HTMLContextMenuItem.prototype = Object.create(ContextMenuItem.prototype, {

Since HTMLContextMenuItem and ContextMenuItem are so similar, I wish we could figure out a way to make HTMLContextMenuItem extend ContextMenuItem and share some of its implementation. It seems like every method needs to be different though...

@@ +8287,5 @@
> +}
> +
> +HTMLContextMenuItem.prototype = Object.create(ContextMenuItem.prototype, {
> +  order: { value: NativeWindow.contextmenus.DEFAULT_HTML5_ORDER },
> +  matches: { value: function(target) {

value property should start on a new line.

Also nit: add another line between order and matches.

@@ +8293,5 @@
> +      return t === target;
> +    },
> +  },
> +
> +  callback: { value: function(target) {

value property should start on its own line

@@ +8309,5 @@
> +        } catch(ex) {
> +          Cu.reportError(ex);
> +        }
> +      } else {
> +        // oltherwise just click the menu item

Nit: s/olther/other/

@@ +8315,5 @@
> +      }
> +    },
> +  },
> +
> +  getValue: { value: function(target) {

value property should start on its own line

Comment 70

[Brian Nicholson (:bnicholson)]

Comment on attachment 8386368 [details] [diff] [review]
Patch 6/6 - Use tabs in context menus

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

::: mobile/android/chrome/content/browser.js
@@ -2178,5 @@
>        return null;
>      },
>  
>      shouldShow: function() {
> -      return this.menuitems > 0;

Oops, I missed in the last review that this should have actually been `this.menuitems.length`. I guess it's fine since it's going away here anyway.

@@ +2270,4 @@
>      _buildMenu: function(x, y) {
>        // now walk up the tree and for each node look for any context menu items that apply
>        let element = this._target;
>        this.menuitems = [];

this.menuitems should be an object now, not an array. Make sure to make this change in _buildMenu too.

@@ +2385,5 @@
>        }
>  
> +      let menu = items;
> +      if (data.tabs0) {
> +        menu = items[data.tabs0.tab];

I really hate this funky auto-generated ID thing with tabs0 -- it's not clear what's happening without being familiar with how Prompt.jsm works internally.

Can you explicitly create a tabId above, pass that tabId to prompt.addTabs, and then have _promptDone accept a tabId argument? Then tabId will either be null if there are no tabs, or it will hold the id that you can use here.

Comment 71

[Wesley Johnston (:wesj)]

Attachment: Patch 1/5

I'm going to land these first 4 patches. I'll send the final review of the last patch over to someone else since brian's on vacation.

Comment 72

[Wesley Johnston (:wesj)]

Attachment: Patch 2/5

Comment 73

[Wesley Johnston (:wesj)]

Attachment: Patch 3/5

Comment 74

[Wesley Johnston (:wesj)]

Attachment: Patch 4/5

Comment 75

[Wesley Johnston (:wesj)]

Attachment: Patch 5/5

This is the last bit here, to show tabs in the dialog.

Comment 76

[Wesley Johnston (:wesj)]

Pushed the top four patches to try:

https://tbpl.mozilla.org/?tree=Try&rev=fe63ad2b49dd

All that should be changing there is ordering. The tabs patch may actually change behavior, so I sorta expect some failures there.... Pushed it to get going on that:

https://tbpl.mozilla.org/?tree=Try&rev=f56edb83446b

Comment 77

[:Margaret Leibovic]

Comment on attachment 8388909 [details] [diff] [review]
Patch 5/5

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

I'd like to see a new version here. Also it would be helpful to have a brief summary of what all these different data structures do.

::: mobile/android/base/prompts/PromptListItem.java
@@ +47,5 @@
>          } else {
>              mIntent = null;
>              showAsActions = false;
> +            // Support both "isParent" (backwards compat for older consumers), and "menu" for the new Tabbed prompt ui.
> +            isParent = aObject.optBoolean("isParent") || aObject.optBoolean("menu");

Perhaps a follow-up, but we should consider renaming this variable, now that it means something different.

::: mobile/android/chrome/content/browser.js
@@ +2171,5 @@
>        if (!this.menuitems)
>          return null;
>  
> +      for (let nodeLabel in this.menuitems) {
> +        let menu = this.menuitems[nodeLabel];

It's not intuitive that menuitems would be an array of "menus". Maybe a follow-up would be to rename this.menuitems to this.menus, or something similar.

@@ +2172,5 @@
>          return null;
>  
> +      for (let nodeLabel in this.menuitems) {
> +        let menu = this.menuitems[nodeLabel];
> +        for (let i = 0; i < menu.length; i++) {

You could use for...of here to make this a bit simpler.

Also, it is confusing that this method is called _containsItem but doesn't return a boolean. I don't see this in m-c, so I assume it was added in a previous patch here, but maybe that's something to consider.

@@ +2181,4 @@
>        return null;
>      },
>  
>      shouldShow: function() {

This would be a good time to add some comments explaining what these functions do.

@@ +2184,5 @@
>      shouldShow: function() {
> +      for (let nodeLabel in this.menuitems) {
> +        let menu = this.menuitems[nodeLabel];
> +        if (menu.length > 0)
> +          return true;

Nit: braces.

@@ +2205,5 @@
> +      } catch(ex) { }
> +
> +      // Fallback to the default
> +      return Strings.browser.GetStringFromName("browser.menu.context.default");
> +>>>>>>>

Oops, looks like some merge junk ended up in here.

@@ +2278,4 @@
>      _buildMenu: function(x, y) {
>        // now walk up the tree and for each node look for any context menu items that apply
>        let element = this._target;
> +      this.menuitems = {};

Add a comment explaining the structure of this object.

@@ +2288,2 @@
>          // then check for any context menu items registered in the ui.
> +        this._addNativeContextMenuItems(element, x, y, nodeLabel);

Instead of passing around nodeLabel between all these functions, maybe _addHTMLContextMenuItemsForElement and _addNativeContextMenuItems can be rewritten as _getFooItems so that they return the items to add, and then we can just do the _addMenuItem calls in here (or get rid of _addMenuItem and directly do the this.menuitems appending here.

@@ +2316,5 @@
>      _getItems: function(target) {
> +      let keys = Object.keys(this.menuitems);
> +      if (keys.length == 1) {
> +        return this._getItemsInList(target, this.menuitems[keys[0]]);
> +      }

This is confusing, add a comment explaining what's going on here.

@@ +2322,5 @@
> +      return this._getItemsInTabs(target);
> +    },
> +
> +    _getItemsInTabs: function(target) {
> +      let keys = Object.keys(this.menuitems);

I also can't find _getItemsInList in m-c, but it's confusing that you're passing in a list there, but in here you're using this.menuitems directly.

@@ +2330,5 @@
> +          label: keys[i],
> +          items: this._getItemsInList(target, this.menuitems[keys[i]])
> +        });
> +      }
> +      return itemArray;

If you want to be really hip here, I think you could use array comprehensions:

return [{
  label: key,
  items: this._getItemsInList(target, this.menuitems[key]
} for (key of keys)];

@@ +2362,5 @@
>        let title = this._findTitle(target);
>  
> +      for (let nodeLabel in this.menuitems) {
> +        this.menuitems[nodeLabel].sort((a,b) => {
> +          if (a.order == b.order) {

Nit: === (while we're being modern, here)

@@ +2404,5 @@
> +      if (data.tabs) {
> +        selectedItemId = menu.items[data.tabs.item].id;
> +      } else {
> +        selectedItemId = menu[data.list[0]].id
> +      }

Setting menu is only really important in the data.tabs case, so I think this would be clearer as:

let selectedItemId;
if (data.tabs) {
  let menu = items[data.tabs.tab];
  selectedItemId = menu.items[data.tabs.item].id;
} else {
  selectedItemId = items[data.list[0]].id;
}

But holy shit this is still confusing to follow. Is this even correct? This means in the data.tabs case, selectedItemId is items[data.tabs.tab].items[data.tabs.item].id.

I think I need a diagram of the data structures being used here.

Comment 78

[Wesley Johnston (:wesj)]

Attachment: Patch 5/5

I fixed up tests locally here, and wrapped some of that up here. There are two other patches that we need for them though, and I'm still working through some GB bugs.

I tried to address most of your comments here. I changed the terminology to try and make it clearer, and added lots of comments, but happy to take more ideas.

> But holy shit this is still confusing to follow. Is this even correct?

Heh. Yes? We pass back the index of the tab that was selected and the index of the item in that tab. We do pass up the GUID itself, and could pass that back, but in the original cases I used this for (<select> elements) we didn't have a GUID anyway, and it was easier to just pass back indexes. It might be safer there to generate an id -> WeakRef pair for <select> elements and then we'd be (somewhat) safer if it mutated under us while we were showing the menu.

To go over the data structures, in NativeWindow.contextmenus, we store a HashTable of ContextMenuItems: 

HashTable<String, ContextMenuItem>

When we loop over them to find ones that should be shown, we generate a new table of "contexts" and items that should be shown for them.

HashTable<String, ContextMenuItem[]>

We then have to loop through and convert those to something Prompt.jsm likes. That depends on what type of prompt we're showing, but they're both similar. i.e. If we wind up with a single context, we reformat them into an array:

PromptListItem[]

If we've got multiple contexts and need tabs, we wind up with something like:

TabDescriptor[]

where TabDescriptor is {
  String label;
  PromptListItem[]; // I'd rather this was some sort of PromptInput[], but for now I stopped here
}

We maybe could resolve that earlier and skip the intermediate HashTable<Context, ContextMenuItem[]> step, but the page technically COULD modify its DOM between the two...

When Java returns we get something like:

Single select:
  { list: [selectedIndex], button: selectedIndex } // Yes, its in there twice for backward compat reasons

Tabbed select:
  { tabs: {
      tab: selectedTab,
      index: selectedIndex
    }
  }

Note these are indicies into the passed in array though, not ID's. At some point I decided I didn't even trust that this.menus was the same as the array I sent to Prompt.jsm (depends on when I ordered the list I think), so I generally just refound the id from the PromptListItem[], or TabDescriptor[] and then used that to find the original ContextMenuItem.

Comment 79

[Wesley Johnston (:wesj)]

Attachment: Test fixes 1

This does the main test fixup. We need to ensure there are at least 2 share intents on the test machines. Otherwise, we don't show menus for them. This adds two to the robocop app. This also changed the Share dialog text from "Share via" to "Share Link/Image/Email/etc." so I updated the tests as well...

Comment 80

[Wesley Johnston (:wesj)]

Attachment: Test fixes 2

I'm still testing this a bit, but we don't currently support actionviews on pre-ICS phones entirely because we were trying to inherit from ActionProvider, even though we don't have any use for inheriting from it. i.e. I'm not sure why we do that, so I removed that restriction here. The alternative means we have to keep around two code paths everywhere. I think having one is nicer, so I'm making this work.

Will ask for r? once I know this works.

Comment 81

[Wesley Johnston (:wesj)]

Attachment: Patch 5/5

Grr. Needed to fold some more stuff in.

Comment 82

[Wesley Johnston (:wesj)]

Attachment: Patch 5/5

I am hg fail tonight.

Comment 83

[Wesley Johnston (:wesj)]

Attachment: Test fixes 1

Add missing files

Comment 84

[Geoff Brown [:gbrown] (pto Jul 1-19)]

Is there a try run with the test changes?

Comment 85

[Wesley Johnston (:wesj)]

(In reply to Geoff Brown [:gbrown] from comment #84)
> Is there a try run with the test changes?

Not quite :) Fighting one last test that passes locally and fails on the machines. I think I have a fix (I think because of the larger screen, an extraneous tap is on the dialog is off the dialog on the tegras and closes the popup). Latest run is at:

https://tbpl.mozilla.org/?tree=Try&rev=c3085330d2c6

(fingers crossed...)

Comment 86

[Wesley Johnston (:wesj)]

Still errors. I wrapped the rest of the openWebContentContextMenu calls in explicit checks to see if the menu is open first.

https://tbpl.mozilla.org/?tree=Try&rev=d1434c82b368

I've seen everything else green before so hopefully this is our last run :)

Comment 87

[Wesley Johnston (:wesj)]

Attachment: Patch

This looks good. https://tbpl.mozilla.org/?tree=Try&rev=4cae3c3c6d6a

Comment 88

[Wesley Johnston (:wesj)]

Comment on attachment 8390326 [details] [diff] [review]
Test fixes 2

This looks good on try. I'll test tomorrow when I have a GB device handy, but want to make sure this is on your (or someones...) radar.

https://tbpl.mozilla.org/?tree=Try&rev=4cae3c3c6d6a

Comment 89

[:Margaret Leibovic]

Comment on attachment 8390328 [details] [diff] [review]
Patch 5/5

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

I still want to look at this more, but here are some preliminary comments, so bnicholson can see what I've already pointed out (I asked him to also look at this patch).

::: mobile/android/base/menu/MenuItemActionView.java
@@ +12,5 @@
>  
>  import android.annotation.TargetApi;
>  import android.content.Context;
>  import android.graphics.drawable.Drawable;
> +import android.os.Build;

Not necessary?

::: mobile/android/chrome/content/browser.js
@@ +2159,3 @@
>        let htmlMenu = element.contextMenu;
>        if (!htmlMenu)
> +        return [];

Nit: braces

@@ +2171,5 @@
> +     *   menu - The <menu> element to iterate through for menuitems
> +     *   target - The target element these context menu items are attached to
> +     */
> +    _getHTMLContextMenuItemsForMenu: function(menu, target) {
> +      var items = [];

s/var/let

@@ +2205,4 @@
>      shouldShow: function() {
> +      for (let context in this.menus) {
> +        let menu = this.menus[context];
> +        Services.console.logStringMessage("Check " + context + " = " + menu.length);

Remove logging before landing.

@@ +2235,5 @@
> +    },
> +
> +    // Adds context menu items added through the add-on api
> +    _getNativeContextMenuItems: function(element, x, y) {
> +      var res = [];

s/var/let

@@ +2266,5 @@
>        Services.obs.notifyObservers(null, "before-build-contextmenu", "");
>        this._buildMenu(x, y);
>  
>        // only send the contextmenu event to content if we are planning to show a context menu (i.e. not on every long tap)
> +      Services.console.logStringMessage("Should show");

Remove this logging before landing.

@@ +2314,5 @@
>      },
>  
> +    // Adds an array of menuitems to the current list of items to show, in the correct contextType menu
> +    _addMenuItems: function(items, contextType) {
> +        if (!this.menus[contextType]) {

In other places you use the term "context" as the key for this.menus. I think we should be consistent one way or the other.

@@ +2332,5 @@
> +      // For instance, if the user taps an image inside a link, we'll have something like:
> +      // {
> +      //   link:  [ ContextMenuItem, ContextMenuItem ]
> +      //   image: [ ContextMenuItem, ContextMenuItem ]
> +      // }

Nice, this is helpful.

@@ +2342,2 @@
>          // First check for any html5 context menus that might exist...
> +        var items = this._getHTMLContextMenuItemsForElement(element);

s/var/let

@@ +2410,2 @@
>        let itemArray = [];
> +      for (let i = 0; i < keys.length; i++) {

Instead of iterating through the keys array, you could just use for...in here, like you do in other paces.

@@ +2512,5 @@
>          target = target.parentNode;
>        }
>      },
>  
> +    // Called when the contextmenu is done propgating to content. If the event wasn't cancelled, will show a contextmenu.

Typo: propagating

@@ +2519,5 @@
>        aEvent.target.ownerDocument.defaultView.removeEventListener("contextmenu", this, false);
>        this._show(aEvent);
>      },
>  
> +    // Called when a long press is observed in the NativeJava frontend. Will start the process of generating/showing a contextmenu.

Typo: native Java

Comment 90

[Brian Nicholson (:bnicholson)]

Comment on attachment 8388905 [details] [diff] [review]
Patch 3/5

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

I found some issues with the updated patch here.

::: mobile/android/chrome/content/browser.js
@@ +2190,5 @@
> +      }
> +    },
> +
> +    _getTitle: function(node) {
> +        if (orTitle && node.hasAttribute && node.hasAttribute("title")) {

orTitle doesn't exist.

Also, indentation here is wrong. Please use 2 spaces.

@@ +2197,5 @@
> +        return this._getUrl(node);
> +    },
> +
> +    _getUrl: function(node) {
> +        } else if ((node instanceof Ci.nsIDOMHTMLAnchorElement && node.href) ||

2-space indentation

@@ +8301,5 @@
> +  },
> +
> +  matches: {
> +    value: function(target) {
> +      let t = this.targetElementRef.get()

Nit: missing semicolon

Comment 91

[Wesley Johnston (:wesj)]

Attachment: Patch 3/5

Yeah, I made these fixes in Patch 4/5 I guess. Juggling multiple patches is not something I love. I'm going to land everything I can from here (including the strings).

Comment 92

[Wesley Johnston (:wesj)]

Attachment: Patch 5/5

Comment 93

[Brian Nicholson (:bnicholson)]

Comment on attachment 8391456 [details] [diff] [review]
Patch 5/5

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

::: mobile/android/chrome/content/browser.js
@@ +2192,5 @@
> +      }
> +
> +      for (let context in this.menus) {
> +        let menu = this.menus[context];
> +        for (let i = 0; i < menu.length; i++) {

Nit: for (let item of menu)

@@ +2235,5 @@
> +    },
> +
> +    // Adds context menu items added through the add-on api
> +    _getNativeContextMenuItems: function(element, x, y) {
> +      let res = [];

"res" isn't a very descriptive name; at first I thought it meant "resources", but I'm assuming it means "result". Can you change this to something else, such as "menuItems"?

@@ +2266,5 @@
>        Services.obs.notifyObservers(null, "before-build-contextmenu", "");
>        this._buildMenu(x, y);
>  
>        // only send the contextmenu event to content if we are planning to show a context menu (i.e. not on every long tap)
> +      Services.console.logStringMessage("Should show");

Drop this

@@ +2313,5 @@
>      },
>  
> +    // Adds an array of menuitems to the current list of items to show, in the correct context
> +    _addMenuItems: function(items, context) {
> +        if (!this.menus[context]) {

2-space indentation

@@ +2421,5 @@
> +     */
> +    _reformatMenuItems: function(target, menuitems) {
> +      let itemArray = [];
> +
> +      for (let i = 0; i < menuitems.length; i++) {

Nit: for (let item of menuitems)

@@ +2430,5 @@
>  
>              // hidden menu items will return null from getValue
>              if (val) {
>                itemArray.push(val);
>                break;

Should this be break, or would continue make more sense? In other words, what if an element is hidden but its parent is not?

@@ +2463,3 @@
>        let prompt = new Prompt({
>          window: target.ownerDocument.defaultView,
> +        title: useTabs ? undefined : title

This is kind of hacky, and you're doing unnecessary work to look up the title if useTabs is true. Maybe do something like this instead:

let promptArgs = { window: target.ownerDocument.defaultView };
if (!useTabs) {
  promptArgs.title = this._findTitle(target);
}
let prompt = new Prompt(promptArgs);

@@ +2487,5 @@
>  
> +      let selectedItemId;
> +      if (data.tabs) {
> +        let menu = items[data.tabs.tab];
> +        selectedItemId = menu.items[data.tabs.item].id;

Can you add a comment here making it very clear that this tabs property exists only because it's used as the ID above? You might want to consider passing the ID to _promptDone to make this even more explicit.

@@ +2490,5 @@
> +        let menu = items[data.tabs.tab];
> +        selectedItemId = menu.items[data.tabs.item].id;
> +      } else {
> +        selectedItemId = items[data.list[0]].id
> +      }

Please add a thorough comment detailing the different API/format possibilities of "data" like you did in other places. I think this is still the most confusing part of the patch.

Comment 94

[Brian Nicholson (:bnicholson)]

Comment on attachment 8391455 [details] [diff] [review]
Patch 3/5

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

::: mobile/android/chrome/content/browser.js
@@ +2192,5 @@
>            if (!SelectionHandler.startSelection(target, {
> +            mode: SelectionHandler.SELECT_AT_POINT,
> +            x: x,
> +            y: y
> +          })) { 

Nit: trailing whitespace

@@ +2269,5 @@
> +
> +            // hidden menu items will return null from getValue
> +            if (val) {
> +              itemArray.push(val);
> +              break;

Should this be break or continue? (see comment from patch 5 review)

Comment 95

[Wesley Johnston (:wesj)]

Landed the first two of these and the strings:
https://hg.mozilla.org/integration/fx-team/rev/683870122209
https://hg.mozilla.org/integration/fx-team/rev/4c8b77b42f55
https://hg.mozilla.org/integration/fx-team/rev/3122101fa40f

Comment 96

[Geoff Brown [:gbrown] (pto Jul 1-19)]

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

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

::: build/mobile/robocop/RobocopShare1.java
@@ +6,5 @@
> +
> +import android.os.Bundle;
> +import android.support.v4.app.FragmentActivity;
> +
> +public class RobocopShare1 extends FragmentActivity {

nit -- add a comment here to avoid accidental "cleanup".

::: mobile/android/base/tests/ContentContextMenuTest.java
@@ +29,5 @@
>  
>      protected void verifyContextMenuItems(String[] items) {
>          // Test that the menu items are displayed
> +        if (!mSolo.searchText(items[0])) {
> +            openWebContentContextMenu(items[0]); // Open the context menu if it is not already

nit - It would be nice if the log was explicit about whether we tried to open a menu or not. There is a dumpLog statement already in openWebContentContextMenu, but it is a little vague. Perhaps s/long-clicking at /openWebContentContextMenu long-clicking at/, or similar?

::: mobile/android/base/tests/testPictureLinkContextMenu.java
@@ +7,5 @@
>      // Test website strings
>      private static String PICTURE_PAGE_URL;
>      private static String BLANK_PAGE_URL;
>      private static final String PICTURE_PAGE_TITLE = "Picture Link";
> +    private static final String tabs [] = { "Image", "Link" };

"Image" and "Link" are short, simple strings that might occur in other contexts, like "Copy Image Location" or "Open Link in New Tab" -- is there a possibility of confusion?

Comment 97

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/683870122209
https://hg.mozilla.org/mozilla-central/rev/4c8b77b42f55
https://hg.mozilla.org/mozilla-central/rev/3122101fa40f

Comment 98

[Wesley Johnston (:wesj)]

Landed part 3 as well:

https://tbpl.mozilla.org/?tree=Try&rev=06f12e5ecb05
https://hg.mozilla.org/integration/fx-team/rev/f785999a6144

Comment 99

[Phil Ringnalda (:philor)]

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

Comment 100

[:Margaret Leibovic]

Comment on attachment 8390326 [details] [diff] [review]
Test fixes 2

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

I don't know the history of the ActionProvider implementation, but this seems reasonable to me.

::: mobile/android/base/menu/GeckoMenuItem.java
@@ +95,5 @@
>  
>      @Override
> +    public ActionProvider getActionProvider() {
> +        return null;
> +    }

You could just get rid of this method implementation, since it's not used anymore (if it was only added in API level 14, I imagine it's not required for the MenuItem implementation).

@@ +208,5 @@
>  
>      @Override
>      public MenuItem setActionProvider(ActionProvider actionProvider) {
> +        return this;
> +    }

Same goes for this method.

@@ +210,5 @@
>      public MenuItem setActionProvider(ActionProvider actionProvider) {
> +        return this;
> +    }
> +
> +    public MenuItem setActionProvider(GeckoActionProvider actionProvider) {

To minimize confusion, maybe call this setGeckoActionProvider.

Comment 101

[:Margaret Leibovic]

Comment on attachment 8391456 [details] [diff] [review]
Patch 5/5

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

I looked through this again, and I agree with the things bnicholson had to say.

::: mobile/android/chrome/content/browser.js
@@ +2341,2 @@
>          // First check for any html5 context menus that might exist...
> +        var items = this._getHTMLContextMenuItemsForElement(element);

s/var/let

Comment 102

[Wesley Johnston (:wesj)]

Attachment: GB Screenshot

Went to land this today and realized I needed to check a GB phone. Here's a screen shot there (after some small tweaks I'll post for reference). Gingerbread phones seemed to have a lot more UI variety than current ICS ones do, so I wouldn't take this as gospel everywhere. Its usable, but not great.

I'm going to reverse what I said earlier and just try to support a non-quickshare mode on GB. That ok? Any thoughts UX?

Comment 103

[Wesley Johnston (:wesj)]

Attachment: Gingerbread fixes

Comment 104

[Ian Barlow (:ibarlow)]

(In reply to Wesley Johnston (:wesj) from comment #102)
> Created attachment 8393252 [details]
> GB Screenshot
> 
> Went to land this today and realized I needed to check a GB phone. Here's a
> screen shot there (after some small tweaks I'll post for reference).
> Gingerbread phones seemed to have a lot more UI variety than current ICS
> ones do, so I wouldn't take this as gospel everywhere. Its usable, but not
> great.
>

That screenshot makes me pretty sad. 
 
> I'm going to reverse what I said earlier and just try to support a
> non-quickshare mode on GB. That ok? Any thoughts UX?

Just to clarify, are you suggesting that GB devices would just show a long single list menu, instead of these tabs? I guess that seems like a reasonable approach, unless we can reliably come up with a tabbed solution that works on all GB devices.

Comment 105

[Ian Barlow (:ibarlow)]

(and by "works" i mean "looks good")

Comment 106

[Wesley Johnston (:wesj)]

Attachment: quickshare2.png

Looking a little better. I take what I said back because I forgot about the tabs bit. I think maybe the simplest way forward is just to theme these manually. Thankfully, we have most of the images we need in tree! We can do that for GB only, or we can do this for all versions.

I want to get those quickshare icons scaled down a bit before I put this up for review. Getting dividers between them on GB may be a challenge (we'll have to add special views for them since Android wasn't smart about dividers back then). How important are they?

Comment 107

[Wesley Johnston (:wesj)]

Attachment: GB Screenshot

Comment 108

[Wesley Johnston (:wesj)]

Attachment: GB Patch

Comment 109

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

(In reply to Wesley Johnston (:wesj) from comment #107)
> Created attachment 8393986 [details]
> GB Screenshot

This looks pretty good for GB. Good enough in my opinion.

Comment 110

[Ian Barlow (:ibarlow)]

(In reply to Mark Finkle (:mfinkle) from comment #109)
> (In reply to Wesley Johnston (:wesj) from comment #107)
> > Created attachment 8393986 [details]
> > GB Screenshot
> 
> This looks pretty good for GB. Good enough in my opinion.

Can we get a handle on those GB share icon sizes at all? They're kind of all over the place, compared to the 4.0+ screenshots

Comment 111

[Wesley Johnston (:wesj)]

Attachment: GB Patch

A bunch of little patches for GB:

1.) MenuItemActionView is a LinearLayout which didn't get a constructor that took a style until ICS. I just switch to the old constructor originally, but this adds back in the missing dividers.

2.) Removes some leftover ints in Prompt.java that aren't used there anymore

3.) Set an inverse background for the dialog on list views explicitly. We removed this when we moved to light themes awhile ago. On GB, even with the light theme, Dialogs are dark though. The light background is applied automatically when you add a list to the dialog, but our new tab lists need it explicitly set it.

4.) Adjust the min row size. I unified this to one dimen, and set up different dimens for Gingerbread vs. ICS+. I added some TODOs because these should really be coming from the system theme.

5.) Use a custom tab type on GB. Still uses native on ICS.

6.) Expose menuItemShareActionButtonStyle on the GB themes.xml. That fixes up the icon styling in the menu.

7.) ActivityChooserModel used an explicit Executor. This isn't available in pre-ICS.

https://tbpl.mozilla.org/?tree=Try&rev=a8129f7b1003

Comment 112

[Brian Nicholson (:bnicholson)]

Comment on attachment 8395895 [details] [diff] [review]
GB Patch

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

This mostly looks OK to me, just a few questions.

::: mobile/android/base/menu/MenuItemActionView.java
@@ +37,5 @@
>      public MenuItemActionView(Context context, AttributeSet attrs) {
>          this(context, attrs, R.attr.menuItemActionViewStyle);
>      }
>  
>      @TargetApi(11)

Should this be bumped to 14 now given the 14 check below?

::: mobile/android/base/prompts/TabInput.java
@@ +25,4 @@
>  import android.widget.TabHost;
>  import android.widget.TabWidget;
> +import android.widget.TextView;
> +import android.widget.AdapterView;

Nit: Alphabetical ordering

::: mobile/android/base/widget/ActivityChooserModel.java
@@ +600,5 @@
>          mHistoricalRecordsChanged = false;
>          if (!TextUtils.isEmpty(mHistoryFileName)) {
> +            PersistHistoryAsyncTask task = new PersistHistoryAsyncTask();
> +            if (Build.VERSION.SDK_INT >= 11) {
> +                task.executeOnExecutor(AsyncTask.SERIAL_EXECUTOR,

This is redundant; you can just use AsyncTask#execute without the API check for the same effect. The reason is that since 11, SERIAL_EXECUTOR is used by default (http://developer.android.com/reference/android/os/AsyncTask.html#execute%28Params...%29).

@@ +605,3 @@
>                      new ArrayList<HistoricalRecord>(mHistoricalRecords), mHistoryFileName);
> +            } else {
> +                task.execute(new ArrayList<HistoricalRecord>(mHistoricalRecords), mHistoryFileName);

But pre-11, AsyncTasks used the equivalent of THREAD_POOL_EXECUTOR. So if SERIAL_EXECUTOR is essential for this code to work properly, this would be dangerous.

But looking at PersistHistoryAsyncTask, I'm not sure why this is an AsyncTask at all. AsyncTasks are designed to do some work on a background thread, then post a result to update the UI thread in onPostExecute. In PersistHistoryAsyncTask, however, we don't even override onPostExecute, and we use it only for background tasks. So can't we replace this AsyncTask altogether, replacing it with a Runnable posted to a Handler?

Comment 113

[Wesley Johnston (:wesj)]

Attachment: GB Patch

(In reply to Brian Nicholson (:bnicholson) from comment #112)
> But looking at PersistHistoryAsyncTask, I'm not sure why this is an
> AsyncTask at all. AsyncTasks are designed to do some work on a background
> thread, then post a result to update the UI thread in onPostExecute. In
> PersistHistoryAsyncTask, however, we don't even override onPostExecute, and
> we use it only for background tasks. So can't we replace this AsyncTask
> altogether, replacing it with a Runnable posted to a Handler?

This code is stolen verbatim from Android. We were trying to avoid changing it. I've made some changes where needed though. We could flip this over to use a Handler, but I don't think it fixes or makes anything better. Since this is a premade task class, I'd prefer to just leave it alone unless there's some benefit.

> This is redundant; you can just use AsyncTask#execute without the API check
> for the same effect. The reason is that since 11, SERIAL_EXECUTOR is used by
> default
> (http://developer.android.com/reference/android/os/AsyncTask.
> html#execute%28Params...%29).

I don't really care here either. I'm guess Google did this because they've flipped the execution style more than once, and wanted to be extra explicit, but I removed it and added a note about the change.

Comment 114

[Brian Nicholson (:bnicholson)]

Comment on attachment 8396757 [details] [diff] [review]
GB Patch

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

As mentioned in IRC, the advantage of using a Handler here would be to guarantee serial execution (which will not happen on <11 using an AsyncTask). Don't think this is a big enough issue to spend any significant time on, but just want to be sure we're aware of the difference in case anything crops up.

Comment 115

[Wesley Johnston (:wesj)]

Try looked good: https://tbpl.mozilla.org/?tree=Try&rev=c87151f71ebf

In:
https://hg.mozilla.org/integration/fx-team/rev/c437a98128f4
https://hg.mozilla.org/integration/fx-team/rev/4006dde253a4
https://hg.mozilla.org/integration/fx-team/rev/3f9c8979f78a
https://hg.mozilla.org/integration/fx-team/rev/05eb52f327ac
https://hg.mozilla.org/integration/fx-team/rev/9387556cc4bf

Comment 116

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c437a98128f4
https://hg.mozilla.org/mozilla-central/rev/4006dde253a4
https://hg.mozilla.org/mozilla-central/rev/3f9c8979f78a
https://hg.mozilla.org/mozilla-central/rev/05eb52f327ac
https://hg.mozilla.org/mozilla-central/rev/9387556cc4bf

Comment 117

[Francesco Lodolo [:flod]]

You're adding strings that were already there
http://hg.mozilla.org/mozilla-central/diff/05eb52f327ac/mobile/android/locales/en-US/chrome/browser.properties

Comment 118

[Wesley Johnston (:wesj)]

Thanks. Backed out the double landing:
https://hg.mozilla.org/integration/fx-team/rev/b6b144d6230c

Comment 119

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 120

[Wesley Johnston (:wesj)]

Forgot there was a leave-open here. Also, noming so that we can track this uplift to 30.

Comment 121

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4af585f4992e

Comment 122

[Wesley Johnston (:wesj)]

Comment on attachment 8388902 [details] [diff] [review]
Patch 1/5

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature
User impact if declined: No quickshare in context menus
Testing completed (on m-c, etc.): This has been on central for a few weeks. I think it (along with its fixed dependencies) are enough to move it forward.
Risk to taking this patch (and alternatives if risky): This is medium risk. Alternatives are to hold this back a release, but we've really wanted this in 30.
String or IDL/UUID changes made by this patch: None. I landed the strings in 30.

Comment 123

[Wesley Johnston (:wesj)]

Comment on attachment 8388903 [details] [diff] [review]
Patch 2/5

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature
User impact if declined: No quickshare in context menus
Testing completed (on m-c, etc.): This has been on central for a few weeks. I think it (along with its fixed dependencies) are enough to move it forward.
Risk to taking this patch (and alternatives if risky): This is medium risk. Alternatives are to hold this back a release, but we've really wanted this in 30.
String or IDL/UUID changes made by this patch: None. I landed the strings in 30.

Comment 124

[Wesley Johnston (:wesj)]

Comment on attachment 8388907 [details] [diff] [review]
Patch 4/5

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature
User impact if declined: No quickshare in context menus
Testing completed (on m-c, etc.): This has been on central for a few weeks. I think it (along with its fixed dependencies) are enough to move it forward.
Risk to taking this patch (and alternatives if risky): This is medium risk. Alternatives are to hold this back a release, but we've really wanted this in 30.
String or IDL/UUID changes made by this patch: None. I landed the strings in 30.

Comment 125

[Wesley Johnston (:wesj)]

Comment on attachment 8390326 [details] [diff] [review]
Test fixes 2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature
User impact if declined: No quickshare in context menus
Testing completed (on m-c, etc.): This has been on central for a few weeks. I think it (along with its fixed dependencies) are enough to move it forward.
Risk to taking this patch (and alternatives if risky): This is medium risk. Alternatives are to hold this back a release, but we've really wanted this in 30.
String or IDL/UUID changes made by this patch: None. I landed the strings in 30.

Comment 126

[Wesley Johnston (:wesj)]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature
User impact if declined: No quickshare in context menus
Testing completed (on m-c, etc.): This has been on central for a few weeks. I think it (along with its fixed dependencies) are enough to move it forward.
Risk to taking this patch (and alternatives if risky): This is medium risk. Alternatives are to hold this back a release, but we've really wanted this in 30.
String or IDL/UUID changes made by this patch: None. I landed the strings in 30.

Comment 127

[Wesley Johnston (:wesj)]

Comment on attachment 8391455 [details] [diff] [review]
Patch 3/5

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature
User impact if declined: No quickshare in context menus
Testing completed (on m-c, etc.): This has been on central for a few weeks. I think it (along with its fixed dependencies) are enough to move it forward.
Risk to taking this patch (and alternatives if risky): This is medium risk. Alternatives are to hold this back a release, but we've really wanted this in 30.
String or IDL/UUID changes made by this patch: None. I landed the strings in 30.

Comment 128

[Wesley Johnston (:wesj)]

Comment on attachment 8396757 [details] [diff] [review]
GB Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature
User impact if declined: No quickshare in context menus
Testing completed (on m-c, etc.): This has been on central for a few weeks. I think it (along with its fixed dependencies) are enough to move it forward.
Risk to taking this patch (and alternatives if risky): This is medium risk. Alternatives are to hold this back a release, but we've really wanted this in 30.
String or IDL/UUID changes made by this patch: None. I landed the strings in 30.

Comment 129

[Paul [pwd]]

There's still the outstanding regression as noted in bug 990642

Comment 130

[Wesley Johnston (:wesj)]

(In reply to Paul [sabret00the] from comment #129)
> There's still the outstanding regression as noted in bug 990642

Yeah. The "fix" there to restore old behaviour is pretty simple, but the drivers have been debating it a bit, and we've been looking at a better fix for it in . To go through the remaining blockers here:

Bug 953272 - Prompt.jsm: Using setMultiChoiceItems causes button to always return false regardless of the button - This is only open because of some tests.
Bug 990642 - Regression: 'Share Image' shares link and not actual image - waiting for decision from above :)
Bug 992877 - Toggling tabs quickly on the Image/Link dialog results in Link share icon shrinking - I've seen this too. Needs investigation, but I wouldn't block on it.
Bug 993407 - Swap Dialog Tabs on any shareable link - Should be a "simple" fix. Not a blocker IMO.

Comment 131

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8396757 [details] [diff] [review]
GB Patch

Am tracking a couple of the remaining bugs on this too, since we're uplifting.

Comment 132

[Lukas Blakk [:lsblakk] use ?needinfo]

a=me on all the other patches, i'm on a plane and approving them all is going to take *forever*.

Comment 133

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7dfa4d834176
https://hg.mozilla.org/releases/mozilla-aurora/rev/69727473b26b
https://hg.mozilla.org/releases/mozilla-aurora/rev/0a20665f3f61
https://hg.mozilla.org/releases/mozilla-aurora/rev/4be0743e2065
https://hg.mozilla.org/releases/mozilla-aurora/rev/3827427f7c06
https://hg.mozilla.org/releases/mozilla-aurora/rev/545cc8ee0dc2

Comment 134

[Paul [pwd]]

(In reply to Wesley Johnston (:wesj) from comment #130)
> (In reply to Paul [sabret00the] from comment #129)
> > There's still the outstanding regression as noted in bug 990642
> 
> Yeah. The "fix" there to restore old behaviour is pretty simple, but the
> drivers have been debating it a bit, and we've been looking at a better fix
> for it in . To go through the remaining blockers here:
> 
> Bug 953272 - Prompt.jsm: Using setMultiChoiceItems causes button to always
> return false regardless of the button - This is only open because of some
> tests.
> Bug 990642 - Regression: 'Share Image' shares link and not actual image -
> waiting for decision from above :)
> Bug 992877 - Toggling tabs quickly on the Image/Link dialog results in Link
> share icon shrinking - I've seen this too. Needs investigation, but I
> wouldn't block on it.
> Bug 993407 - Swap Dialog Tabs on any shareable link - Should be a "simple"
> fix. Not a blocker IMO.

I'm not sure where the discussion being held is taking place, but has there been any news Wes? With this landing on Aurora, we're ultimately just breaking what was a working feature.

Comment 135

[Wesley Johnston (:wesj)]

Comment on attachment 8388902 [details] [diff] [review]
Patch 1/5

Clearing these to make my dashboard less cluttered :)

Comment 136

[Wesley Johnston (:wesj)]

Comment on attachment 8388903 [details] [diff] [review]
Patch 2/5

Clearing these to make my dashboard less cluttered :)

Comment 137

[Wesley Johnston (:wesj)]

(In reply to Paul [sabret00the] from comment #134)
> I'm not sure where the discussion being held is taking place, but has there
> been any news Wes? With this landing on Aurora, we're ultimately just
> breaking what was a working feature.

Share Image? We're going to try and pull the image out of the Firefox cache instead, which is a larger rewrite of the feature. For 30 at least, we're just going to go with this likely.

Comment 138

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes database.

Comment 139

[Elbart]

I assume there won't be a way to turn the browser back to being a browser instead of a sharer by putting the "Share link"-entry back down to where it belongs, right?

Comment 140

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

(In reply to Elbart from comment #139)
> I assume there won't be a way to turn the browser back to being a browser
> instead of a sharer by putting the "Share link"-entry back down to where it
> belongs, right?

Not at this time. Yes, it might take a few times to re-learn the order of the menus. We will be watching for feedback, like yours, though.

Comment 141

[Teodora Vermesan (:TeoVermesan)]

Test Cases added in moztrap:
https://moztrap.mozilla.org/manage/case/12975/ Sharing a link through context menu
https://moztrap.mozilla.org/manage/case/12977/ Quickshare menu and context menu "Share link" should use same history
https://moztrap.mozilla.org/manage/case/12976/ Multiple quick share
https://moztrap.mozilla.org/manage/case/13346/ Sharing an image through context menu