Closed Bug 1406280 Opened 7 years ago Closed 7 years ago

Restyle Preferences .h files

Categories

(Core :: Preferences: Backend, enhancement)

Product:
Core
Component:
Preferences: Backend
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

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

References

Details

Attachments

(2 files)

Bug 1406280 - Restyle modules/libpref/*.h (clang-format). .
59 bytes, text/x-review-board-request
erahm
: review+
Details
Bug 1406280 - Restyle modules/libpref/*.h (manual). .
59 bytes, text/x-review-board-request
erahm
: review+
Details 
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.
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.
No longer depends on: 1406205
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 ;)
Attachment #8915836 - Flags: review?(erahm) → review+
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).
Attachment #8915837 - Flags: review?(erahm) → review-
> 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 on attachment 8915837 [details]
Bug 1406280 - Restyle modules/libpref/*.h (manual). .

https://reviewboard.mozilla.org/r/187068/#review192924
Attachment #8915837 - Flags: review?(erahm) → review+
https://hg.mozilla.org/mozilla-central/rev/014f84dbd970
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Blocks: prefs-cleanup
Assignee: nobody → n.nethercote
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: