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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Comment 1

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

Attachment: MozReview Request: Bug 1255208 - Remove unused swipe refresh colors resources. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/39047/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39047/

Comment 2

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

Attachment: MozReview Request: Bug 1255208 - Remove miscellaneous unused resources. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/39049/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39049/

Comment 3

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

Attachment: MozReview Request: Bug 1255208 - Remove unused services resources. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/39051/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39051/

Comment 4

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

Attachment: MozReview Request: Bug 1255208 - Add suggested sites to UnusedResourceUtil. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/39053/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39053/

Comment 5

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

Attachment: MozReview Request: Bug 1255208 - Add preferences_privacy_clear_tablet to UnusedResourcesUtil. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/39055/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39055/

Comment 6

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

Attachment: MozReview Request: Bug 1255208 - Remove unused search strings. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/39057/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39057/

Comment 7

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

Attachment: MozReview Request: Bug 1255208 - Remove unused services strings. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/39059/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39059/

Comment 8

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

Attachment: MozReview Request: Bug 1255208 - Add crash reporter strings to UnusedResourcesUtil. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/39061/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39061/

Comment 9

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

Attachment: MozReview Request: Bug 1255208 - Add bookmarkdefaults to UnusedResourcesUtil. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/39063/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39063/

Comment 10

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

Attachment: MozReview Request: Bug 1255208 - Remove unused strings. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/39065/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39065/

Comment 11

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

Attachment: MozReview Request: Bug 1255208 - Adding clarifying comments to removed string special case. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/39067/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39067/

Comment 12

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

Attachment: MozReview Request: Bug 1255208 - Force error for UnusedResources lint. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/39069/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39069/

Comment 13

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

Felt the need to return the favor for bug 1241810. ;)

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

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?

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8728710 [details]
MozReview Request: Bug 1255208 - Remove miscellaneous unused resources. r=sebastian

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

Nice!

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

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

https://reviewboard.mozilla.org/r/39051/#review35821

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

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?

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

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 20

[Sebastian Kaspari (:sebastian; :pocmo)]

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

https://reviewboard.mozilla.org/r/39059/#review35847

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8728716 [details]
MozReview Request: Bug 1255208 - Add crash reporter strings to UnusedResourcesUtil. r=sebastian

https://reviewboard.mozilla.org/r/39061/#review35849

Comment 22

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8728717 [details]
MozReview Request: Bug 1255208 - Add bookmarkdefaults to UnusedResourcesUtil. r=sebastian

https://reviewboard.mozilla.org/r/39063/#review35851

Comment 23

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8728718 [details]
MozReview Request: Bug 1255208 - Remove unused strings. r=sebastian

https://reviewboard.mozilla.org/r/39065/#review35855

Comment 24

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

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/

Comment 26

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

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`).

Comment 27

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

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

Comment 28

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

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.

Comment 29

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

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

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eb8fb90d1582
https://hg.mozilla.org/mozilla-central/rev/81be7df2c496
https://hg.mozilla.org/mozilla-central/rev/78565e0dc28e
https://hg.mozilla.org/mozilla-central/rev/ac97f8eb56ba
https://hg.mozilla.org/mozilla-central/rev/c908af35a2d5
https://hg.mozilla.org/mozilla-central/rev/994643184618
https://hg.mozilla.org/mozilla-central/rev/9613174a01bc
https://hg.mozilla.org/mozilla-central/rev/52dc663f2819
https://hg.mozilla.org/mozilla-central/rev/ed1ddedeac75
https://hg.mozilla.org/mozilla-central/rev/0995bb2323b5
https://hg.mozilla.org/mozilla-central/rev/ffd6647fb50f
https://hg.mozilla.org/mozilla-central/rev/1f0fdd00dba1