1407112 - Reduce modules/libpref/ to one .h file and one .cpp file

Status: RESOLVED FIXED

(Core :: Preferences: Backend, enhancement)

Description

[Nicholas Nethercote [inactive]]

modules/libpref/ currently has five .h files (only one of which, Preferences.h, is exported) and five .cpp files.

The aim of this bug is to combine the four non-exported .h files and the five .cpp files into a single .cpp file. This will allow many follow-up clean-ups and simplifications, especially to break down the no-longer-needed distinction between the low-level API and the high-level API.

But I will do those follow-ups in follow-up bugs; this bug is just about the large-scale code movement.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Merge nsPrefsFactory.cpp into Preferences.cpp

MozReview-Commit-ID: V1tONOw0wT

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Merge nsPrefBranch.{cpp,h} into Preferences.cpp

MozReview-Commit-ID: 3ugPL8ba6te

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 8916863 [details] [diff] [review]
(part 1) - Merge nsPrefsFactory.cpp into Preferences.cpp

Wait, I'll do these patches with MozReview instead.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Bug 1407112 (part 1) - Merge nsPrefsFactory.cpp into Preferences.cpp. .

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

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Bug 1407112 (part 2) - Merge nsPrefBranch.{cpp,h} into Preferences.cpp. .

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

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: Bug 1407112 (part 3) - Merge prefread.{cpp,h} into Preferences.cpp. .

Note: the #include of prefread.h from prefapi.cpp was unnecessary.

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

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: Bug 1407112 (part 4) - Merge prefapi.{cpp,h} and prefapi_private_data.h into Preferences.cpp. .

This required moving WatchinPrefRAII and WATCHING_PREF_RAII

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

Comment 8

[Code Review Bot [:reviewbot]]

Comment on attachment 8916877 [details]
Bug 1407112 (part 4) - Merge prefapi.{cpp,h} and prefapi_private_data.h into Preferences.cpp. .

https://reviewboard.mozilla.org/r/187932/#review192980


