Closed
Bug 1384835
Opened 7 years ago
Closed 7 years ago
Remove nsAdopting[C]String methods from libpref
Categories
(Core :: Preferences: Backend, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(3 files)
49.72 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
51.23 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
73.55 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
We want to remove nsAdopting[C]String (bug 1384834). The main use of these classes is in the Preferences::Get*String() functions.
Assignee | ||
Comment 1•7 years ago
|
||
This is basically a cosmetic change; references are the normal way to do string outparams.
Attachment #8890706 -
Flags: review?(nfroyd)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
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)
Assignee | ||
Comment 3•7 years ago
|
||
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)
Updated•7 years ago
|
Attachment #8890706 -
Flags: review?(nfroyd) → review+
Comment 4•7 years ago
|
||
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 5•7 years ago
|
||
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+
Assignee | ||
Comment 6•7 years ago
|
||
> > + 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.
Assignee | ||
Comment 7•7 years ago
|
||
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.
Assignee | ||
Comment 8•7 years ago
|
||
> 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.
Comment 9•7 years ago
|
||
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)
Comment 10•7 years ago
|
||
Ok, also seen this on Android debug and OSX debug.
Comment hidden (obsolete) |
Assignee | ||
Comment 12•7 years ago
|
||
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.
Comment 13•7 years ago
|
||
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
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c23ec9e1395c https://hg.mozilla.org/mozilla-central/rev/f2356ef5b902 https://hg.mozilla.org/mozilla-central/rev/2b347fb55a99 https://hg.mozilla.org/mozilla-central/rev/06d31a972d44
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•