[lint: UnusedResources #2] Let's save us some APK size

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

(Blocks 1 bug)

unspecified
Firefox 48
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(12 attachments)

58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
No description provided.
Felt the need to return the favor for bug 1241810. ;)
Assignee: nobody → michael.l.comella
Summary: [lint: UnusedResources] Let's save us some APK size → [lint: UnusedResources #2] Let's save us some APK size
Comment on attachment 8728709 [details]
MozReview Request: Bug 1255208 - Remove unused swipe refresh colors resources. r=sebastian

https://reviewboard.mozilla.org/r/39047/#review35817

::: mobile/android/base/resources/layout/home_suggestion_prompt.xml:47
(Diff revision 1)
> -                  android:textColor="@color/swipe_refresh_white"
> +                  android:textColor="#fff"

I always wonder what's better in such a case:
* #fff
* or: @android:color/white

There seem to be no performance gain (and they would be negligible anyways) but do you think one of them is more readable?
Attachment #8728709 - Flags: review?(s.kaspari) → review+
Attachment #8728710 - Flags: review?(s.kaspari) → review+
Comment on attachment 8728710 [details]
MozReview Request: Bug 1255208 - Remove miscellaneous unused resources. r=sebastian

https://reviewboard.mozilla.org/r/39049/#review35819

Nice!
Comment on attachment 8728711 [details]
MozReview Request: Bug 1255208 - Remove unused services resources. r=sebastian

https://reviewboard.mozilla.org/r/39051/#review35821
Attachment #8728711 - Flags: review?(s.kaspari) → review+
Comment on attachment 8728712 [details]
MozReview Request: Bug 1255208 - Add suggested sites to UnusedResourceUtil. r=sebastian

https://reviewboard.mozilla.org/r/39053/#review35823

::: mobile/android/base/java/org/mozilla/gecko/util/UnusedResourcesUtil.java:54
(Diff revision 1)
> +            R.drawable.suggestedsites_restricted_fxsupport,

Seeing this I filed bug 1255349. We only use those for restricted profiles afaik and maybe we want to stop doing that.
Attachment #8728712 - Flags: review?(s.kaspari) → review+
Comment on attachment 8728713 [details]
MozReview Request: Bug 1255208 - Add preferences_privacy_clear_tablet to UnusedResourcesUtil. r=sebastian

https://reviewboard.mozilla.org/r/39055/#review35843

::: mobile/android/base/resources/values/xml.xml:7
(Diff revision 1)
> +    <!-- These items are v11+ resources but are referenced in code shipped with
> +         API 9 builds. Since v11+ resources don't ship on API 9 builds, in order
> +         for the resource ID to be found (and thus compilation to succeed), we
> +         provide dummy values below. -->
> +    <item type="xml" name="preferences_privacy_clear_tablet">@null</item>

Is this actually the case? Why didn't we see any issues?
Attachment #8728713 - Flags: review?(s.kaspari) → review+
Attachment #8728714 - Flags: review?(s.kaspari) → review+
Comment on attachment 8728714 [details]
MozReview Request: Bug 1255208 - Remove unused search strings. r=sebastian

https://reviewboard.mozilla.org/r/39057/#review35845

Red patches are good patches.
Comment on attachment 8728715 [details]
MozReview Request: Bug 1255208 - Remove unused services strings. r=sebastian

https://reviewboard.mozilla.org/r/39059/#review35847
Attachment #8728715 - Flags: review?(s.kaspari) → review+
Comment on attachment 8728716 [details]
MozReview Request: Bug 1255208 - Add crash reporter strings to UnusedResourcesUtil. r=sebastian

https://reviewboard.mozilla.org/r/39061/#review35849
Attachment #8728716 - Flags: review?(s.kaspari) → review+
Attachment #8728717 - Flags: review?(s.kaspari) → review+
Comment on attachment 8728717 [details]
MozReview Request: Bug 1255208 - Add bookmarkdefaults to UnusedResourcesUtil. r=sebastian

https://reviewboard.mozilla.org/r/39063/#review35851
Comment on attachment 8728718 [details]
MozReview Request: Bug 1255208 - Remove unused strings. r=sebastian

