Closed Bug 788520 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox for Android :: General, defect, major)

15 Branch
All
Android
defect
Not set
major

Tracking

()

RESOLVED FIXED
Firefox 18
Tracking Status
firefox16 --- affected
firefox17 --- affected
fennec 17+ ---

People

(Reporter: drew, Assigned: sriram)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
This could explain a lot of the bug reports I've gotten from Phony users about the menu item not being added.
Severity: normal → major
tracking-fennec: --- → ?
OS: Windows 7 → Android
Hardware: x86 → All
Is there a regression range for this?
Assignee: nobody → sriram
tracking-fennec: ? → 15+
tracking-fennec: 15+ → 17+
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().
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #658630 - Flags: review?(mark.finkle)
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.)
That's a good catch. I'll update the patch and test it.
Attached patch PatchSplinter Review
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).
Attachment #658630 - Attachment is obsolete: true
Attachment #658630 - Flags: review?(mark.finkle)
Attachment #658686 - Flags: review?(mark.finkle)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Rather than caching menu items, could you just manually call onCreateOptionsMenu() if mMenu is null?
(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.
Attachment #658686 - Flags: review?(mark.finkle) → review+
The first one in inbound was a dummy changeset (accidentally :( )
This is the actual one: https://hg.mozilla.org/integration/mozilla-inbound/rev/f16404a61304
https://hg.mozilla.org/mozilla-central/rev/1da958ae4a00
https://hg.mozilla.org/mozilla-central/rev/f16404a61304
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Andrew, can you verify on tomorrow (09/28)'s Nightly build that the fix here works?
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.
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
I filed bug 795911 with a patch to fix the problem
(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.
Can we land this fix (along with bug 795911) on Aurora for Firefox 17?
Depends on: 795911
(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.
Depends on: 793136
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.
(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.
You need to log in before you can comment on or make changes to this bug.