Closed Bug 1416638 Opened 2 years ago Closed 2 years ago

Remove PREF_* layer in libpref (part 1)

Categories

(Core :: Preferences: Backend, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(9 files)

59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
libpref has three layers for most of its operations:

- PREF_*, which is the lowest level that does most of the work.

- Preferences, which is the C++ interface, and which calls directly into PREF_*.

- nsPrefBranch, which is the XPIDL interface, and which calls into PREF_* after prepending the branch prefix.

This is one more layer than necessary, and a consequence of the old multi-file structure for libpref. In this bug I'm going to merge the PREF_* layer into Preferences, and make nsPrefBranch calls into Preferences.
Comment on attachment 8928087 [details]
Bug 1416638 - Introduce PrefValueKind.

https://reviewboard.mozilla.org/r/199324/#review204638
Attachment #8928087 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8928088 [details]
Bug 1416638 - Add a PrefValueKind arg to Preferences::Set*().

https://reviewboard.mozilla.org/r/199326/#review204642

::: modules/libpref/Preferences.cpp:4692
(Diff revision 1)
>                          const nsIID& aType,
> -                        nsISupports* aValue)
> +                        nsISupports* aValue,
> +                        PrefValueKind aKind)
>  {
>    NS_ENSURE_TRUE(InitStaticMembers(), NS_ERROR_NOT_AVAILABLE);
> -  return sPreferences->mRootBranch->SetComplexValue(aPref, aType, aValue);
> +  nsCOMPtr<nsIPrefBranch> branch = (aKind == PrefValueKind::Default)

You don't really need a nsCOMPtr here.
Attachment #8928088 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8928089 [details]
Bug 1416638 - Add a PrefValueKind arg to Preferences::Get*(), and remove Preferences::GetDefault*().

https://reviewboard.mozilla.org/r/199328/#review204644

::: modules/libpref/Preferences.h:94
(Diff revision 1)
>      NS_ENSURE_TRUE(InitStaticMembers(), nullptr);
>      return sPreferences;
>    }
>  
>    // Returns shared pref branch instance. NOTE: not addreffed.
> -  static nsIPrefBranch* GetRootBranch()
> +  static nsIPrefBranch* GetRootBranch(PrefValueKind aKind = PrefValueKind::User)

Maybe GetBranch?

