Closed Bug 1415799 Opened 2 years ago Closed 2 years ago

Some libpref initialization tweaks

Categories

(Core :: Preferences: Backend, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(6 files)

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

https://reviewboard.mozilla.org/r/197986/#review203188
Attachment #8926747 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8926748 [details]
Bug 1415799 - Inline and remove PREF_Cleanup.

https://reviewboard.mozilla.org/r/197988/#review203190
Attachment #8926748 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8926749 [details]
Bug 1415799 - Inline and remove PREF_Init() and PREF_CleanupPrefs().

https://reviewboard.mozilla.org/r/197990/#review203192
Attachment #8926749 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8926750 [details]
Bug 1415799 - Remove PREF_ClearUserPref() forward declaration.

https://reviewboard.mozilla.org/r/197992/#review203194
Attachment #8926750 - Flags: review?(mh+mozilla) → review+
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.
Attachment #8926751 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8926752 [details]
Bug 1415799 - Rename pref_HashPref() as pref_SetPref(). .

https://reviewboard.mozilla.org/r/197996/#review203198
Attachment #8926752 - Flags: review?(mh+mozilla) → review+
> 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?
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 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)"
You need to log in before you can comment on or make changes to this bug.