[findbugs] [MS] mutable arrays

RESOLVED FIXED in Firefox 53

Status

()

Firefox for Android
General
P3
normal
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: sebastian, Assigned: Shan11812)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 53
All
Android
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

+++ This bug was initially created as a clone of Bug #1316009 +++

* org.mozilla.gecko.AboutPages.DEFAULT_ICON_PAGES is a mutable array

* org.mozilla.gecko.background.common.GlobalConstants.DEFAULT_CIPHER_SUITES is a mutable array

* org.mozilla.gecko.background.common.GlobalConstants.DEFAULT_PROTOCOLS is a mutable array
(Assignee)

Comment 1

7 months ago
(In reply to Sebastian Kaspari (:sebastian) from comment #0)
> +++ This bug was initially created as a clone of Bug #1316009 +++
> 
> * org.mozilla.gecko.AboutPages.DEFAULT_ICON_PAGES is a mutable array
> 
> * org.mozilla.gecko.background.common.GlobalConstants.DEFAULT_CIPHER_SUITES
> is a mutable array
> 
> * org.mozilla.gecko.background.common.GlobalConstants.DEFAULT_PROTOCOLS is a
> mutable array

As far as I know, we cant make an array immutable in java. I can only think of unmodifiableCollection (and may be enum?)
Flags: needinfo?(s.kaspari)
Yeah, see bug 1316009 comment 14 and the following ones. It's very likely that we are going to suppress the warning for GlobalConstants (because we will need an array anyways).

For AboutPages.DEFAULT_ICON_PAGES we might change the code. But I haven't looked at all the use cases yet.
Flags: needinfo?(s.kaspari)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

7 months ago
Changed AboutPages.DEFAULT_ICON_PAGES to list.
(Also changed GlobalConstants.DEFAULT_PROTOCOLS just in case you want to change it too?)
(Reporter)

Comment 5

7 months ago
mozreview-review
Comment on attachment 8815254 [details]
Bug 1320298 - Change mutable array to unmutable list;

https://reviewboard.mozilla.org/r/96268/#review96738

I don't think we should change the values in GlobalConstants to a List. All callers require an array so we would just convert between list and array representation all the time. Changing DEFAULT_ICON_PAGES on the other hand makes sense.

::: mobile/android/base/java/org/mozilla/gecko/AboutPages.java:102
(Diff revision 1)
> -        for (int i = 0; i < DEFAULT_ICON_PAGES.length; ++i) {
> -            if (DEFAULT_ICON_PAGES[i].equals(url)) {
> +        for (int i = 0; i < DEFAULT_ICON_PAGES.size(); ++i) {
> +            if (DEFAULT_ICON_PAGES.get(i).equals(url)) {

After turning this into a list we can use a nicer version of the loop:
> for (String page : DEFAULT_ICON_PAGES) {
> ..
Attachment #8815254 - Flags: review?(s.kaspari) → review-
Comment hidden (mozreview-request)
Assignee: nobody → 11812r
Status: NEW → ASSIGNED
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb3aa30b89f6
(Reporter)

Comment 8

6 months ago
mozreview-review
Comment on attachment 8815254 [details]
Bug 1320298 - Change mutable array to unmutable list;

https://reviewboard.mozilla.org/r/96268/#review98456

This failed to build in automation:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb3aa30b89f6&selectedJob=32362807

> /builds/slave/try-and-api-15-000000000000000/build/src/mobile/android/base/java/org/mozilla/gecko/icons/preparation/AboutPagesPreparer.java:26: error: method addAll in class Collections cannot be applied to given types;

Can you fix this and upload a new patch?
Attachment #8815254 - Flags: review?(s.kaspari) → review-
Comment hidden (mozreview-request)
(Reporter)

Comment 10

6 months ago
mozreview-review
Comment on attachment 8815254 [details]
Bug 1320298 - Change mutable array to unmutable list;

https://reviewboard.mozilla.org/r/96268/#review99396
Attachment #8815254 - Flags: review?(s.kaspari) → review+
I pushed the new patch to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b01c39c34df2

Comment 12

6 months ago
Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fc105f5be22c
Change mutable array to unmutable list; r=sebastian

Comment 13

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fc105f5be22c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.