Closed
Bug 1406280
Opened 7 years ago
Closed 7 years ago
Restyle Preferences .h files
Categories
(Core :: Preferences: Backend, enhancement)
Core
Preferences: Backend
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(2 files)
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
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•7 years ago
|
||
mozreview-review |
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 5•7 years ago
|
||
mozreview-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-
Assignee | ||
Comment 6•7 years ago
|
||
> 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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 9•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 10•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/014f84dbd970d28beb9ee89e385a764b814916e4 Bug 1406280 - Restyle modules/libpref/*.h. r=erahm.
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/014f84dbd970
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assignee | ||
Updated•7 years ago
|
Blocks: prefs-cleanup
Updated•6 years ago
|
Assignee: nobody → n.nethercote
You need to log in
before you can comment on or make changes to this bug.
Description
•