error-prone: make inner classes static when possible

RESOLVED FIXED in Firefox 58

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: andrew, Assigned: andrew)

Tracking

Trunk
Firefox 58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

2 years ago
This avoids an implicit this field and can help GC.  Found via error-prone.
(Assignee)

Updated

2 years ago
Attachment #8887713 - Flags: review?(s.kaspari)
Attachment #8887713 - Flags: review?(nalexander)
Assignee: nobody → andrew
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!
Attachment #8887713 - Flags: review?(nalexander) → review+
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
Attachment #8887713 - Flags: review?(s.kaspari) → review+
(Assignee)

Comment 4

2 years ago
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.
(Assignee)

Updated

2 years ago
Flags: needinfo?(s.kaspari)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(s.kaspari)
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. :)
(Assignee)

Updated

2 years ago
Flags: needinfo?(s.kaspari)
(Assignee)

Comment 6

2 years ago
Do you need anything more from me to move forward with this?
(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.
(Assignee)

Comment 8

2 years ago
This avoids an implicit this field and can help GC.  Found via error-prone.
(Assignee)

Updated

2 years ago
Flags: needinfo?(nalexander)
Attachment #8887713 - Attachment is obsolete: true
Flags: needinfo?(s.kaspari)
Thank you, Andrew! I added the "checkin-needed" keyword so that a code tree sheriff can land the patch.
Flags: needinfo?(nalexander)
Keywords: checkin-needed

Comment 10

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c264d9bf22a
Make inner classes static when possible. r=nalexander, r=sebastian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7c264d9bf22a
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
(Assignee)

Comment 13

a year ago
The former autoboxes to an Integer and uses the Integer object cache to avoid
allocations.
Attachment #8924221 - Flags: review?(s.kaspari)
Attachment #8924221 - Flags: review?(nalexander)
(Assignee)

Updated

a year ago
Attachment #8924221 - Flags: review?(s.kaspari)
Attachment #8924221 - Flags: review?(nalexander)
(Assignee)

Comment 14

a year ago
Sorry copy and paste error I intended to tag bug 1413620.
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`.
(Assignee)

Updated

a year ago
Attachment #8924221 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.