[findbugs] [MS] mutable arrays

RESOLVED FIXED in Firefox 53

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sebastian, Assigned: Shan11812)

Tracking

(Blocks 1 bug)

unspecified
Firefox 53
All
Android
Points:
---
Dependency tree / graph

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
(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)
Changed AboutPages.DEFAULT_ICON_PAGES to list.
(Also changed GlobalConstants.DEFAULT_PROTOCOLS just in case you want to change it too?)
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-
Assignee: nobody → 11812r
Status: NEW → ASSIGNED
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 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+
Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fc105f5be22c
Change mutable array to unmutable list; r=sebastian
https://hg.mozilla.org/mozilla-central/rev/fc105f5be22c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.