1195517 - Remove unused menu drawables

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from bug 1172831 comment #26)
> liuche pointed out that while some icons are used in code, e.g. [1], they
> are not actually used in the application. In this case, we used to show
> icons in the menu on v11 but we no longer do and did not remove the icons.
> 
> We still set the icon in the code, but just don't show the View which keeps
> the icons in memory. If we just remove the icons, we'd be using the < v11
> assets and still be using memory - it may be better to use null resource
> aliases so the icon is always set as null. We can add a comment in the file.
> 
> [1]:
> https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> drawable-xhdpi-v11/ic_menu_desktop_mode_on.png

Not quite accurate - an action_menu_item_view is composed of a MenuItemDefault and a MenuItemActionBar [0]. The MenuItemDefault is a TextView, which is used when the menu item is the only item on its row [0.5]. The TextView can show icons via compound drawables but this behavior is overruled by GeckoMenu, which determines if the icons will be shown for a list item [1]. By default, mShowIcons is false (i.e. the Java default) and this is overturned when a menu item has a submenu with an ActionProvider [1] – i.e. the Share button.

MenuItemActionBar is just an icon and is used when an item does not solely occupy the row (e.g. back, forward).

We should remove the icons on all MenuItemDefaults (i.e. single row items).

[0]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/menu_item_action_view.xml
[0.5]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/menu/MenuItemActionView.java?rev=a9738e5f636a#57
[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/menu/GeckoMenu.java?rev=28d346b0142c#829
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/menu/GeckoMenu.java?rev=28d346b0142c#632

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1195517 - Remove simple unused menu drawables. r=liuche

Bug 1195517 - Remove simple unused menu drawables. r=liuche

On GB devices, ic_menu_new_*tab is in the base menu, unlike the other
configurations, so it stayed on GB.

More complex removals to follow.

I tested this by clicking on all of the base menu items on a Lollipop phone,
KitKat large tablet, and a GB phone.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1195517 - Remove unused desktop_mode menu icons. r=liuche

Bug 1195517 - Remove unused desktop_mode menu icons. r=liuche

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

~21.3KB. :)

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1195517 - Remove unused ic_menu_addons_filler. r=liuche

Bug 1195517 - Remove unused ic_menu_addons_filler. r=liuche

This also removes code to set an icon from an addon, which is unused because,
afaik, addons' icons are also hidden. iirc, there was a bug open on whether we
want addons to be able to show icons (e.g. could be used to show status), but
we can reimplement this if that bug is ever decided.

I was not able to test this with an addon that sets an icon but I did test that
the application did not crash with no addons installed.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Make that ~24.2 KB.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1195517 - Remove unused ic_menu_quit from gecko_app. r=liuche

Bug 1195517 - Remove unused ic_menu_quit from gecko_app. r=liuche

This doesn't appear in Fennec builds and I don't know which web apps "quit"
could appear in so I didn't test this in a web app. I can confirm no issues
with a typical Fennec build, however.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

And ~26.3 KB.

Comment 8

[Chenxia Liu [:liuche]]

Comment on attachment 8648985 [details]
MozReview Request: Bug 1195517 - Remove simple unused menu drawables. r=liuche

https://reviewboard.mozilla.org/r/16333/#review15443

Ship It!

Comment 9

[Chenxia Liu [:liuche]]

Comment on attachment 8648986 [details]
MozReview Request: Bug 1195517 - Remove unused desktop_mode menu icons. r=liuche

https://reviewboard.mozilla.org/r/16335/#review15445

Ship It!

Comment 10

[Chenxia Liu [:liuche]]

Comment on attachment 8648993 [details]
MozReview Request: Bug 1195517 - Remove unused ic_menu_addons_filler. r=liuche

https://reviewboard.mozilla.org/r/16337/#review15447

Ship It!

Comment 11

[Chenxia Liu [:liuche]]

Comment on attachment 8648998 [details]
MozReview Request: Bug 1195517 - Remove unused ic_menu_quit from gecko_app. r=liuche

https://reviewboard.mozilla.org/r/16339/#review15449

Ship It!

Comment 12

[Chenxia Liu [:liuche]]

Sorry this took so long!

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

url:        https://hg.mozilla.org/integration/fx-team/rev/8ddc5d9ca2f5685ba0000a11eb6452b7cc97de44
changeset:  8ddc5d9ca2f5685ba0000a11eb6452b7cc97de44
user:       Michael Comella <[e-mail]>
date:       Mon Aug 17 14:50:58 2015 -0700
description:
Bug 1195517 - Remove simple unused menu drawables. r=liuche

On GB devices, ic_menu_new_*tab is in the base menu, unlike the other
configurations, so it stayed on GB.

More complex removals to follow.

I tested this by clicking on all of the base menu items on a Lollipop phone,
KitKat large tablet, and a GB phone.

url:        https://hg.mozilla.org/integration/fx-team/rev/47b183f5c740d259bec4deb39cd76d1dc4864cd7
changeset:  47b183f5c740d259bec4deb39cd76d1dc4864cd7
user:       Michael Comella <[e-mail]>
date:       Mon Aug 17 15:11:13 2015 -0700
description:
Bug 1195517 - Remove unused desktop_mode menu icons. r=liuche

url:        https://hg.mozilla.org/integration/fx-team/rev/27be4ca1e6c9602b9c6aa718700e137e3f4f8361
changeset:  27be4ca1e6c9602b9c6aa718700e137e3f4f8361
user:       Michael Comella <[e-mail]>
date:       Mon Aug 17 15:29:52 2015 -0700
description:
Bug 1195517 - Remove unused ic_menu_addons_filler. r=liuche

This also removes code to set an icon from an addon, which is unused because,
afaik, addons' icons are also hidden. iirc, there was a bug open on whether we
want addons to be able to show icons (e.g. could be used to show status), but
we can reimplement this if that bug is ever decided.

I was not able to test this with an addon that sets an icon but I did test that
the application did not crash with no addons installed.

url:        https://hg.mozilla.org/integration/fx-team/rev/4878b6a9ff7d9cdeb34d1dd45b3a1dddc56367ba
changeset:  4878b6a9ff7d9cdeb34d1dd45b3a1dddc56367ba
user:       Michael Comella <[e-mail]>
date:       Mon Aug 17 15:56:53 2015 -0700
description:
Bug 1195517 - Remove unused ic_menu_quit from gecko_app. r=liuche

This doesn't appear in Fennec builds and I don't know which web apps "quit"
could appear in so I didn't test this in a web app. I can confirm no issues
with a typical Fennec build, however.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ddc5d9ca2f5
https://hg.mozilla.org/mozilla-central/rev/47b183f5c740
https://hg.mozilla.org/mozilla-central/rev/27be4ca1e6c9
https://hg.mozilla.org/mozilla-central/rev/4878b6a9ff7d