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

RESOLVED FIXED in Firefox 15



Firefox for Android
6 years ago
6 years ago


(Reporter: sriram, Assigned: sriram)


Firefox 15
Mac OS X

Firefox Tracking Flags

(Not tracked)



(2 attachments)



6 years ago
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.


6 years ago
Blocks: 760217

Comment 1

6 years ago
Created attachment 629252 [details] [diff] [review]

This patch adds and removes addon menu items as we get the message from Gecko. The onPrepareOptionsMenu() is clean now.
Assignee: nobody → sriram
Attachment #629252 - Flags: review?(mbrubeck)
Comment on attachment 629252 [details] [diff] [review]

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

::: mobile/android/base/
@@ +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.
Attachment #629252 - Flags: review?(mbrubeck) → review+

Comment 3

6 years ago
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.
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
(In reply to Sriram Ramasubramanian [:sriram] from comment #3)

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

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

Comment 7

6 years ago
Created attachment 629983 [details] [diff] [review]

Reverting back the PKG-INFO file.
Attachment #629983 - Flags: review?(dholbert)
Comment on attachment 629983 [details] [diff] [review]

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.
Attachment #629983 - Flags: review?(dholbert)

Comment 9

6 years ago
Comment on attachment 629983 [details] [diff] [review]

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.
You need to log in before you can comment on or make changes to this bug.