1415799 - Some libpref initialization tweaks

Status: RESOLVED FIXED

(Core :: Preferences: Backend, enhancement)

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Bug 1415799 - Inline and remove pref_SizeOfPrivateData().

It has a single call site.

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

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Bug 1415799 - Inline and remove PREF_Cleanup.

It has a single call site.

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

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Bug 1415799 - Inline and remove PREF_Init() and PREF_CleanupPrefs().

They both have two callsites, but in one of those they appear together, where
much of the code can be replaced with a ClearAndPrepareForLength() call.

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

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Bug 1415799 - Remove PREF_ClearUserPref() forward declaration.

It's not needed.

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

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Bug 1415799 - Inline and remove Preferences::Init().

It has a single call site.

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

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: Bug 1415799 - Rename pref_HashPref() as pref_SetPref(). .

Because it sets the pref.

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

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8926747 [details]
Bug 1415799 - Inline and remove pref_SizeOfPrivateData().

https://reviewboard.mozilla.org/r/197986/#review203188

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8926748 [details]
Bug 1415799 - Inline and remove PREF_Cleanup.

https://reviewboard.mozilla.org/r/197988/#review203190

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8926749 [details]
Bug 1415799 - Inline and remove PREF_Init() and PREF_CleanupPrefs().

https://reviewboard.mozilla.org/r/197990/#review203192

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8926750 [details]
Bug 1415799 - Remove PREF_ClearUserPref() forward declaration.

https://reviewboard.mozilla.org/r/197992/#review203194

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8926751 [details]
Bug 1415799 - Inline and remove Preferences::Init().

https://reviewboard.mozilla.org/r/197994/#review203196

::: modules/libpref/Preferences.cpp:3622
(Diff revision 1)
> +    if (NS_FAILED(rv)) {
> +      sPreferences = nullptr;
> +      gCacheDataDesc = "AddObserver(\"profile-before-change\") failed";
> +      return nullptr;
> +    }

it's preexisting, but it's weird that only one of those AddObserver is checked for.

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8926752 [details]
Bug 1415799 - Rename pref_HashPref() as pref_SetPref(). .

https://reviewboard.mozilla.org/r/197996/#review203198

Comment 13

[Nicholas Nethercote [inactive]]

> it's preexisting, but it's weird that only one of those AddObserver is
> checked for.

Yes. I figure it's supposed to indicate that the checked one is more important than the others?

Comment 14

[Mike Hommey [:glandium]]

The first one that was added without a check looks like it might have been an oversight.
https://hg.mozilla.org/mozilla-central/rev/a844e2a8c654#l1.21

The following ones look like they just have cargo culted from that one, especially after the profile-do-change one was removed.

Comment 15

[Code Review Bot [:reviewbot]]

Comment on attachment 8926751 [details]
Bug 1415799 - Inline and remove Preferences::Init().

https://reviewboard.mozilla.org/r/197994/#review203274


C/C++ static analysis found 1 defect in this patch.

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


::: modules/libpref/Preferences.cpp:3624
(Diff revision 1)
> +    observerService->AddObserver(
> +      sPreferences, "suspend_process_notification", true);
> +
> +    if (NS_FAILED(rv)) {
> +      sPreferences = nullptr;
> +      gCacheDataDesc = "AddObserver(\"profile-before-change\") failed";

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

      gCacheDataDesc = "AddObserver(\"profile-before-change\") failed";
                       ^
                       R"(AddObserver("profile-before-change") failed)"

Comment 16

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05a3c8d0c61a4a41bc735364082930d56b9eb18b
Bug 1415799 - Inline and remove pref_SizeOfPrivateData(). r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ecee4342b40b32f14a52e7396bc3aa121043d77
Bug 1415799 - Inline and remove PREF_Cleanup. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ac606e618639da441d54024dea61315d8f468b0
Bug 1415799 - Inline and remove PREF_Init() and PREF_CleanupPrefs(). r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/66c548fb700ce3d1b2085aa0fae19bead57744d8
Bug 1415799 - Remove PREF_ClearUserPref() forward declaration. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/14ad11ba54d7daea9fbe68fe7e1bf9e821d6989c
Bug 1415799 - Inline and remove Preferences::Init(). r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/72bc69654fcd69b48ab8de35e9a23b83157ce31b
Bug 1415799 - Rename pref_HashPref() as pref_SetPref(). r=glandium.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/05a3c8d0c61a
https://hg.mozilla.org/mozilla-central/rev/1ecee4342b40
https://hg.mozilla.org/mozilla-central/rev/1ac606e61863
https://hg.mozilla.org/mozilla-central/rev/66c548fb700c
https://hg.mozilla.org/mozilla-central/rev/14ad11ba54d7
https://hg.mozilla.org/mozilla-central/rev/72bc69654fcd