::: modules/libpref/Preferences.cpp:4618
(Diff revision 1)
> +                                nsAString& aResult,
> +                                PrefValueKind aKind)
>  {
>    NS_ENSURE_TRUE(InitStaticMembers(), NS_ERROR_NOT_AVAILABLE);
>    nsCOMPtr<nsIPrefLocalizedString> prefLocalString;
> -  nsresult rv = sPreferences->mRootBranch->GetComplexValue(
> +  nsCOMPtr<nsIPrefBranch> branch = (aKind == PrefValueKind::Default)

You could use Get(Root)Branch here (and in the function that has the same code from previous patch). And you don't really need a nsCOMPtr.

::: modules/libpref/Preferences.cpp:4638
(Diff revision 1)
> -  return sPreferences->mRootBranch->GetComplexValue(aPref, aType, aResult);
> +  nsCOMPtr<nsIPrefBranch> branch = (aKind == PrefValueKind::Default)
> +                                     ? sPreferences->mDefaultRootBranch
> +                                     : sPreferences->mRootBranch;

ditto

::: modules/libpref/Preferences.cpp:4742
(Diff revision 1)
> +  nsCOMPtr<nsIPrefBranch> branch = (aKind == PrefValueKind::Default)
> +                                     ? sPreferences->mDefaultRootBranch
> +                                     : sPreferences->mRootBranch;

ditto
Attachment #8928089 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8928090 [details]
Bug 1416638 - Change nsPrefBranch mIsDefault to a PrefValueKind. .

https://reviewboard.mozilla.org/r/199330/#review204648

::: modules/libpref/Preferences.cpp:2059
(Diff revision 1)
>    PrefName GetPrefName(const char* aPrefName) const;
>  
>    void FreeObserverList(void);
>  
>    const nsCString mPrefRoot;
> -  bool mIsDefault;
> +  PrefValueKind mKind;

note that PrefValueKind, being an enum, is going to be an int, and that will make this class slightly larger. It probably doesn't matter, though. But you might want to make PrefValueKind a bool.
Attachment #8928090 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8928091 [details]
Bug 1416638 - Use `private` instead of `protected` in Preferences. .

https://reviewboard.mozilla.org/r/199332/#review204650
Attachment #8928091 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8928092 [details]
Bug 1416638 - Move a couple of functions into Preferences. .

https://reviewboard.mozilla.org/r/199334/#review204652
Attachment #8928092 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8928093 [details]
Bug 1416638 - Rename SetCharPrefInternal() as SetCharPrefNoLengthCheck(). .

https://reviewboard.mozilla.org/r/199336/#review204654
Attachment #8928093 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8928094 [details]
Bug 1416638 - Inline and remove PREF_Set*(). .

https://reviewboard.mozilla.org/r/199338/#review204656

::: modules/libpref/Preferences.h:206
(Diff revision 1)
> +
>    static nsresult SetFloat(const char* aPrefName,
>                             float aValue,
> -                           PrefValueKind aKind = PrefValueKind::User);
> +                           PrefValueKind aKind = PrefValueKind::User)
> +  {
> +    return SetCString(aPrefName, nsPrintfCString("%f", aValue).get(), aKind);

Note this might lead to larger code on the call site for these functions. It might be desirable to still keep them defined in the cpp.

::: modules/libpref/Preferences.h:401
(Diff revision 1)
>  
>    static nsresult SetValueFromDom(const char* aPrefName,
>                                    const dom::PrefValue& aValue,
>                                    PrefValueKind aKind);
>  
> +  static nsresult SetBoolInAnyProcess(

It's not clear what "InAnyProcess" refers to. Please add comments.
Attachment #8928094 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8928095 [details]
Bug 1416638 - Inline and remove PREF_Get*(). .

https://reviewboard.mozilla.org/r/199340/#review204668

I'll note there's still a lot of duplication between all those Get methods.
Attachment #8928095 - Flags: review?(mh+mozilla) → review+
> > -  static nsIPrefBranch* GetRootBranch()
> > +  static nsIPrefBranch* GetRootBranch(PrefValueKind aKind = PrefValueKind::User)
> 
> Maybe GetBranch?

I think the "Root" is useful. nsIPrefService has a method getBranch(), and it takes a prefix, so I think it's worth distinguishing this case.
> But you might want to make PrefValueKind a bool.

I didn't know you could do that. Good suggestion.
Why don't you use `enum class PrefValueKind : uint8_t`? It will resolve both the size issue and the readability issue.
> Note this might lead to larger code on the call site for these functions. It
> might be desirable to still keep them defined in the cpp.

It might lead to larger code, but the number of callsites is low, so I don't think it's a problem.


> It's not clear what "InAnyProcess" refers to. Please add comments.

I will. It means it can run in any process, not just the parent process (in contrast with the other setters).
> Why don't you use `enum class PrefValueKind : uint8_t`?

Turns out you can use `enum class PrefValueKind : bool`, which is what I have done.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a52f616fdb98e9b92cbf62127a900bcbf535fb5f
Bug 1416638 - Introduce PrefValueKind. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/bac6c8bb402836e11f4b0eb5f201dde08c97eeaf
Bug 1416638 - Add a PrefValueKind arg to Preferences::Set*(). r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/df10cef47ecc86603ac88a87045bce6b78a5af76
Bug 1416638 - Add a PrefValueKind arg to Preferences::Get*(), and remove Preferences::GetDefault*(). r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/b163b82f0875aa0ac7428a18e8fb31cbfd0e55bd
Bug 1416638 - Change nsPrefBranch mIsDefault to a PrefValueKind. r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfa9ac92acf5ccc803f0614db7e567b2c73fd4fe
Bug 1416638 - Use `private` instead of `protected` in Preferences. r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/46af1340907eb154c9dd63bb13b1d250b48ca1bb
Bug 1416638 - Move a couple of functions into Preferences. r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b8881153e11c1f09efb758be605a695e0e527a0
Bug 1416638 - Rename SetCharPrefInternal() as SetCharPrefNoLengthCheck(). r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5f54a05259edee7d7b4f9205bef9e8d240049f
Bug 1416638 - Inline and remove PREF_Set*(). r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/33a809bfb84082255572bb5c719603f9830645eb
Bug 1416638 - Inline and remove PREF_Get*(). r=glandium.
This has landed, but there is more PREF_* removal to be done, which I will do in a follow-up bug.
Summary: Remove PREF_* layer in libpref → Remove PREF_* layer in libpref (part 1)
You need to log in before you can comment on or make changes to this bug.