696324 - Provide JS API for adding items to the Android menu

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 1

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

Comment on attachment 568607 [details] [diff] [review]
patch


>diff --git a/embedding/android/GeckoAppShell.java b/embedding/android/GeckoAppShell.java

>+    static class ExtraMenuItem implements MenuItem.OnMenuItemClickListener {
>+        String label;
>+        int id;
>+        public boolean 	onMenuItemClick(MenuItem item) {

TAB snuck in

>+            GeckoAppShell.sendEventToGecko(new GeckoEvent("AndroidMenuClicked", Integer.toString(id)));

We have been using  a different format for event names: "android-menu-click"

>+            return true;
>+        }
>+    }
>+
>+    static Vector<ExtraMenuItem> sExtraMenuItems = new Vector<ExtraMenuItem>();
>+    static int sExtraMenuItemIDs = 0;
>+
>     // A looper thread, accessed by GeckoAppShell.getHandler
>     private static class LooperThread extends Thread {
>         public SynchronousQueue<Handler> mHandlerQueue =
>@@ -1573,7 +1586,20 @@ public class GeckoAppShell
>             final JSONObject geckoObject = json.getJSONObject("gecko");
>             String type = geckoObject.getString("type");
> 
>-            if (type.equals("DOMContentLoaded")) {
>+            Log.i("GeckoMessage", "type: " + type);
>+
>+            if (type.equals("addMenuItem")) {

"addMenuItem" seems pretty generic for a global event name. I suppose it's fine for now, but we might want to use a convention when we start adding more UI base events. Maybe something like "Native:MenuAdd"

Heck, I'd be OK with using that format for the other event - "Native:MenuClick"  (I really don't like "foo-bar-event" myself)

>+                ExtraMenuItem e = new ExtraMenuItem();
>+                e.id = ++sExtraMenuItemIDs;

Let's generate the ID in JavaScript and send it over with the JSON. We do that for some other stuff too (Tabs) so keeping it similar makes me feel better.

>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js

>+var Window = {

Maybe we should call this "NativeWindow" to avoid confusion with the JS window

>+    },
>+    _menuCallbacks: [],

Add a blank line before the _menuCallbacks, but none after it (keeps the menu stuff together)

>+    addMenuItem: function(name, func) {
>+	var bridge = Cc["@mozilla.org/android/bridge;1"].getService(Ci.nsIAndroidBridge);
>+	var id = bridge.handleGeckoMessage(JSON.stringify({ gecko: {type: "addMenuItem", name: name }}));

Switch to the normal sendMessageToJava
Use let instead for var

Comment 2

[[:fabrice] Fabrice Desré]

Comment on attachment 568607 [details] [diff] [review]
patch

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

We should also add a couple of improvements:
- add an icon to the menu
- allow the removal of an item (must have for restartless addons)

::: embedding/android/GeckoApp.java
@@ +367,5 @@
> +        Iterator<GeckoAppShell.ExtraMenuItem> i = 
> +            GeckoAppShell.sExtraMenuItems.iterator();
> +        while (i.hasNext()) {
> +            GeckoAppShell.ExtraMenuItem emi = i.next();
> +            MenuItem mi = menu.add(emi.label);

this means that you will add the extra menu items each time the menu is open, and will end up with duplicates.

::: mobile/chrome/content/browser.js
@@ +49,5 @@
> +    },
> +    _menuCallbacks: [],
> +    addMenuItem: function(name, func) {
> +	var bridge = Cc["@mozilla.org/android/bridge;1"].getService(Ci.nsIAndroidBridge);
> +	var id = bridge.handleGeckoMessage(JSON.stringify({ gecko: {type: "addMenuItem", name: name }}));

handleGeckoMessage returns void. We need to create the id here, and pass it to the java layer.

Comment 3

[[:fabrice] Fabrice Desré]

Attachment: updated patch

Addresses comments from the previous patch.

There's a bit of testing code : tapping 3 times on the new "hello" menu entry will make it go away.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Fabrice Desré [:fabrice] from comment #2)
> Comment on attachment 568607 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> Review of attachment 568607 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> We should also add a couple of improvements:
> - add an icon to the menu
> - allow the removal of an item (must have for restartless addons)
good follow ups
> ::: embedding/android/GeckoApp.java
> @@ +367,5 @@
> > +        Iterator<GeckoAppShell.ExtraMenuItem> i = 
> > +            GeckoAppShell.sExtraMenuItems.iterator();
> > +        while (i.hasNext()) {
> > +            GeckoAppShell.ExtraMenuItem emi = i.next();
> > +            MenuItem mi = menu.add(emi.label);
> 
> this means that you will add the extra menu items each time the menu is
> open, and will end up with duplicates.
no, this is called when the menu is constructed and a blank menu is passed in each time
> ::: mobile/chrome/content/browser.js
> @@ +49,5 @@
> > +    },
> > +    _menuCallbacks: [],
> > +    addMenuItem: function(name, func) {
> > +	var bridge = Cc["@mozilla.org/android/bridge;1"].getService(Ci.nsIAndroidBridge);
> > +	var id = bridge.handleGeckoMessage(JSON.stringify({ gecko: {type: "addMenuItem", name: name }}));
> 
> handleGeckoMessage returns void. We need to create the id here, and pass it
> to the java layer.
Wes's prompt service patch changes it to return a string. This patch is dependant on his patch for that change

Comment 5

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

Comment on attachment 568802 [details] [diff] [review]
updated patch

Comments:

* Brad mentioned the onCreateOptionsMenu difference from onPrepareOptionsMenu. I guess for removing items, we could use the onPrepareOptionsMenu approach. Also would support dynamic hide/show in future.

Let's start a more standard convention for Java/Js messages
* "addMenuItem" -> "Native:MenuAdd" 
* "removeMenuItem" -> "Native:MenuRemove"

Let's prepare for more Native features
* NativeWindow.menu.add(...)
* NativeWindow.menu.remove(...)

So add a "menu" sub object to the NativeWindow object. NativeWindow could still be the nsIObserver for now. Move _menuCallbacks to menu._callbacks

nits:
* Not sure we really want to add images to menus. Do you thinks it's proper for Android? If we remove it for now, it's less complexity and we could always add it later.
* NativeWindow.shutdown() -> NativeWindow.unint()  (to match naming conventions)

r- for changes, but looks solid to me.

Comment 6

[[:fabrice] Fabrice Desré]

Attachment: updated patch

> Brad mentioned the onCreateOptionsMenu difference from onPrepareOptionsMenu. I guess for  removing items, we could use the onPrepareOptionsMenu approach. Also would support dynamic hide/show in future.

I didn't do this in this patch. I'm not sure we want to maintain an "items to remove" list just for this.

Comment 7

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

Comment on attachment 569125 [details] [diff] [review]
updated patch


>diff --git a/embedding/android/GeckoApp.java b/embedding/android/GeckoApp.java

>+    public boolean onPrepareOptionsMenu(Menu aMenu)

>+                    else {
>+                        try {
>+                            URL url = new URL(item.icon);
>+                            InputStream is = (InputStream) url.getContent();
>+                            Drawable drawable = Drawable.createFromStream(is, "src");
>+                            mi.setIcon(drawable);
>+                        } catch(Exception e) {
>+                            Log.e("Gecko", "onPrepareOptionsMenu: Unable to set icon", e);
>+                        }

This frightens me. We either need to move the code into a background task or remove it for now and are-add it in a followup. See the favicon handling for an example of the background thread. Also, being able to add any resource directly from the Internet seems like overkill.

(I favor removing it for now and adding it back later in a new patch/bug)

>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js
>+    
>+    let called = 0;
>+    let icon = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAYAAABzenr0AAAAGXRFWHRTb2Z0d2FyZQBBZG9iZSBJbWFnZVJlYWR5ccllPAAAA2ZpVFh0WE1MOmNvbS5hZG9iZS54bXAAAAAAADw
>+    let menuId = NativeWindow.menu.add("Hello", icon, function() {
>+        dump("Gecko called from menu " + menuId);
>+        called++;
>+        if (called == 3)
>+            NativeWindow.menu.remove(menuId);
>+    });
>+    dump("Gecko menuId=" + menuId);

Remove this :)

r+ with nits fixed and icon loading from URL removed form this patch (add it to a new one)

Comment 8

[[:fabrice] Fabrice Desré]

Attachment: updated patch

Sorry about asking for review again. Changes:
- refactored to use the GeckoEventListener pattern
- icons can be |data:|, |jar:| or |file://| uris. The |jar:| and |file://| ones are loaded in a background thread.

Comment 9

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

Comment on attachment 569225 [details] [diff] [review]
updated patch

Two things:
* posting a runnable to the main handler doesn't download the image in a background thread (I know, I did that too). Here is how Gian-Carlo fixed my code:
http://hg.mozilla.org/projects/birch/rev/1685afdada81

* As we keep getting consistent with evnet/message names, I think "Menu:Add", "Menu:Remove" and "Menu:Clicked" would be better names.

r- for the background download, but with the code from the changeset, you should be ready to go in the next patch.

Comment 10

[[:fabrice] Fabrice Desré]

Attachment: updated patch

Changes:
- uses Menu:Add, Menu:remove and Menu:Clicked
- uses GeckoAppsShell.getHandler() to not post the runnable on the main thread

Comment 11

[[:fabrice] Fabrice Desré]

pushed:
http://hg.mozilla.org/projects/birch/rev/50835832a726