If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Fix nsVolumeService to use strings better

RESOLVED FIXED

Status

Firefox OS
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dhylands, Assigned: baku)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

2.68 KB, patch
Justin Lebar (not reading bugmail)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
+++ This bug was initially created as a clone of Bug #835408 +++

In particular bug 835408 comment 7:

>   nsString stateStr(NS_ConvertUTF8toUTF16(vol->StateStr()));
>   obs->NotifyObservers(vol, NS_VOLUME_STATE_CHANGED, stateStr.get());
On an unrelated note, I believe NS_ConvertUTF8toUTF16 derives from nsAutoString, so if the conversion results in a short enough string it gets stored in the auto buffer and then promptly copied to stateStr. There are two nicer ways to write this:

NS_ConvertUTF8toUTF16 stateStr(vol->StateStr());
obs->NotifyObservers(vol, NS_VOLUME_STATE_CHANGED, stateStr.get());

or

obs->NotifyObservers(vol, NS_VOLUME_STATE_CHANGED,
                     NS_ConvertUTF8toUTF16(vol->StateStr()).get());
(Assignee)

Comment 1

5 years ago
Created attachment 707580 [details] [diff] [review]
patch
Attachment #707580 - Flags: review?(justin.lebar+bug)
(Assignee)

Updated

5 years ago
Assignee: nobody → amarchesini
Comment on attachment 707580 [details] [diff] [review]
patch

Sure.

You could even do

  NotifyObservers(..., NS_ConvertUTF8toUTF16(volume->StateStr()).get());

if you like.  Either way is fine with me.
Attachment #707580 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 3

5 years ago
Should this patch land to b2g18?
Keywords: checkin-needed
(Assignee)

Comment 4

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b03b419446f
(In reply to Andrea Marchesini (:baku) from comment #3)
> Should this patch land to b2g18?

Does this patch solve some immediate problem?
(Assignee)

Comment 6

5 years ago
Not really :) It's just a small optimization.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b03b419446f

Please post changeset links and remove checkin-needed when landing your patches on inbound so others don't waste their time attempting to do the same.
Keywords: checkin-needed
(Assignee)

Comment 8

5 years ago
I forgot to remove checkin-needed. Sorry for this.
https://hg.mozilla.org/mozilla-central/rev/7b03b419446f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.