760220 - Add add-on menu items as soon as we get notification from Gecko

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sriram Ramasubramanian [:sriram]]

The add-on menu items are added only when the menu is about to be shown. This causes redundant way of going over menu items every time menu is shown. It's better to add them when the notification is received from Gecko.

Menu is initialized in startup (even for custom menu). Hence after a Gecko:Ready notification, it's safe to believe that menu is initialized and new menu items can be added.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch adds and removes addon menu items as we get the message from Gecko. The onPrepareOptionsMenu() is clean now.

Comment 2

[Matt Brubeck (:mbrubeck)]

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

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

::: mobile/android/base/GeckoApp.java
@@ +380,5 @@
> +        for (MenuItem item : sAddonMenuItems) {
> +            if (item.getItemId() == id) {
> +                sAddonMenuItems.remove(item);
> +
> +                if (sMenu == null)

I'd prefer to move everything starting from "if (sMenu == null)" to the outside of the loop, since it doesn't need to be inside it.  This doesn't make any difference functionally, but it makes it clearer to me.

Comment 3

[Sriram Ramasubramanian [:sriram]]

I thought of moving out. But then felt like having it there, as it makes sense that these things should be done when the id matches.
http://hg.mozilla.org/integration/mozilla-inbound/rev/0d736d4fa057

Comment 4

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0d736d4fa057

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #3)
> http://hg.mozilla.org/integration/mozilla-inbound/rev/0d736d4fa057

Looks like this bug's checkin (above) accidentally updated the file /build/pylib/blessings/blessings.egg-info/PKG-INFO to say "Metadata-Version 1.1" (formerly 1.0)

That change wasn't in the attached patch, but it was in the landed cset.

No idea if that's problematic & worth reverting or not.  CC'ing :gps (who added the 'blessings' code in Bug 754469) to find out.

Comment 6

[Sriram Ramasubramanian [:sriram]]

I don't know how that got added. I've never worked on those. I'm sorry. Can I backout this patch?

Comment 7

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Reverting back the PKG-INFO file.

Comment 8

[Daniel Holbert [:dholbert]]

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

Looks like this whole directory got removed (by adding it to hgignore) in bug 760094, so this doesn't matter anyway because this file no longer exists.

Sorry for the false (or no-longer-mattering) alarm.  --> Canceling review.

Comment 9

[Gregory Szorc [:gps]]

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

No need. This file was removed in bug 761105. It is OK that you didn't notice: the build system was broken by leaving this door open. You just happened to be the first person to qref and check in a patch on a tree with blessings in it.