Showing quickshare in context menus means we need to know what you're sharing with. Since the default Android chooser doesn't tell us anything, we need a way to show a custom intent chooser when you tap on the "share" button. AFAICT, Android has to do the same thing with their ShareActionProvider, but they're often shown as a spinner in the actionbar. There's a version of this in GeckoActionProvider, but it uses submenus and the GeckoMenu system: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/GeckoActionProvider.java#87 I'd love to pull those together, and am starting it in bug 943568. I'm not sure how much that will give us in this direction though.

Comment on attachment 8376541 [details] [diff] [review] Patch Review of attachment 8376541 [details] [diff] [review]: ----------------------------------------------------------------- Looks good overall; r- for some cleanup and threading fixes. Also, do you have any patches based on these changes that I could refer to? It would help to piece together some of the decisions you made, such as how hasActivities() gets used, why gotIntent() returns a position, etc. ::: mobile/android/base/prompts/IntentChooserPrompt.java @@ +3,5 @@ > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +package org.mozilla.gecko.prompts; > + > +import org.mozilla.gecko.GeckoAppShell; Nit: whitespace @@ +21,5 @@ > + > +import java.util.ArrayList; > +import java.util.List; > + > +/* Nit: Use ** for JavaDoc. @@ +24,5 @@ > + > +/* > + * Shows a prompt letting the user pick from a list of intent handlers for a set of Intents or > + * for a GeckoActionProvider. Basic usage: > + * IntentChooserPrompt orompt = new IntentChooserPrompt(new Intent[] { s/orompt/prompt/ @@ +28,5 @@ > + * IntentChooserPrompt orompt = new IntentChooserPrompt(new Intent[] { > + ... // some intents > + }); > + prompt.show("Title", context, new IntentHandler() { > + public void gotIntent(Intent intent) { } Example is missing position arg. @@ +29,5 @@ > + ... // some intents > + }); > + prompt.show("Title", context, new IntentHandler() { > + public void gotIntent(Intent intent) { } > + }); Nit: Prefix these lines with "*" since we're in a comment. @@ +54,5 @@ > + */ > + public void show(final String title, final Context context, final IntentHandler handler) { > + final ArrayList<PromptListItem> items = getItems(context); > + > + if (items.size() == 0) { Nit: Use items.isEmpty() instead. @@ +56,5 @@ > + final ArrayList<PromptListItem> items = getItems(context); > + > + if (items.size() == 0) { > + Log.i(LOGTAG, "No activities for the intent chooser!"); > + handler.gotIntent(null, -1); Rather than passing null and -1 as a gotIntent callback, can we instead have a second method in IntentHandler that's called specifically for missing or cancelled providers (e.g., onProviderCancelled)? @@ +67,5 @@ > + return; > + } > + > + final Prompt prompt = new Prompt(context, new Prompt.PromptCallback() { > + public void onPromptFinished(String promptServiceResult) { Nit: @Override @@ +68,5 @@ > + } > + > + final Prompt prompt = new Prompt(context, new Prompt.PromptCallback() { > + public void onPromptFinished(String promptServiceResult) { > + if (handler == null) { Can we do this check at the beginning of the outer show() method? In other words, is there any reason to show the prompt in the first place if there's no handler for the result? @@ +80,5 @@ > + Log.e(LOGTAG, "result from promptservice was invalid: ", e); > + } > + > + if (itemId == -1) { > + handler.gotIntent(null, itemId); Also seems like a good opportunity to use a separate onProviderCancelled callback. @@ +87,5 @@ > + } > + } > + }); > + > + ThreadUtils.postToUiThread(new Runnable() { Can we just require show() to be called on the UI thread instead of posting here? @@ +88,5 @@ > + } > + }); > + > + ThreadUtils.postToUiThread(new Runnable() { > + @Override public void run() { Nit: Separate line for @Override @@ +100,5 @@ > + } > + > + // Whether or not any activities were found. Useful for checking if you should try a different Intent set > + public boolean hasActivities(Context context) { > + return getItems(context).size() > 0; Nit: Use !isEmpty() instead. This is a public method, so we need to be wary of race conditions since mItems/getItems() isn't thread-safe. If show() is guaranteed to always be called on the UI thread, you can do the same for hasActivities(), and that would be sufficient since those are the only two places that use getItems(). Otherwise, you'll probably want to synchronize getItems(). @@ +103,5 @@ > + public boolean hasActivities(Context context) { > + return getItems(context).size() > 0; > + } > + > + // Gets a list of PromptListItems for the current Intent or GeckoActionProvider set Nit: "for the" spacing @@ +106,5 @@ > + > + // Gets a list of PromptListItems for the current Intent or GeckoActionProvider set > + private ArrayList<PromptListItem> getItems(Context context) { > + if (mItems != null) > + return mItems; Actually, since getItems() appears to act as a one-time getter, any reason we can't just do getItems() immediately in IntentChooserPrompt's constructor? Then we won't need to worry about any threading issues. ::: mobile/android/base/prompts/IntentHandler.java @@ +6,5 @@ > + > +import android.content.Intent; > + > +public interface IntentHandler { > + public void gotIntent(Intent intent, int position); "gotIntent" doesn't follow the normal onXXX convention for listeners. How about onProviderSelected? Also, what is the position used for?

Updated patch.

Comment on attachment 8379979 [details] [diff] [review] Patch Review of attachment 8379979 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/prompts/IntentChooserPrompt.java @@ +24,5 @@ > +import java.util.List; > + > +/** > + * Shows a prompt letting the user pick from a list of intent handlers for a set of Intents or > + * for a GeckoActionProvider. Basic usage: Nit: Alignment on second line @@ +66,5 @@ > + > + final Prompt prompt = new Prompt(context, new Prompt.PromptCallback() { > + @Override > + public void onPromptFinished(String promptServiceResult) { > + if (handler == null) { Can we do this null check at the beginning of show() instead of this callback? In other words, is there any reason to show the prompt in the first place if there's no handler for the result? @@ +72,5 @@ > + } > + > + int itemId = -1; > + try { > + itemId = new JSONObject(promptServiceResult).getInt("button"); So we're stringifying JSON in PromptService, then parsing it immediately in Java? This is another place I would advocate stripping JSON from any parameters/return values in the Java API, and keep the JSON parsing and building local to where we actually need them. Otherwise we end up with code that moves around JSON where it's not even needed.... I filed bug 976249 -- will make a good front end topic this week!