1382049 - error-prone: make inner classes static when possible

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Andrew Gaul]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170718164211

Comment 1

[Andrew Gaul]

Attachment: Make inner classes static when possible

This avoids an implicit this field and can help GC.  Found via error-prone.

Comment 2

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8887713 [details] [diff] [review]
Make inner classes static when possible

Review of attachment 8887713 [details] [diff] [review]:
-----------------------------------------------------------------

In general I'm fine with this, but will defer to Sebastian.  Thanks for pushing on this, Andrew!

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8887713 [details] [diff] [review]
Make inner classes static when possible

Review of attachment 8887713 [details] [diff] [review]:
-----------------------------------------------------------------

Making inner classes static is great to avoid accidental references. Does error-prone output a reasoning for making some of those classes final?

Do you have try[1] access to test your patch?


[1] https://wiki.mozilla.org/ReleaseEngineering/TryServer

Comment 4

[Andrew Gaul]

I added final when adding static as a personal preference.  error-prone does not do this kind of visibility analysis.

I do not have try access as this is my first patch.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

I pushed the patch to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=08b8b6c92f1320c1a3c78bfbcb82b5bd358d9326

If it passes all tests then we can land the patch. :)

Comment 6

[Andrew Gaul]

Do you need anything more from me to move forward with this?

Comment 7

[Nick Alexander :nalexander [he/him]]

(In reply to Andrew Gaul from comment #6)
> Do you need anything more from me to move forward with this?

Try looks green, so we should be good.  I think this just fell off the radar.  Would you mind rebasing and pushing a new patch?  I'll set checkin-needed.

Thanks, Andrew!  Sorry for the long delay.

Comment 8

[Andrew Gaul]

Attachment: Make inner classes static when possible

This avoids an implicit this field and can help GC.  Found via error-prone.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Thank you, Andrew! I added the "checkin-needed" keyword so that a code tree sheriff can land the patch.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c264d9bf22a
Make inner classes static when possible. r=nalexander, r=sebastian

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7c264d9bf22a

Comment 12

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/7c264d9bf22a

Comment 13

[Andrew Gaul]

Attachment: Prefer Integer.parseInt over new Integer

The former autoboxes to an Integer and uses the Integer object cache to avoid
allocations.

Comment 14

[Andrew Gaul]

Sorry copy and paste error I intended to tag bug 1413620.

Comment 15

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8924221 [details] [diff] [review]
Prefer Integer.parseInt over new Integer

Review of attachment 8924221 [details] [diff] [review]:
-----------------------------------------------------------------

r- just to revisit.  Is the implicit Integer created equivalent to Integer.valueOf.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalURLMetadata.java
@@ +81,5 @@
>                  Iterator<String> keys = icons.keys();
>  
>                  ArrayList<Integer> sizes = new ArrayList<Integer>(icons.length());
>                  while (keys.hasNext()) {
> +                    sizes.add(Integer.parseInt(keys.next()));

https://stackoverflow.com/a/31115685 suggests that `Integer.valueOf()` is better yet, since we need an `Integer`.