C/C++ static analysis found 5 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: modules/libpref/Preferences.cpp:598
(Diff revision 1)
> +
> +  // Paranoid worst case all slashes will free quickly.
> +  for (p = aOriginal; *p; ++p) {
> +    switch (*p) {
> +      case '\n':
> +        aResult.AppendLiteral("\\n");

Warning: Escaped string literal can be written as a raw string literal [clang-tidy: modernize-raw-string-literal]

        aResult.AppendLiteral("\\n");
                              ^
                              R"(\n)"

::: modules/libpref/Preferences.cpp:602
(Diff revision 1)
> +      case '\n':
> +        aResult.AppendLiteral("\\n");
> +        break;
> +
> +      case '\r':
> +        aResult.AppendLiteral("\\r");

Warning: Escaped string literal can be written as a raw string literal [clang-tidy: modernize-raw-string-literal]

        aResult.AppendLiteral("\\r");
                              ^
                              R"(\r)"

::: modules/libpref/Preferences.cpp:606
(Diff revision 1)
> +      case '\r':
> +        aResult.AppendLiteral("\\r");
> +        break;
> +
> +      case '\\':
> +        aResult.AppendLiteral("\\\\");

Warning: Escaped string literal can be written as a raw string literal [clang-tidy: modernize-raw-string-literal]

        aResult.AppendLiteral("\\\\");
                              ^
                              R"(\\)"

::: modules/libpref/Preferences.cpp:610
(Diff revision 1)
> +      case '\\':
> +        aResult.AppendLiteral("\\\\");
> +        break;
> +
> +      case '\"':
> +        aResult.AppendLiteral("\\\"");

Warning: Escaped string literal can be written as a raw string literal [clang-tidy: modernize-raw-string-literal]

        aResult.AppendLiteral("\\\"");
                              ^
                              R"(\")"

::: modules/libpref/Preferences.cpp:724
(Diff revision 1)
> +  for (auto iter = aTable->Iter(); !iter.Done(); iter.Next()) {
> +    auto pref = static_cast<PrefHashEntry*>(iter.Get());
> +
> +    nsAutoCString prefValue;
> +    nsAutoCString prefPrefix;
> +    prefPrefix.AssignLiteral("user_pref(\"");

Warning: Escaped string literal can be written as a raw string literal [clang-tidy: modernize-raw-string-literal]

    prefPrefix.AssignLiteral("user_pref(\"");
                             ^
                             R"(user_pref(")"

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8916874 [details]
Bug 1407112 (part 1) - Merge nsPrefsFactory.cpp into Preferences.cpp. .

https://reviewboard.mozilla.org/r/187926/#review193392

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8916875 [details]
Bug 1407112 (part 2) - Merge nsPrefBranch.{cpp,h} into Preferences.cpp. .

https://reviewboard.mozilla.org/r/187928/#review193396

::: modules/libpref/Preferences.cpp:84
(Diff revision 1)
>  
>  #define ENSURE_MAIN_PROCESS(message, pref)                                     \
>    do {                                                                         \
>      if (MOZ_UNLIKELY(!XRE_IsParentProcess())) {                                \
>        nsPrintfCString msg("ENSURE_MAIN_PROCESS failed. %s %s", message, pref); \
>        NS_WARNING(msg.get());                                                   \

nsPrefBranch uses NS_ERROR here.

::: modules/libpref/Preferences.cpp:395
(Diff revision 1)
> +  nsCOMPtr<nsIFile> mFile;
> +  nsCString mRelativeToKey;
> +};
> +
> +static ContentChild*
> +GetContentChild()

The result of this function is never actually used other than as a bool, and it doesn't seem relevant to its callers that the content child singleton is non-null. Replace with XRE_IsContentProcess in the callers? (Or maybe !XRE_IsParentProcess, whichever is more relevant, which I'm not 100% sure ; ENSURE_MAIN_PROCESS uses the latter in this file, but used GetContentChild in PrefBranch)

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8916876 [details]
Bug 1407112 (part 3) - Merge prefread.{cpp,h} into Preferences.cpp. .

https://reviewboard.mozilla.org/r/187930/#review193402

::: modules/libpref/Preferences.cpp:117
(Diff revision 1)
>  
>  //===========================================================================
> +// Prefs parsing
> +//===========================================================================
> +
> +#ifdef __cplusplus

Obviously, this file is C++, you don't need the #ifdef. You probably also don't need to keep the functions as extern "C".

::: modules/libpref/Preferences.cpp:197
(Diff revision 1)
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#ifdef TEST_PREFREAD

Nothing defines this manually and this "unlocks" a main() in this file, do you want to keep it?

::: modules/libpref/Preferences.cpp:202
(Diff revision 1)
> +#ifdef TEST_PREFREAD
> +#include <stdio.h>
> +#define NS_WARNING(_s) printf(">>> " _s "!\n")
> +#define NS_NOTREACHED(_s) NS_WARNING(_s)
> +#else
> +#include "nsDebug.h" // for NS_WARNING

other things are using NS_WARNING, so the include is probably not necessary.

Comment 12

[Nicholas Nethercote [inactive]]

> >  #define ENSURE_MAIN_PROCESS(message, pref)                                     \
> >    do {                                                                         \
> >      if (MOZ_UNLIKELY(!XRE_IsParentProcess())) {                                \
> >        nsPrintfCString msg("ENSURE_MAIN_PROCESS failed. %s %s", message, pref); \
> >        NS_WARNING(msg.get());                                                   \
> 
> nsPrefBranch uses NS_ERROR here.

I will switch to NS_ERROR.

> The result of this function is never actually used other than as a bool

True. I'll fix it in a follow-up.

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8916877 [details]
Bug 1407112 (part 4) - Merge prefapi.{cpp,h} and prefapi_private_data.h into Preferences.cpp. .

https://reviewboard.mozilla.org/r/187932/#review193406

::: modules/libpref/Preferences.cpp:89
(Diff revision 1)
> +
> +#ifdef MOZ_CRASHREPORTER
> +#include "nsICrashReporter.h"
> +#endif
> +
> +#ifdef _WIN32

Seems like this should be XP_WIN.

::: modules/libpref/Preferences.cpp:156
(Diff revision 1)
> +pref_GetPrefFromEntry(PrefHashEntry* aHashEntry,
> +                      mozilla::dom::PrefSetting* aPref);
> +
> +size_t
> +pref_SizeOfPrivateData(mozilla::MallocSizeOf aMallocSizeOf);
> +#ifdef __cplusplus

#ifdef unnecessary ; extern "C" probably unnecessary too.

Comment 14

[Nicholas Nethercote [inactive]]

> Obviously, this file is C++, you don't need the #ifdef. You probably also
> don't need to keep the functions as extern "C".

I will fix this and some other C-isms in a follow-up.

> > +#ifdef TEST_PREFREAD
> 
> Nothing defines this manually and this "unlocks" a main() in this file, do
> you want to keep it?

Another follow-up! :)

Comment 15

[Nicholas Nethercote [inactive]]

> Seems like this should be XP_WIN.

Ok.

> #ifdef unnecessary ; extern "C" probably unnecessary too.

Another follow-up :)

Comment 16

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #12)
> > >  #define ENSURE_MAIN_PROCESS(message, pref)                                     \
> > >    do {                                                                         \
> > >      if (MOZ_UNLIKELY(!XRE_IsParentProcess())) {                                \
> > >        nsPrintfCString msg("ENSURE_MAIN_PROCESS failed. %s %s", message, pref); \
> > >        NS_WARNING(msg.get());                                                   \
> > 
> > nsPrefBranch uses NS_ERROR here.
> 
> I will switch to NS_ERROR.

Oh dear. This caused some mochitest failures because we actually have code that fails this check, and NS_ERROR causes mochitest assertions where NS_WARNING did not. I will split the macro in two (giving it two different names) and file a follow-up bug about the bad code.

Comment 17

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/16941fa18f1c25cfde54978cefe95a52ed1ba8c4
Bug 1407112 (part 1) - Merge nsPrefsFactory.cpp into Preferences.cpp. r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/af42b728e0edce3b43e2f96ef68ba3ea05ececa1
Bug 1407112 (part 2) - Merge nsPrefBranch.{cpp,h} into Preferences.cpp. r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b09c968f6cb99fcd80f6e9a7265b4afc783ac01c
Bug 1407112 (part 3) - Merge prefread.{cpp,h} into Preferences.cpp. r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebd299dbebdae35905c4d4fd05d49fc1eb21a399
Bug 1407112 (part 4) - Merge prefapi.{cpp,h} and prefapi_private_data.h into Preferences.cpp. r=glandium.

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/16941fa18f1c
https://hg.mozilla.org/mozilla-central/rev/af42b728e0ed
https://hg.mozilla.org/mozilla-central/rev/b09c968f6cb9
https://hg.mozilla.org/mozilla-central/rev/ebd299dbebda