956360 - Use Arrays.asList when possible

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

There are some places where we fill an ArrayList manually from an existing array; Arrays.asList can be used instead for improved readability/performance. I found the following cases,

* http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/sqlite/SQLiteBridge.java#203
* http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#707
* http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/browserid/verifier/BrowserIDRemoteVerifierClient.java#121
* http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/favicons/Favicons.java#377
* http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/health/BrowserHealthRecorder.java#896

* http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java#316
* http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/repositories/android/FormHistoryRepositorySession.java#351
* http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/setup/activities/SendTabData.java#53

Comment 1

[Marcos Cezar Mendes da Costa Junior]

Can i grab this bug? Assign to me ok!?

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Thanks Marcos! Let me know if you have any questions.

Comment 3

[Marcos Cezar Mendes da Costa Junior]

I want to know if we have more places like this in the code that this change could be apply. Do we have?

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Marcos Cezar Mendes da Costa Junior from comment #3)
> I want to know if we have more places like this in the code that this change
> could be apply. Do we have?

I'm not aware of other places. For now, it would be great to fix the places in comment 0.

Comment 5

[Marcos Cezar Mendes da Costa Junior]

Attachment: bug956360.diff

Comment 6

[:Margaret Leibovic]

Comment on attachment 8384609 [details] [diff] [review]
bug956360.diff

Thanks for the patch! I'm going to pass this review off to jchen, since he's more familiar with this bug and knows what needs to be done to fix it.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8384609 [details] [diff] [review]
bug956360.diff

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

::: mobile/android/base/GeckoApp.java
@@ +672,5 @@
>                  Intent intent = GeckoAppShell.getOpenURIIntent(sAppContext, message.optString("url"),
>                      message.optString("mime"), message.optString("action"), message.optString("title"));
>                  String[] handlers = GeckoAppShell.getHandlersForIntent(intent);
> +
> +                ArrayList<String> appList = new ArrayList<String>(Arrays.asList(handlers));

You should write this line as,

> List<String> appList = Arrays.asList(handlers);

This is more efficient because you don't have to call "new ArrayList". Can you make this change to the other places, too?

Just ask me again for review once you have a new patch. Let me know if you have questions! Thanks!

Comment 8

[Marcos Cezar Mendes da Costa Junior]

Attachment: bug956360-2.diff

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8385427 [details] [diff] [review]
bug956360-2.diff

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

Looks great, Marcos! Thanks! :) Here's a try run, https://tbpl.mozilla.org/?tree=Try&rev=2e0cdb48aedd

r+ from me if you fix the following nits:

::: mobile/android/base/browserid/verifier/BrowserIDRemoteVerifierClient.java
@@ +118,5 @@
>      BaseResource r = new BaseResource(verifierUri);
>  
>      r.delegate = new RemoteVerifierResourceDelegate(r, delegate);
>  
> +    List<NameValuePair> nvps = Arrays.asList(new NameValuePair[] { new BasicNameValuePair("audience", audience), 

Please remove the extra space at the end of this line.

@@ +119,5 @@
>  
>      r.delegate = new RemoteVerifierResourceDelegate(r, delegate);
>  
> +    List<NameValuePair> nvps = Arrays.asList(new NameValuePair[] { new BasicNameValuePair("audience", audience), 
> +                                                                   new BasicNameValuePair("assertion", assertion) });

We want to keep lines to less than 100 characters. You can write something like,

>    List<NameValuePair> nvps = Arrays.asList(new NameValuePair[] {
>        new BasicNameValuePair("audience", audience),
>        new BasicNameValuePair("assertion", assertion) });

::: mobile/android/base/favicons/Favicons.java
@@ +397,5 @@
>          }
>  
>          // Load and cache the built-in favicon in each of its sizes.
>          // TODO: don't open the zip twice!
> +        List<Bitmap> toInsert = Arrays.asList(loadBrandingBitmap(context, "favicon64.png"), 

Extra space

::: mobile/android/base/health/BrowserHealthRecorder.java
@@ +790,5 @@
>              new MeasurementFields() {
>                  @Override
>                  public Iterable<FieldSpec> getFields() {
> +                    List<FieldSpec> out = Arrays.asList(new FieldSpec("normal", Field.TYPE_JSON_DISCRETE),
> +                                                        new FieldSpec("abnormal", Field.TYPE_JSON_DISCRETE));

Line width

Comment 10

[Marcos Cezar Mendes da Costa Junior]

Attachment: bug956360-3.diff

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8385734 [details] [diff] [review]
bug956360-3.diff

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

Perfect! Thanks, Marcos!

Comment 12

[Marcos Cezar Mendes da Costa Junior]

When the patch will be integrated to the master branch?

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

Ah sorry, there should be a "checkin-needed" keyword to let someone know to check in the patch.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/82f3207d8070

Comment 15

[Nick Alexander :nalexander [he/him]]

Hey folks, this landed code into mobile/android/base/{browserid,sync}, which are managed upstream (in the android-sync github repo).  I'll port upstream.

Comment 16

[(inactive) Jim Chen [:jchen] [:darchons]]

Ah, thanks for catch it, Nick!

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/82f3207d8070

Comment 18

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #15)
> Hey folks, this landed code into mobile/android/base/{browserid,sync}, which
> are managed upstream (in the android-sync github repo).  I'll port upstream.

https://github.com/mozilla-services/android-sync/commit/406bb377d230f4409326ef3ba3b6b2113d7e14a9