Closed Bug 1384835 Opened 7 years ago Closed 7 years ago

Remove nsAdopting[C]String methods from libpref

Categories

(Core :: Preferences: Backend, enhancement)

54 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

Attachments

(3 files)

We want to remove nsAdopting[C]String (bug 1384834). The main use of these classes is in the Preferences::Get*String() functions.
This is basically a cosmetic change; references are the normal way to do string
outparams.
Attachment #8890706 - Flags: review?(nfroyd)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Because we want to remove nsAdoptingString. We have other variants that don't
use nsAdoptingString, which can be used instead. There are three basic
patterns.

1. The easiest case is when we don't check for success.

> nsAdoptingString s = Preferences::GetString("foo");
> foo(s);

becomes:

> nsAutoString s;
> Preferences::GetString("foo", s);
> foo(s);

2. The next case is when we check if the result is empty.

> nsAdoptingString s = Preferences::GetString("foo");
> if (s.IsEmpty()) { ... }

becomes:

> nsAutoString s;
> Preferences::GetString("foo", s);
> if (s.IsEmpty()) { ... }

3. The final case is when we null check the result.

> nsAdoptingString s = Preferences::GetString("foo");
> if (s) { ... }

becomes:

> nsAutoString s;
> nsresult rv = Preferences::GetString("foo", s);
> if (NS_SUCCEEDED(rv)) { ... }

The patch also avoids some UTF8/UTF16 conversions in a few places.
Attachment #8890708 - Flags: review?(nfroyd)
This is similar like the previous patch, but for the 8-bit string variants.
Also, it changes assignment to Adopt() in GetCString() and GetDefaultCString()
to avoid an extra copy.
Attachment #8890709 - Flags: review?(nfroyd)
Attachment #8890706 - Flags: review?(nfroyd) → review+
Comment on attachment 8890708 [details] [diff] [review]
(part 2) - Remove the Preferences::Get*String() variants that return nsAdoptingString

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

