Closed Bug 777427 Opened 8 years ago Closed 8 years ago

BrowserApp should handle its menu items

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 17
Tracking Status
firefox16 --- fixed
firefox17 --- verified
firefox18 --- verified

People

(Reporter: sriram, Unassigned)

References

Details

(Whiteboard: [qa+], [blocking-webrtandroid1+])

Attachments

(2 files)

Currently GeckoApp takes care of BrowserApp's menu items. However, this is shared by WebApp too.
The only common entry for both is "Quit", which should be handled by GeckoApp. The rest of the entries should be handled by BrowserApp or WebApp as per their need.
Attached patch PatchSplinter Review
GeckoApp handles only "Quit".
WebApp doesn't need menu at this point -- but when needed, it can be added as how addon related menu items are added to BrowserApp.
BrowserApp handles its menu items.

All gecko_menu.xml.in have been renamed to browser_app_menu.xml.in.
2 copies of gecko_app_menu.xml are added -- which contains just "Quit".

(More refactor follows).
Attachment #645820 - Flags: review?(wjohnston)
Menu addons are moved to BrowserApp as WebApp don't need them.
However the message is "Menu:Add" and "Menu:Remove" from Gecko.
If WebApp is going to use them, please move them back in to GeckoApp.
Attachment #645827 - Flags: review?(wjohnston)
This removes quit from the default BrowserApp menu
(In reply to Wesley Johnston (:wesj) from comment #3)
> This removes quit from the default BrowserApp menu

That's because Quit is from GeckoApp -- its common for both BrowserApp and WebApp -- hence the superclass adds it.
Comment on attachment 645820 [details] [diff] [review]
Patch

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

Seems good to me. I still hate having more than one browser_app_menu.xml, but that's a separate bug.

::: mobile/android/base/GeckoApp.java
@@ +104,5 @@
>  
>      private LayerController mLayerController;
>      private GeckoLayerClient mLayerClient;
>      private AbsoluteLayout mPluginContainer;
> +    protected FindInPageBar mFindInPageBar;

We can just move this to BrowserApp too. Move the initialization to initializeChrome there.
Attachment #645820 - Flags: review?(wjohnston) → review+
Attachment #645827 - Flags: review?(wjohnston) → review+
Duplicate of this bug: 773088
https://hg.mozilla.org/mozilla-central/rev/34268322c4d4
https://hg.mozilla.org/mozilla-central/rev/94d96919c763
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Whiteboard: [qa+]
Whiteboard: [qa+] → [qa+], [blocking-webrtandroid1+]
Comment on attachment 645827 [details] [diff] [review]
Patch (2/2): Menu addon refactor

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Webapps stuff
User impact if declined: Wrong menu items show up for webapps
Testing completed (on m-c, etc.): Landed on mc 3 or 4 weeks ago.
Risk to taking this patch (and alternatives if risky): Very low risk. Just moves code around.
String or UUID changes made by this patch: None
Attachment #645827 - Flags: approval-mozilla-aurora?
Comment on attachment 645820 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Webapps stuff
User impact if declined: Wrong menu items show up for webapps
Testing completed (on m-c, etc.): Landed on mc 3 or 4 weeks ago.
Risk to taking this patch (and alternatives if risky): Very low risk. Just moves code around.
String or UUID changes made by this patch: None
Attachment #645820 - Flags: approval-mozilla-aurora?
Attachment #645820 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #645827 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Indeed, the BrowerApp's menu is different, except Quit option. However I cannot check this on the latest Beta version, since I cannot install any app. Closing bug as verified fixed on the latest Nightly and Aurora builds.

--
Device: Galaxy Note
OS: Android 4.0.4
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.