788520 - NativeWindow.menu.add() broken in Android 2.x

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Andrew Zitnay]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:15.0) Gecko/20100101 Firefox/15.0
Build ID: 20120824154833

Steps to reproduce:

Our extension attempts to add a menu item to Firefox for Android via NativeWindow.menu.add().


Actual results:

On an Android 4.x device, the menu item is added to the action bar dropdown just fine.  However, on an Android 2.x device with a menu button, the menu item is only actually added if the menu has been pulled up at least once since Firefox was started.


Expected results:

The menu item should be added to the menu regardless of whether or not the menu has been pulled up since Firefox was started.  NativeWindow.menu.add() seems to return a valid menu ID in either case, so it's not possible to detect failure and try again later.  I'm guessing the problem is due to the fact that the Android menu hasn't yet been created.

Comment 1

[Matt Brubeck (:mbrubeck)]

This could explain a lot of the bug reports I've gotten from Phony users about the menu item not being added.

Comment 2

[Sriram Ramasubramanian [:sriram]]

Is there a regression range for this?

Comment 3

[Sriram Ramasubramanian [:sriram]]

Here is the problem:
1. In the case of 11+, we have our custom menu and we inflate the custom menu during startup.
2. In case of 10-, we use Android's menu and it is inflated on "first call" to menu. Now, when the menu is not initialized (not open for the first time yet), and an addon tries to add a menu item, we don't add the menu item as we don't have a menu yet.

Earlier, we used to inflate everytime we showed the menu -- which I cleaned it up.
A probable fix would be to cache the entries until menu is created for first time, and inflate it in onCreateOptionsMenu().

Comment 4

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch caches the menu items, if they are added before the menu is initialized. When the menu is initialized, they are added to the menu, and cleared.
The same variable that used to hold the list of addon menu items have been used as a cache, as the variable wasn't of any use earlier.

If the menu is already ready, the cache is not used at all (this is the default case for 11+).

Tested on 2.x and 3.0+ with:
1. Add a addon without opening menu.
2. Add a addon after opening menu atleast once.
3. Add two or more addon menu items without opening menu.
4. Remove addon.

Comment 5

[Matt Brubeck (:mbrubeck)]

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

It's kind of an edge case, but it looks like the cache will end up in the wrong state here if you add *and then remove* an add-on, all before the menu is created.  (removeAddonMenuItem does not update the cache.)

Comment 6

[Sriram Ramasubramanian [:sriram]]

That's a good catch. I'll update the patch and test it.

Comment 7

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch remove the add-on menu item from cache, if the cache has an entry for it. This had missed my testing as I always went to add-on manager through menu (so, the menu was initialized).

Comment 8

[Brian Nicholson (:bnicholson)]

Rather than caching menu items, could you just manually call onCreateOptionsMenu() if mMenu is null?

Comment 9

[Sriram Ramasubramanian [:sriram]]

(In reply to Brian Nicholson (:bnicholson) from comment #8)
> Rather than caching menu items, could you just manually call
> onCreateOptionsMenu() if mMenu is null?

We cannot call onCreateOptionsMenu() unless we know the Menu object created by Android, which is passed as argument. We cannot force feed android to use our object. Things will get messier here.

Comment 10

[Sriram Ramasubramanian [:sriram]]

The first one in inbound was a dummy changeset (accidentally :( )
This is the actual one: https://hg.mozilla.org/integration/mozilla-inbound/rev/f16404a61304

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1da958ae4a00
https://hg.mozilla.org/mozilla-central/rev/f16404a61304

Comment 12

[Aaron Train [:aaronmt]]

Andrew, can you verify on tomorrow (09/28)'s Nightly build that the fix here works?

Comment 13

[Andrew Zitnay]

The menu issue does indeed appear to be fixed on the latest Nightly.

I'm seeing a new issue on Nightly, where tapping our menu item doesn't do anything (still works fine on Aurora and lower).  I'll have to dig into our code to try to figure out why.  Probably won't be able to do that until tomorrow at the earliest, though.

Comment 14

[Andrew Zitnay]

This does indeed seem to be a bug in Nightly.  I tried the example on:

https://developer.mozilla.org/en-US/docs/Extensions/Mobile/API/NativeWindow/menu/add

In latest Aurora, the toast is shown when choosing the menu item.  In latest Nightly, it's not.

Should I open a new bug for this?

Comment 15

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

(In reply to Andrew Zitnay from comment #14)
> This does indeed seem to be a bug in Nightly.  I tried the example on:
> 
> https://developer.mozilla.org/en-US/docs/Extensions/Mobile/API/NativeWindow/
> menu/add
> 
> In latest Aurora, the toast is shown when choosing the menu item.  In latest
> Nightly, it's not.
> 
> Should I open a new bug for this?

Yes, please. I know the cause of this regression as well and I am working on a fix. The regression is not due to this bug. It's from the additional patch on bug 793136.

Comment 16

[Andrew Zitnay]

Opened as:

https://bugzilla.mozilla.org/show_bug.cgi?id=795895

Comment 17

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

I filed bug 795911 with a patch to fix the problem

Comment 18

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

(In reply to Andrew Zitnay from comment #16)
> Opened as:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=795895

Sorry Andrew, I missed this comment and filed a new bug.

Comment 19

[Matt Brubeck (:mbrubeck)]

Can we land this fix (along with bug 795911) on Aurora for Firefox 17?

Comment 20

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

(In reply to Matt Brubeck (:mbrubeck) from comment #19)
> Can we land this fix (along with bug 795911) on Aurora for Firefox 17?

Not for Aurora. Too late for that. We can ask for Beta. Note that at least one patch from bug 793136 will be needed too.

Comment 21

[Matt Brubeck (:mbrubeck)]

So... any chance of getting this fix and its dependencies uplifted to beta?  This is the #1 cause of negative reviews and support questions on AMO for add-ons like Phony.

Comment 22

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

(In reply to Matt Brubeck (:mbrubeck) from comment #21)
> So... any chance of getting this fix and its dependencies uplifted to beta? 
> This is the #1 cause of negative reviews and support questions on AMO for
> add-ons like Phony.

We would need to be careful due to the regressions/fixes.