1413085 - Make PREF_Get{CString,Int,Bool}Pref() more uniform

Status: RESOLVED FIXED

(Core :: Preferences: Backend, enhancement)

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Bug 1413085 - Make PREF_Get{CString,Int,Bool}Pref() more uniform. .

This patch does the following.

- Reduces nesting and simplifies control flow by handling failure cases
  earlier, and makes all three functions have identical structure.

- Avoids unnecessary temporary local variables, including |rv|.

- Removes low-value comments, including some misleading ones (e.g. "This
  function will perform conversion...").

- PREF_GetCStringValue() previously did not check HasDefaultValue(), unlike the
  other two. It now does.

- PREF_GetCStringValue() now calls SetIsVoid(true) at the start, so that
  aValueOut will be Voided on all failure paths.

Review commit: https://reviewboard.mozilla.org/r/194802/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194802/

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Bug 1413085 - Inline and remove pref_SetPref(). .

It has a single call site.

Review commit: https://reviewboard.mozilla.org/r/194804/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194804/

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8923677 [details]
Bug 1413085 - Make PREF_Get{CString,Int,Bool}Pref() more uniform. .

https://reviewboard.mozilla.org/r/194802/#review199898

Should GetIntPref and GetBoolPref reset aValueOut on error?

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8923680 [details]
Bug 1413085 - Inline and remove pref_SetPref(). .

https://reviewboard.mozilla.org/r/194804/#review199900

::: modules/libpref/Preferences.cpp:4104
(Diff revision 1)
> +    rv = SetPrefValue(prefName, defaultValue.get_PrefValue(), DEFAULT_VALUE);
> +    if (NS_FAILED(rv)) {

you could do `if (NS_FAILED(SetPrefValue(...))` and not have rv at all. If you still prefer to have rv = ... ; if (NS_FAILED(rv)), at least move the variable declaration here.

Comment 5

[Nicholas Nethercote [inactive]]

> Should GetIntPref and GetBoolPref reset aValueOut on error?

They don't currently, so I'll leave them unchanged. Assuming anything about outparams on failure is a bad idea.
Some GetStringPref() do that, unfortunately, and the Voiding is required. I will probably add some MOZ_MUST_USE and [must_use] annotations to fix up these uses.

Comment 6

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/870f7ca1b57a0b68f123eca351bea20d2ae1922e
Bug 1413085 - Make PREF_Get{CString,Int,Bool}Pref() more uniform. r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa42202670b85ee2ef0a66f3948b2a4357d68c0a
Bug 1413085 - Inline and remove pref_SetPref(). r=glandium.

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/870f7ca1b57a
https://hg.mozilla.org/mozilla-central/rev/fa42202670b8