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.

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 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.

I think Wes is taking a look at this.

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

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?

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.

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.

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 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 :)

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 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

(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.

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.

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.

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).

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.

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

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.

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).

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).

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).

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.

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 :(

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....

This turns on two quick share rows in the menus.

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

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.

Updated to trunk

Updated to recent changes in the adapter.

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 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.

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

This updates the PromptListAdapter to handle action views.

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.

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

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 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 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 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.

(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.

Too much juggling.

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.

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

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

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 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.

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 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 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 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 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.

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 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

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.

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...

Updated.

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 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"?

(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).

> 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.

(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.

> "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 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 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.

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.

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

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 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.

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.

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...

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.

Grr. Needed to fold some more stuff in.

I am hg fail tonight.

Add missing files

Is there a try run with the test changes?

(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...)

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 :)

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

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 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 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

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 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 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)

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 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?

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

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

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 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

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?

(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.

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

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?

(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.

(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

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 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?

(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 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.

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

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

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

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

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

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 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 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 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 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 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 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.

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

(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 on attachment 8396757 [details] [diff] [review] GB Patch Am tracking a couple of the remaining bugs on this too, since we're uplifting.

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

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

(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 on attachment 8388902 [details] [diff] [review] Patch 1/5 Clearing these to make my dashboard less cluttered :)

Comment on attachment 8388903 [details] [diff] [review] Patch 2/5 Clearing these to make my dashboard less cluttered :)

(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.

Added to the release notes database.

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?

(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.

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