Last Comment Bug 777427 - BrowserApp should handle its menu items
: BrowserApp should handle its menu items
Status: VERIFIED FIXED
[qa+], [blocking-webrtandroid1+]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: -- normal (vote)
: Firefox 17
Assigned To: Nobody; OK to take it and work on it
:
: Sebastian Kaspari (:sebastian)
Mentors:
: 773088 (view as bug list)
Depends on:
Blocks: Blocking-FFA-WebRT1+
  Show dependency treegraph
 
Reported: 2012-07-25 10:57 PDT by Sriram Ramasubramanian [:sriram]
Modified: 2012-09-27 05:18 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
verified
verified


Attachments
Patch (21.12 KB, patch)
2012-07-25 11:04 PDT, Sriram Ramasubramanian [:sriram]
wjohnston2000: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Patch (2/2): Menu addon refactor (11.60 KB, patch)
2012-07-25 11:23 PDT, Sriram Ramasubramanian [:sriram]
wjohnston2000: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Sriram Ramasubramanian [:sriram] 2012-07-25 10:57:51 PDT
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] 2012-07-25 11:04:32 PDT
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).
Comment 2 Sriram Ramasubramanian [:sriram] 2012-07-25 11:23:13 PDT
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.
Comment 3 Wesley Johnston (:wesj) 2012-07-25 12:33:19 PDT
This removes quit from the default BrowserApp menu
Comment 4 Sriram Ramasubramanian [:sriram] 2012-07-25 12:35:37 PDT
(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) 2012-07-25 15:37:40 PDT
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 6 Wesley Johnston (:wesj) 2012-07-25 15:52:08 PDT
*** Bug 773088 has been marked as a duplicate of this bug. ***
Comment 7 Sriram Ramasubramanian [:sriram] 2012-07-26 23:57:39 PDT
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 9 Wesley Johnston (:wesj) 2012-08-23 14:44:02 PDT
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) 2012-08-23 14:44:16 PDT
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 12 Cristian Nicolae (:xti) 2012-09-27 05:18:53 PDT
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

Note You need to log in before you can comment on or make changes to this bug.