777427 - BrowserApp should handle its menu items

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sriram Ramasubramanian [:sriram]]

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.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

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

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (2/2): Menu addon refactor

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.

Comment 3

[Wesley Johnston (:wesj)]

This removes quit from the default BrowserApp menu

Comment 4

[Sriram Ramasubramanian [:sriram]]

(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 5

[Wesley Johnston (:wesj)]

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.

Comment 7

[Sriram Ramasubramanian [:sriram]]

Pushed to inbound with required changes:
http://hg.mozilla.org/integration/mozilla-inbound/rev/34268322c4d4
http://hg.mozilla.org/integration/mozilla-inbound/rev/94d96919c763

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/34268322c4d4
https://hg.mozilla.org/mozilla-central/rev/94d96919c763

Comment 9

[Wesley Johnston (:wesj)]

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

Comment 10

[Wesley Johnston (:wesj)]

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

Comment 11

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e11642a5a509
https://hg.mozilla.org/releases/mozilla-aurora/rev/905a439a5717

Comment 12

[Cristian Nicolae (:xti)]

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