BrowserApp should handle its menu items

VERIFIED FIXED in Firefox 16

Status

()

Firefox for Android
General
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: sriram, Unassigned)

Tracking

unspecified
Firefox 17
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox16 fixed, firefox17 verified, firefox18 verified)

Details

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

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Created attachment 645820 [details] [diff] [review]
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).
Attachment #645820 - Flags: review?(wjohnston)
(Reporter)

Comment 2

5 years ago
Created attachment 645827 [details] [diff] [review]
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.
Attachment #645827 - Flags: review?(wjohnston)
This removes quit from the default BrowserApp menu
(Reporter)

Comment 4

5 years ago
(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
Blocks: 766259
(Reporter)

Comment 7

5 years ago
Pushed to inbound with required changes:
http://hg.mozilla.org/integration/mozilla-inbound/rev/34268322c4d4
http://hg.mozilla.org/integration/mozilla-inbound/rev/94d96919c763
https://hg.mozilla.org/mozilla-central/rev/34268322c4d4
https://hg.mozilla.org/mozilla-central/rev/94d96919c763
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17

Updated

5 years ago
status-firefox16: --- → affected
Whiteboard: [qa+]

Updated

5 years ago
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/e11642a5a509
https://hg.mozilla.org/releases/mozilla-aurora/rev/905a439a5717

Updated

5 years ago
status-firefox16: affected → fixed
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
status-firefox17: --- → verified
status-firefox18: --- → verified
You need to log in before you can comment on or make changes to this bug.