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

RESOLVED FIXED in Firefox 15

Status

()

Firefox for Android
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sriram, Assigned: sriram)

Tracking

unspecified
Firefox 15
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

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.
(Assignee)

Updated

6 years ago
Blocks: 760217
(Assignee)

Comment 1

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

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]
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.
Attachment #629252 - Flags: review?(mbrubeck) → review+
(Assignee)

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.
http://hg.mozilla.org/integration/mozilla-inbound/rev/0d736d4fa057
https://hg.mozilla.org/mozilla-central/rev/0d736d4fa057
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
(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.
(Assignee)

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?
(Assignee)

Comment 7

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

Reverting back the PKG-INFO file.
Attachment #629983 - Flags: review?(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.
Attachment #629983 - Flags: review?(dholbert)

Comment 9

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