sync/Utils.java claims `StringBuffer should be used instead`

RESOLVED FIXED in Firefox 27

Status

()

Firefox for Android
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ckitching, Assigned: mcomella)

Tracking

Trunk
Firefox 27
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Method byte2hex of Utils.java in mobile/android/base/sync has a comment reminding the programmer that StringBuffer should be used instead of String concatenation in this method, but concatenation is wastefully still being used (And what you really want here is a StringBuilder anyway, because it`s only being accessed from one thread)

Let´s fix that.
(Reporter)

Updated

5 years ago
Assignee: nobody → ckitching
(Reporter)

Comment 1

5 years ago
Created attachment 768655 [details] [diff] [review]
byte2hex now uses StringBuilder, as requested by the comment.

A patch to do as described.
Attachment #768655 - Flags: review?(margaret.leibovic)
(Reporter)

Comment 2

5 years ago
Created attachment 768659 [details] [diff] [review]
byte2hex now uses StringBuilder, as requested by the comment, V2 (Added initial capacity).

Missed the initial capacity... fixed.
Attachment #768655 - Attachment is obsolete: true
Attachment #768655 - Flags: review?(margaret.leibovic)
Attachment #768659 - Flags: review?(margaret.leibovic)

Comment 3

5 years ago
Comment on attachment 768659 [details] [diff] [review]
byte2hex now uses StringBuilder, as requested by the comment, V2 (Added initial capacity).

This is also rnewman's domain.

You've made your way into sync code! :)
Attachment #768659 - Flags: review?(margaret.leibovic) → review?(rnewman)
Comment on attachment 768659 [details] [diff] [review]
byte2hex now uses StringBuilder, as requested by the comment, V2 (Added initial capacity).

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

Don't forget to clean up the patch headers; 'hg qfold' concatenates your temporary commit messages into the destination patch.

In order to land this, you first need to take it upstream:

https://github.com/mozilla-services/android-sync/blob/develop/README.rst

This is a good set of skills to acquire. If you can test that change (there are plenty of tests in the repo that exercise it), open a pull req and we'll get it merged in both places.

If you're disinclined to do so (I know margaret is a harsh taskmistress), let me know and I'll take care of the landing.

Please don't set checkin-needed without addressing the style nit and upstreaming first.

::: mobile/android/base/sync/Utils.java
@@ +82,5 @@
>    /**
>     * Helper to convert a byte array to a hex-encoded string
>     */
>    public static String byte2hex(byte[] b) {
> +    StringBuilder hs = new StringBuilder(b.length*2);

Spaces around arithmetic operators.
Attachment #768659 - Flags: review?(rnewman) → review+
Status: NEW → ASSIGNED
I implemented the test ckitching was missing:

https://github.com/mozilla-services/android-sync/pull/362
Flags: needinfo?(rnewman)
r+ via github.
Assignee: chriskitching → michael.l.comella
Flags: needinfo?(rnewman)
Created attachment 814673 [details] [diff] [review]
01c: m-c Patch

r+ via github.
Attachment #768659 - Attachment is obsolete: true
Attachment #814673 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e72312830067
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
You need to log in before you can comment on or make changes to this bug.