https://reviewboard.mozilla.org/r/39065/#review35855
Attachment #8728718 - Flags: review?(s.kaspari) → review+
Comment on attachment 8728719 [details]
MozReview Request: Bug 1255208 - Adding clarifying comments to removed string special case. r=sebastian

https://reviewboard.mozilla.org/r/39067/#review35857

::: mobile/android/base/locales/en-US/android_strings.dtd:341
(Diff revision 1)
> +<!-- When this item is removed, also remove datareporting_notification_action_long:
> +     it is unused in strings.xml. -->

Isn't this something you should always check when removing a string that contains other strings (e.g. check all sub strings, not only the one in the comment)? Such comments easily get outdated (or misleading) without anyone noticing.
Attachment #8728719 - Flags: review?(s.kaspari) → review+
Comment on attachment 8728720 [details]
MozReview Request: Bug 1255208 - Force error for UnusedResources lint. r=sebastian

https://reviewboard.mozilla.org/r/39069/#review35861

Yeah. This lint error will be very valuable \o/
Attachment #8728720 - Flags: review?(s.kaspari) → review+
https://reviewboard.mozilla.org/r/39047/#review35817

> I always wonder what's better in such a case:
> * #fff
> * or: @android:color/white
> 
> There seem to be no performance gain (and they would be negligible anyways) but do you think one of them is more readable?

Good call. I like `@android:color/white` because it shows a clearer intent (whereas with `#fff` could have meant `#eee`).
https://reviewboard.mozilla.org/r/39055/#review35843

> Is this actually the case? Why didn't we see any issues?

Since we reference preferences_privacy_clear_tablet in UnusedResourcesUtil (which presumably ships on API 9 – it retrospect, it shouldn't have but too late), it'll be a compile-time error on API 9 builds when associated xml resource (which is in v11) can't be found. We fixed this by maxing a lot of similar files, like https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/values/layout.xml
https://reviewboard.mozilla.org/r/39067/#review35857

> Isn't this something you should always check when removing a string that contains other strings (e.g. check all sub strings, not only the one in the comment)? Such comments easily get outdated (or misleading) without anyone noticing.

You're right – this should be the case – but this is the first time I encountered such a combination and wouldn't have done the lookup if I was removing the Strings, hence I added a comment.

fwiw, I think String combinations could potentially break l10n so I'd prefer to see somewhat redundant strings anyway, which would solve this problem.
https://hg.mozilla.org/integration/fx-team/rev/eb8fb90d15822beb39e8cbaaac6c0b31e85fd67c
Bug 1255208 - Remove unused swipe refresh colors resources. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/81be7df2c496e9081b13956977c6809982c27df4
Bug 1255208 - Remove miscellaneous unused resources. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/78565e0dc28ed5b659ea56cb2dd6a1e392176134
Bug 1255208 - Remove unused services resources. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/ac97f8eb56ba40d1f506a1561ee5edac6c4ab995
Bug 1255208 - Add suggested sites to UnusedResourceUtil. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/c908af35a2d561dee6ead186051f838c64682a3a
Bug 1255208 - Add preferences_privacy_clear_tablet to UnusedResourcesUtil. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/994643184618b460d446b5c977aa02da936b682f
Bug 1255208 - Remove unused search strings. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/9613174a01bc52d7e3b90dcf8192522491868eb4
Bug 1255208 - Remove unused services strings. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/52dc663f2819e9f8da422ac37dd813a84feea957
Bug 1255208 - Add crash reporter strings to UnusedResourcesUtil. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/ed1ddedeac75299f7f47e32959fa1183c2b0abcb
Bug 1255208 - Add bookmarkdefaults to UnusedResourcesUtil. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/0995bb2323b5e0bfe1c193cd041a414b0546da21
Bug 1255208 - Remove unused strings. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/ffd6647fb50f4c62e8d1b71cc2cb96b91d5ac7cd
Bug 1255208 - Adding clarifying comments to removed string special case. r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/1f0fdd00dba1edda8774055aec3f51d84c7913a3
Bug 1255208 - Force error for UnusedResources lint. r=sebastian
You need to log in before you can comment on or make changes to this bug.