::: dom/media/CubebUtils.cpp
@@ +140,5 @@
>      StaticMutexAutoLock lock(sMutex);
>      if (value.IsEmpty()) {
>        sVolumeScale = 1.0;
>      } else {
> +      sVolumeScale = std::max<double>(0, PR_strtod(value.get(), nullptr));

I wish we knew why this didn't use float preferences...
Attachment #8890708 - Flags: review?(nfroyd) → review+
Comment on attachment 8890709 [details] [diff] [review]
(part 3) - Remove the Preferences::Get*CString() variants that return nsAdoptingCString

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

::: modules/libpref/Preferences.cpp
@@ +1678,2 @@
>    if (NS_SUCCEEDED(rv)) {
> +    aResult.Adopt(result);

Can you file a followup to improve the situation here?  We're (probably) passing in an nsAutoCString to this function, only to allocate |result| and Adopt it into the nsAutoCString, thus completely negating the point of the Auto buffer.  We should be able to Assign the preference string directly into |aResult|, which would only require a string copy without the extra allocation.

I realize this is not exactly performance-critical code, but being smarter here would be nice...
Attachment #8890709 - Flags: review?(nfroyd) → review+
> > +      sVolumeScale = std::max<double>(0, PR_strtod(value.get(), nullptr));
> 
> I wish we knew why this didn't use float preferences...

The funny thing is that float prefs are actually stored as strings. But I don't want to futz with this, so I'll leave it alone.
https://hg.mozilla.org/integration/mozilla-inbound/rev/07b534f447404246cce2c784ef9f0136d467b7a6
Bug 1384835 (part 1) - Use nsA[C]String references instead of pointers for outparams of Get*String() pref functions. r=froydnj.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5df4d877860281f66dc14b512de963539dc443b0
Bug 1384835 (part 2) - Remove the Preferences::Get*String() variants that return nsAdoptingString. r=froydnj.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef5feef07bed07583c52e434dbc5e4b9a2545deb
Bug 1384835 (part 3) - Remove the Preferences::Get*CString() variants that return nsAdoptingCString. r=froydnj.
> Can you file a followup to improve the situation here?  We're (probably)
> passing in an nsAutoCString to this function, only to allocate |result| and
> Adopt it into the nsAutoCString, thus completely negating the point of the
> Auto buffer.  We should be able to Assign the preference string directly
> into |aResult|, which would only require a string copy without the extra
> allocation.

I filed bug 1385160.
Backed out for failing xpcshell's security/manager/ssl/tests/unit/test_cert_sha1.js checkCertErrorGenericAtTime on Linux x64 debug:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b57e8e2ae846d39319c45bd3446eb4d9e974ac5b
https://hg.mozilla.org/integration/mozilla-inbound/rev/04539e21f0d579a537e1a6abab75d9b88258d6cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/88e14ba4308e1ca878548a2b1616276c7b543c39

Push which ran failing test: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=dc186bad8bd537919a15722a1a79dd46660f95f1&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=118836240&repo=mozilla-inbound

[task 2017-07-28T06:51:17.402595Z] 06:51:17     INFO -  TEST-START | security/manager/ssl/tests/unit/test_cert_sha1.js
[task 2017-07-28T06:51:19.711417Z] 06:51:19  WARNING -  TEST-UNEXPECTED-FAIL | security/manager/ssl/tests/unit/test_cert_sha1.js | xpcshell return code: 0
[task 2017-07-28T06:51:19.765889Z] 06:51:19     INFO -  TEST-INFO took 2307ms
[task 2017-07-28T06:51:19.766246Z] 06:51:19     INFO -  >>>>>>>
[task 2017-07-28T06:51:19.770630Z] 06:51:19     INFO -  PID 11373 | [11373] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /home/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2866
[task 2017-07-28T06:51:19.770972Z] 06:51:19     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2017-07-28T06:51:19.772548Z] 06:51:19     INFO -  "cert cn=ee-pre"
[task 2017-07-28T06:51:19.774492Z] 06:51:19     INFO -  "cert issuer cn=int-pre"
[task 2017-07-28T06:51:19.779448Z] 06:51:19     INFO -  TEST-PASS | security/manager/ssl/tests/unit/test_cert_sha1.js | checkCertErrorGenericAtTime - [checkCertErrorGenericAtTime : 183] Actual and expected error should match - 0 == 0
[task 2017-07-28T06:51:19.781533Z] 06:51:19     INFO -  "cert cn=ee-post"
[task 2017-07-28T06:51:19.787035Z] 06:51:19     INFO -  "cert issuer cn=int-pre"
[task 2017-07-28T06:51:19.789425Z] 06:51:19     INFO -  TEST-PASS | security/manager/ssl/tests/unit/test_cert_sha1.js | checkCertErrorGenericAtTime - [checkCertErrorGenericAtTime : 183] Actual and expected error should match - 0 == 0
[task 2017-07-28T06:51:19.798372Z] 06:51:19     INFO -  "cert cn=ee-post"
[task 2017-07-28T06:51:19.800253Z] 06:51:19     INFO -  "cert issuer cn=int-post"
[task 2017-07-28T06:51:19.807001Z] 06:51:19     INFO -  TEST-PASS | security/manager/ssl/tests/unit/test_cert_sha1.js | checkCertErrorGenericAtTime - [checkCertErrorGenericAtTime : 183] Actual and expected error should match - 0 == 0
[task 2017-07-28T06:51:19.808855Z] 06:51:19     INFO -  "cert cn=ee-pre"
[task 2017-07-28T06:51:19.810777Z] 06:51:19     INFO -  "cert issuer cn=int-pre"
[task 2017-07-28T06:51:19.813679Z] 06:51:19     INFO -  TEST-PASS | security/manager/ssl/tests/unit/test_cert_sha1.js | checkCertErrorGenericAtTime - [checkCertErrorGenericAtTime : 183] Actual and expected error should match - -8016 == -8016
[task 2017-07-28T06:51:19.826092Z] 06:51:19     INFO -  "cert cn=ee-post"
[task 2017-07-28T06:51:19.828031Z] 06:51:19     INFO -  "cert issuer cn=int-pre"
[task 2017-07-28T06:51:19.830174Z] 06:51:19     INFO -  TEST-PASS | security/manager/ssl/tests/unit/test_cert_sha1.js | checkCertErrorGenericAtTime - [checkCertErrorGenericAtTime : 183] Actual and expected error should match - -8016 == -8016
[task 2017-07-28T06:51:19.832064Z] 06:51:19     INFO -  "cert cn=ee-post"
[task 2017-07-28T06:51:19.838814Z] 06:51:19     INFO -  "cert issuer cn=int-post"
[task 2017-07-28T06:51:19.840922Z] 06:51:19     INFO -  TEST-PASS | security/manager/ssl/tests/unit/test_cert_sha1.js | checkCertErrorGenericAtTime - [checkCertErrorGenericAtTime : 183] Actual and expected error should match - -8016 == -8016
[task 2017-07-28T06:51:19.842796Z] 06:51:19     INFO -  "cert cn=ee-pre"
[task 2017-07-28T06:51:19.846218Z] 06:51:19     INFO -  "cert issuer cn=int-pre"
[task 2017-07-28T06:51:19.851167Z] 06:51:19     INFO -  TEST-PASS | security/manager/ssl/tests/unit/test_cert_sha1.js | checkCertErrorGenericAtTime - [checkCertErrorGenericAtTime : 183] Actual and expected error should match - -8016 == -8016
[task 2017-07-28T06:51:19.853125Z] 06:51:19     INFO -  "cert cn=ee-post"
[task 2017-07-28T06:51:19.859494Z] 06:51:19     INFO -  "cert issuer cn=int-pre"
[task 2017-07-28T06:51:19.862692Z] 06:51:19     INFO -  TEST-PASS | security/manager/ssl/tests/unit/test_cert_sha1.js | checkCertErrorGenericAtTime - [checkCertErrorGenericAtTime : 183] Actual and expected error should match - -8016 == -8016
[task 2017-07-28T06:51:19.866756Z] 06:51:19     INFO -  "cert cn=ee-post"
[task 2017-07-28T06:51:19.868818Z] 06:51:19     INFO -  "cert issuer cn=int-post"
[task 2017-07-28T06:51:19.875306Z] 06:51:19     INFO -  TEST-PASS | security/manager/ssl/tests/unit/test_cert_sha1.js | checkCertErrorGenericAtTime - [checkCertErrorGenericAtTime : 183] Actual and expected error should match - -8016 == -8016
[task 2017-07-28T06:51:19.877498Z] 06:51:19     INFO -  "cert cn=ee-pre"
[task 2017-07-28T06:51:19.879732Z] 06:51:19     INFO -  "cert issuer cn=int-pre"
[task 2017-07-28T06:51:19.883979Z] 06:51:19     INFO -  TEST-PASS | security/manager/ssl/tests/unit/test_cert_sha1.js | checkCertErrorGenericAtTime - [checkCertErrorGenericAtTime : 183] Actual and expected error should match - -8016 == -8016
[task 2017-07-28T06:51:19.892953Z] 06:51:19     INFO -  "cert cn=ee-post"
[task 2017-07-28T06:51:19.894846Z] 06:51:19     INFO -  "cert issuer cn=int-pre"
[task 2017-07-28T06:51:19.897011Z] 06:51:19     INFO -  TEST-PASS | security/manager/ssl/tests/unit/test_cert_sha1.js | checkCertErrorGenericAtTime - [checkCertErrorGenericAtTime : 183] Actual and expected error should match - -8016 == -8016
[task 2017-07-28T06:51:19.903059Z] 06:51:19     INFO -  "cert cn=ee-post"
[task 2017-07-28T06:51:19.906295Z] 06:51:19     INFO -  "cert issuer cn=int-post"
[task 2017-07-28T06:51:19.908440Z] 06:51:19     INFO -  TEST-PASS | security/manager/ssl/tests/unit/test_cert_sha1.js | checkCertErrorGenericAtTime - [checkCertErrorGenericAtTime : 183] Actual and expected error should match - -8016 == -8016
[task 2017-07-28T06:51:19.914973Z] 06:51:19     INFO -  "cert cn=ee-pre"
[task 2017-07-28T06:51:19.917724Z] 06:51:19     INFO -  "cert issuer cn=int-pre"
[task 2017-07-28T06:51:19.920001Z] 06:51:19  WARNING -  TEST-UNEXPECTED-FAIL | security/manager/ssl/tests/unit/test_cert_sha1.js | checkCertErrorGenericAtTime - [checkCertErrorGenericAtTime : 183] Actual and expected error should match - -8016 == 0
[task 2017-07-28T06:51:19.923349Z] 06:51:19     INFO -  /home/worker/workspace/build/tests/xpcshell/tests/security/manager/ssl/tests/unit/head_psm.js:checkCertErrorGenericAtTime:183
[task 2017-07-28T06:51:19.934444Z] 06:51:19     INFO -  /home/worker/workspace/build/tests/xpcshell/tests/security/manager/ssl/tests/unit/test_cert_sha1.js:checkEndEntity:30
[task 2017-07-28T06:51:19.936528Z] 06:51:19     INFO -  /home/worker/workspace/build/tests/xpcshell/tests/security/manager/ssl/tests/unit/test_cert_sha1.js:run_test:121
[task 2017-07-28T06:51:19.942869Z] 06:51:19     INFO -  /home/worker/workspace/build/tests/xpcshell/head.js:_execute_test:538
[task 2017-07-28T06:51:19.946144Z] 06:51:19     INFO -  -e:null:1
[task 2017-07-28T06:51:19.948014Z] 06:51:19     INFO -  exiting test
Flags: needinfo?(n.nethercote)
Ok, also seen this on Android debug and OSX debug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c23ec9e1395c8c02b458d99349795031c680374a
Bug 1384835 (part 1, attempt 2) - Use nsA[C]String references instead of pointers for outparams of Get*String() pref functions. r=froydnj.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2356ef5b902b23d5e8c5ff6a53df46b3489c637
Bug 1384835 (part 2, attempt 2) - Remove the Preferences::Get*String() variants that return nsAdoptingString. r=froydnj.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b347fb55a9965acec727f6e40671ba859636603
Bug 1384835 (part 3, attempt 2) - Remove the Preferences::Get*CString() variants that return nsAdoptingCString. r=froydnj.
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06d31a972d44
Fix the build for a single remaining callsite that was landed on autoland. irc-r=marco
You need to log in before you can comment on or make changes to this bug.