Closed Bug 1172831 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mhaigh, Assigned: mcomella)

References

(Blocks 3 open bugs)

Details

Attachments

(5 files)

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.
(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.
Flags: needinfo?(nalexander)
(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.
Flags: needinfo?(nalexander)
Blocks: fatfennec
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.
Assignee: mhaigh → nobody
Summary: Sanity check our drawables to make sure we're not upscaling anything or including unused assets → [Linter: UnusedResources] Sanity check our drawables to make sure we're not upscaling anything or including unused assets
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
(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.
Bug 1172831 - Remove unused onboarding resources. r?liuche
Attachment #8621148 - Flags: review?(liuche)
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. :)
That's what [leave open] or filing a sub-bug are for :)
Whiteboard: [leave-open]
Comment on attachment 8621148 [details]
MozReview Request: Bug 1172831 - Remove unused color resources. r=liuche

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

Ship It!
Attachment #8621148 - Flags: review?(liuche) → review+
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 on attachment 8621148 [details]
MozReview Request: Bug 1172831 - Remove unused color resources. r=liuche

Bug 1172831 - Remove unused color resources. r=liuche
Attachment #8621148 - Attachment description: MozReview Request: Bug 1172831 - Remove unused onboarding resources. r?liuche → MozReview Request: Bug 1172831 - Remove unused color resources. r=liuche
Attachment #8621148 - Flags: review+ → review?(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.
Attachment #8625602 - Flags: review?(liuche)
Comment on attachment 8625599 [details]
MozReview Request: Bug 1172831 - Remove unused dimens resources. r=liuche

https://reviewboard.mozilla.org/r/11877/#review10413
Attachment #8625599 - Flags: review?(liuche) → review+
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!
Attachment #8625600 - Flags: review?(liuche) → review+
Comment on attachment 8625601 [details]
MozReview Request: Bug 1172831 - Remove unused resources. r=liuche

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

Ship It!
Attachment #8625601 - Flags: review?(liuche) → review+
Attachment #8625602 - Flags: review?(liuche) → review+
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?
Attachment #8621148 - Flags: review?(liuche) → review+
Comment on attachment 8621148 [details]
MozReview Request: Bug 1172831 - Remove unused color resources. r=liuche

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

Ship It!
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".
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
Closing in favor of the meta: bug 1195512.
Assignee: nobody → michael.l.comella
Summary: [Linter: UnusedResources] Sanity check our drawables to make sure we're not upscaling anything or including unused assets → [Linter: UnusedResources] Remove unused resources & add fake references to prevent Linter warnings
Whiteboard: [leave-open]
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.