1406280 - Restyle Preferences .h files

Status: RESOLVED FIXED

(Core :: Preferences: Backend, enhancement)

Description

[Nicholas Nethercote [inactive]]

Following on from bug 1406205...

Process:

- Patch 1: Run clang-format to fix whitespace.

- Patch 2: Do manual fix-ups -- braces, rename things, etc. -- and then finish with another clang-format.

- Squash the patches together before landing.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Bug 1406280 - Restyle modules/libpref/*.h (clang-format). .

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

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Bug 1406280 - Restyle modules/libpref/*.h (manual). .

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

Comment 3

[Nicholas Nethercote [inactive]]

erahm: I know you don't like MozReview, but for these sorts of patches it really does make reviewing easier, because it does a nice job of (a) identifying lines where only the indenting changed, and (b) identifying exactly which chars within a line changed.

Also, for the second patch, you might be wondering about my purging of the doc-style /** comments. I did it because they're only used rarely and inconsistently within libpref, we don't have any tooling that takes advantage of them, and often the information (esp. for @param entries) is trivial. So I opted for consistent use of // comments instead.

Comment 4

[Eric Rahm [:erahm]]

Comment on attachment 8915836 [details]
Bug 1406280 - Restyle modules/libpref/*.h (clang-format). .

https://reviewboard.mozilla.org/r/187066/#review192840

Not sure what your plans are, but following up to remove the "C" interface might make sense (or I guess we could fix things so we support a C interface again...)

::: modules/libpref/prefapi.h:24
(Diff revision 1)
>  // 1 MB should be enough for everyone.
>  static const uint32_t MAX_PREF_LENGTH = 1 * 1024 * 1024;
>  // Actually, 4kb should be enough for everyone.
>  static const uint32_t MAX_ADVISABLE_PREF_LENGTH = 4 * 1024;
>  
> -typedef union
> +typedef union {

Ugh are we still pretending to support c?

::: modules/libpref/prefapi.h:55
(Diff revision 1)
>  // Preference flags, including the native type of the preference. Changing any of these
>  // values will require modifying the code inside of PrefTypeFlags class.
>  // </font>
>  */
>  
> -enum class PrefType {
> +enum class PrefType

Hah, I guess not ;)

Comment 5

[Eric Rahm [:erahm]]

Comment on attachment 8915837 [details]
Bug 1406280 - Restyle modules/libpref/*.h (manual). .

https://reviewboard.mozilla.org/r/187068/#review192844

Looks good, a few minor things to fix.

::: modules/libpref/Preferences.h:140
(Diff revision 1)
>      return result;
>    }
>  
> -  /**
> -   * Gets int, float, or bool type pref value with raw return value of
> -   * nsIPrefBranch.
> +  // Gets int, float, or bool type pref value with raw return value of
> +  // nsIPrefBranch. |aResult| must not be nullptr; its contents are never
> +  // modified when these methods fail.

One thing I like about the doxygen style comments is having the params documented in a separate paragraph. Maybe move the last sentence down a line.

::: modules/libpref/prefapi.cpp:402
(Diff revision 1)
>  {
> -  if (aHashEntry->prefFlags.GetPrefType() != PrefType::String) {
> +  if (aHashEntry->mPrefFlags.GetPrefType() != PrefType::String) {
>      return true;
>    }
>  
> -  char* stringVal;
> +  char* mStringVal;

A `sed` slip up perhaps? Please revert this.

::: modules/libpref/prefapi.cpp:494
(Diff revision 1)
>    if (!gHashTable) {
>      return NS_ERROR_NOT_INITIALIZED;
>    }
>  
>    nsresult rv = NS_ERROR_UNEXPECTED;
> -  char* stringVal;
> +  char* mStringVal;

ditto

::: modules/libpref/prefread.h:18
(Diff revision 1)
>  extern "C" {
>  #endif
>  
> -/**
> - * Callback function used to notify consumer of preference name value pairs.
> - * The pref name and value must be copied by the implementor of the callback
> +// Callback function used to notify consumer of preference name value pairs.
> +// The pref name and value must be copied by the implementor of the callback
> +// if they are needed beyond the scope of the callback function.

Some form of the param docs should be kept. Particularly what `aIsDefault` means and `aIsStickyDefault`.

::: modules/libpref/prefread.h
(Diff revision 1)
> - * @param reporter
> - *        PrefParseErrorReporter callback function, which will be called if we
> - *        encounter any errors (stop) or warnings (continue) during parsing.
> - * @param closure
> - *        PrefReader closure.
> - */

Please keep the param docs, they're reasonably useful.

::: modules/libpref/prefread.h
(Diff revision 1)
> -                    void* closure);
> -
> -/**
> +                    void* aClosure);
> +
> +// Release any memory in use by the PrefParseState instance.
> - * PREF_FinalizeParseState
> - *
> - * Called to release any memory in use by the PrefParseState instance.

This comment is somewhat useful (indicating you'll leak memory if you don't finalize).

Comment 6

[Nicholas Nethercote [inactive]]

> Not sure what your plans are, but following up to remove the "C" interface
> might make sense (or I guess we could fix things so we support a C interface
> again...)

Yep, it's on my todo list.

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 8915836 [details]
Bug 1406280 - Restyle modules/libpref/*.h (clang-format). .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187066/diff/1-2/

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 8915837 [details]
Bug 1406280 - Restyle modules/libpref/*.h (manual). .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187068/diff/1-2/

Comment 9

[Eric Rahm [:erahm]]

Comment on attachment 8915837 [details]
Bug 1406280 - Restyle modules/libpref/*.h (manual). .

https://reviewboard.mozilla.org/r/187068/#review192924

Comment 10

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/014f84dbd970d28beb9ee89e385a764b814916e4
Bug 1406280 - Restyle modules/libpref/*.h. r=erahm.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/014f84dbd970