[findbugs] [MS] mutable arrays

RESOLVED FIXED in Firefox 53

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sebastian, Assigned: Shan11812)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 53
All
Android
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

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

2 years 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

2 years 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

2 years 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
(Reporter)

Comment 8

2 years 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

2 years 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+

Comment 12

2 years 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fc105f5be22c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years 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.