1172831 - [Linter: UnusedResources] Remove unused resources & add fake references to prevent Linter warnings

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martyn Haigh (:mhaigh)]

In bug 1171946 I uncovered some oddities in our drawables (unused assets, assets only defined in the mdpi folder etc).  I propose that we do a full investigation of our drawable folders to weed out any upscaling or unused assets.

We want to:
- ensure that all assets are defined at the hdpi, xhdpi and xxhdpi levels (http://cyrilmottier.com/2014/08/26/putting-your-apks-on-diet/)
- find any assets which are no longer used and remove.

Comment 1

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

(In reply to Martyn Haigh (:mhaigh) from comment #0)
> - find any assets which are no longer used and remove.

You can use android lint to find unused resources – `mach gradle base:lint`. I believe this works on the code after pre-processing so no issues there.

Nick, just to verify, if we lint the base sub-module, are those resources expected to be used only in base? Or can other sub-modules use these resources?

If other sub-modules can, the results of lint my be inaccurate.

Comment 2

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Comella (:mcomella) from comment #1)
> (In reply to Martyn Haigh (:mhaigh) from comment #0)
> > - find any assets which are no longer used and remove.
> 
> You can use android lint to find unused resources – `mach gradle base:lint`.
> I believe this works on the code after pre-processing so no issues there.

I'd just like to say that this is awesome.

> Nick, just to verify, if we lint the base sub-module, are those resources
> expected to be used only in base? Or can other sub-modules use these
> resources?
> 
> If other sub-modules can, the results of lint my be inaccurate.

If sub-module A depends on sub-module B, then A can access B's resources.  However, our configuration is pretty simple, it's basically:

app > base > ... everything else.

If you want whole program analysis, lint app.  But I would expect linting base to yield all the insights.

Comment 3

[Chenxia Liu [:liuche]]

I pointed antlam to the drawables-hdpi folder, and he says he doesn't recognize a lot of those icons. There are definitely leftover icons from the onboarding-v1.5 that didn't get removed.

Comment 4

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

Android lint has some additional warnings that could be useful here:
* IconXmlAndPng (specified as both .xml and bitmap)
* IconDipSize: Icon density-independent size validation
* IconDuplicatesConfig: Identical bitmaps across various configurations
* IconDensities: Icon densities validation
* IconDuplicates: Duplicated icons under different names

Comment 5

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Chenxia Liu [:liuche] from comment #3)
> I pointed antlam to the drawables-hdpi folder, and he says he doesn't
> recognize a lot of those icons. There are definitely leftover icons from the
> onboarding-v1.5 that didn't get removed.

Yep, the 4 onboard_start_*.png images can definitely be removed.

Comment 6

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

Attachment: MozReview Request: Bug 1172831 - Remove unused color resources. r=liuche

Bug 1172831 - Remove unused onboarding resources. r?liuche

Comment 7

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

To be clear, by posting a patch here, I don't mean that I am going to work on this bug - I am just trying to take care of the easy stuff. :)

Comment 8

[Richard Newman [:rnewman]]

That's what [leave open] or filing a sub-bug are for :)

Comment 9

[Chenxia Liu [:liuche]]

Comment on attachment 8621148 [details]
MozReview Request: Bug 1172831 - Remove unused color resources. r=liuche

https://reviewboard.mozilla.org/r/10913/#review9577

Ship It!

Comment 10

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

https://hg.mozilla.org/integration/fx-team/rev/f89a37d28001

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f89a37d28001

Comment 12

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

The first item on the list, ab_search, is used in JavaScript:

https://mxr.mozilla.org/mozilla-central/search?string=ab_search&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

We'll have to trick the linter to get it to understand our JS usages, or disable the warning ( :( ).

Comment 13

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

Comment on attachment 8621148 [details]
MozReview Request: Bug 1172831 - Remove unused color resources. r=liuche

Bug 1172831 - Remove unused color resources. r=liuche

Comment 14

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

Attachment: MozReview Request: Bug 1172831 - Remove unused dimens resources. r=liuche

Bug 1172831 - Remove unused dimens resources. r=liuche

Comment 15

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

Attachment: MozReview Request: Bug 1172831 - Use static scrollbar resource reference. r=liuche

Bug 1172831 - Use static scrollbar resource reference. r=liuche

Comment 16

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

Attachment: MozReview Request: Bug 1172831 - Remove unused resources. r=liuche

Bug 1172831 - Remove unused resources. r=liuche

Comment 17

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

Attachment: MozReview Request: Bug 1172831 - Add fake references to used resources to trick Android Lint. r=liuche

Bug 1172831 - Add fake references to used resources to trick Android Lint. r=liuche

For example, these values may be used in JS only.

Comment 18

[Chenxia Liu [:liuche]]

Comment on attachment 8625599 [details]
MozReview Request: Bug 1172831 - Remove unused dimens resources. r=liuche

https://reviewboard.mozilla.org/r/11877/#review10413

Comment 19

[Chenxia Liu [:liuche]]

Comment on attachment 8625600 [details]
MozReview Request: Bug 1172831 - Use static scrollbar resource reference. r=liuche

https://reviewboard.mozilla.org/r/11879/#review10799

Ship It!

Comment 20

[Chenxia Liu [:liuche]]

Comment on attachment 8625601 [details]
MozReview Request: Bug 1172831 - Remove unused resources. r=liuche

https://reviewboard.mozilla.org/r/11881/#review10801

Ship It!

Comment 21

[Chenxia Liu [:liuche]]

Comment on attachment 8625602 [details]
MozReview Request: Bug 1172831 - Add fake references to used resources to trick Android Lint. r=liuche

https://reviewboard.mozilla.org/r/11883/#review10803

I'm not entirely sure why this is necessary, but I guess it's the same as a config/ignore file, I guess?

::: mobile/android/base/util/UnusedResourcesUtil.java:12
(Diff revision 1)
> +            R.dimen.match_parent,

Why are these unused?

Comment 22

[Chenxia Liu [:liuche]]

Comment on attachment 8621148 [details]
MozReview Request: Bug 1172831 - Remove unused color resources. r=liuche

https://reviewboard.mozilla.org/r/10913/#review10811

Ship It!

Comment 23

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

https://reviewboard.mozilla.org/r/11883/#review10803

> Why are these unused?

We added wrap_content to use and added match_parent for consistency (or vice versa), but match_parent was never actually used in our resources. Note that these are custom values to be used in different places from android's concept of "match_parent".

Comment 24

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

https://hg.mozilla.org/integration/fx-team/rev/3fa68701be17
https://hg.mozilla.org/integration/fx-team/rev/6d13ec6fac60
https://hg.mozilla.org/integration/fx-team/rev/661c1c9fac9d
https://hg.mozilla.org/integration/fx-team/rev/135789f735cc
https://hg.mozilla.org/integration/fx-team/rev/2fc104758bb5

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3fa68701be17
https://hg.mozilla.org/mozilla-central/rev/6d13ec6fac60
https://hg.mozilla.org/mozilla-central/rev/661c1c9fac9d
https://hg.mozilla.org/mozilla-central/rev/135789f735cc
https://hg.mozilla.org/mozilla-central/rev/2fc104758bb5

Comment 26

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

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

Comment 27

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

Closing in favor of the meta: bug 1195512.