800690 - Create a template class to cache and subscribe preferences

Status: NEW

(Core :: Graphics, defect)

Description

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Create something to cache preferences so we don't end up with bespoke code all over the place repeating the pattern of checking the preference if we haven't already done so all over the place.

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 1: Clean up white space

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 2: Create a container that caches and subscribes preference values

Comment 3

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 3: Clean up Azure Canvas and Content prefs

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 670687 [details] [diff] [review]
Part 2: Create a container that caches and subscribes preference values

Review of attachment 670687 [details] [diff] [review]:
-----------------------------------------------------------------

Make the string be a template parameter? Then it doesn't have to be stored in the object.

bsmedberg will need to review this.

Comment 5

[Mozilla RelEng Bot]

Try run for 21322ebec200 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=21322ebec200
Results (out of 13 total builds):
    success: 13
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-21322ebec200

Comment 6

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 1: Clean up white space v2

Comment 7

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 2: Create a container that caches and subscribes preference values v2

Comment 8

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 3: Clean up Azure Canvas and Content prefs v2

Comment 9

[Benjamin Smedberg]

Comment on attachment 671592 [details] [diff] [review]
Part 2: Create a container that caches and subscribes preference values v2

Thank you for the patch. I don't think that the API is quite right yet, but I do think that something like this would help make pref usage simpler and avoid code duplication. Catch me on irc `bsmedberg` if you'd like to talk about this in realtime.

1) Does this compile? I'm having trouble understading how

template <const nsDependentCString& tPref, typename T>

could work, since tPref can't be a compile-time constant. I also don't think we want to paramaterize it that way since you'd be generating a new copy of the methods for each pref name.

2) The code here is very similar to Preferences::Add*VarCache. Can we unify the callback mechanism some?

3) I don't think that the Preferences::Get specialization is very intuitive (specializing on return values gets ambiguous quickly). I think it would be better to do all this by specializing PrefContainer instead.

4) PrefContainer.cpp is an empty file and shouldn't exist.

Comment 10

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> Comment on attachment 671592 [details] [diff] [review]
> Part 2: Create a container that caches and subscribes preference values v2
> 
> Thank you for the patch. I don't think that the API is quite right yet, but
> I do think that something like this would help make pref usage simpler and
> avoid code duplication. Catch me on irc `bsmedberg` if you'd like to talk
> about this in realtime.
> 
> 1) Does this compile? I'm having trouble understading how
>
> template <const nsDependentCString& tPref, typename T>

See some usage examples in Part 3.

> could work, since tPref can't be a compile-time constant. I also don't think
> we want to paramaterize it that way since you'd be generating a new copy of
> the methods for each pref name.

The size issue can be eliminated by creating helper methods but that would seem premature at this stage.
 
> 2) The code here is very similar to Preferences::Add*VarCache. Can we unify
> the callback mechanism some?

I had considered that but neither contain complicated logic and the complexity of implementing one in terms of the other seemed to be greater than the waste involved in the overlap. I hope that PrefContainer to be simpler and safer alternative to Add*VarCache on the basis that it protects against observer leakage.

You may have preferred the previous incarnation of the patch, however I followed roc's advice and this lead me to what I consider to be an simpler and better implementation.

Comment 11

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 670687 [details] [diff] [review]
Part 2: Create a container that caches and subscribes preference values

Let's consider the v2 patches a blind alley and consider the original patches instead.

Comment 12

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 2: Create a container that caches and subscribes preference values v3

Rebased for strongly typed nsresult.

Comment 13

[Mozilla RelEng Bot]

Try run for 75d167396c28 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=75d167396c28
Results (out of 2 total builds):
    exception: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-75d167396c28

Comment 14

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 2: Create a container that caches and subscribes preference values v4

Comment 15

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 3: Clean up Azure Canvas and Content prefs v3

Comment 16

[Mozilla RelEng Bot]

Try run for ba26192dafe0 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ba26192dafe0
Results (out of 38 total builds):
    success: 31
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ba26192dafe0

Comment 17

[Benjamin Smedberg]

Comment on attachment 672173 [details] [diff] [review]
Part 2: Create a container that caches and subscribes preference values v4

>diff --git a/modules/libpref/public/PrefContainer.h b/modules/libpref/public/PrefContainer.h

>+/**
>+ * PrefContainer is cached and self updating preference container. It removes
>+ * the need for special code to cache a preference. Rather the preference is
>+ * read and subscribed when the object is created and it is unsubscribed when
>+ * the object is destroyed.
>+ *
>+ * The usage is something like this:
>+ *
>+ *   PrefContainer<boolean> mSelfDestructEnabled;
>+ *
>+ * And in the constructor
>+ *
>+ *   : mSelfDestructEnabled("self.destruct.enabled", true);
>+ *
>+ * It defines a default conversion so you can write:
>+ *
>+ *   if (mSelfDestructEnabled.Get())

Please list the types that PrefContainer is currently specialized on.

>diff --git a/modules/libpref/src/Makefile.in b/modules/libpref/src/Makefile.in
>--- a/modules/libpref/src/Makefile.in
>+++ b/modules/libpref/src/Makefile.in
>@@ -19,16 +19,17 @@ GRE_MODULE	= 1
> LIBXUL_LIBRARY = 1
> 
> CPPSRCS		= \
> 			  nsPrefBranch.cpp	\
> 			  nsPrefsFactory.cpp	\
> 			  prefapi.cpp \
> 			  prefread.cpp \
> 			  Preferences.cpp	\
>+                          PrefContainer.cpp     \
> 			 $(NULL)

nit, either match the tabtabtabspacespace or convert the entire block to spaces

>+template<>
>+void
>+PrefContainerBase<nsCString>::Update()
>+{
>+  if (NS_SUCCEEDED(Preferences::GetCString(mPref, &mValue))) {
>+    mValue = mDefault;

I'm pretty sure you mean NS_FAILED here, no? Too bad that writing internal-linkage compiled code tests is so hard.

r=me with those changes