656826 - Redesign nsPrefService for helpful static utility methods

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: Part.1 Implement the pref related methods v1.0

We should implement pref related mehtods to WidgetUtils.

Now, all widget code get nsIPrefBranch instance manually. This makes unnecessary cost and ugly code. I think that WidgetUtils should have its own nsIPrefBranch instance and all widget code should get/set the pref value via it.

This helps to implement new features which should be killed by pref for aurora.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: Part.2 xpwidgets should use WidgetUtils for accessing prefs v1.0

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: Part.3 windows should use WidgetUtils for accessing prefs v1.0

I'll request review after the part.1 is granted.

Comment 3

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

I think now that we use libxul everywhere you can just use nsContentUtils. It's a slight layering violation; we might actually want to move those nsContentUtils methods to xpcom or somewhere else shareable. Benjamin, what do you think?

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

If we use nsContentUtils.h, it needs to include very many modules, isn't it? So, moving them from nsContentUtils sounds good thing. But I don't have idea for the good place.

To be static methods is good thing for simplifying the callers.

Comment 5

[Makoto Kato [:m_kato]]

(In reply to comment #4)
> If we use nsContentUtils.h, it needs to include very many modules, isn't it?
> So, moving them from nsContentUtils sounds good thing. But I don't have idea
> for the good place.
> 
> To be static methods is good thing for simplifying the callers.

nsContentUtils isn't marked as exported API.  So if shared build, we cannot use it into widget code.

But I don't know whether we can still use shared build.  If this options is no longer work, we can use nsContentUtils in widget.

Comment 6

[Makoto Kato [:m_kato]]

Also, we can only use libxul, we should remove shared options from Makefiles.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Can we make modules/libpref/public/PrefUtils.h and modules/libpref/src/PrefUtils.cpp?

Comment 8

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

There's no such thing as a shared build anymore. libxul is required.

Comment 9

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

(In reply to comment #7)
> Can we make modules/libpref/public/PrefUtils.h and
> modules/libpref/src/PrefUtils.cpp?

That sounds good to me but I'd like bsmedberg to sign off on it.

Comment 10

[(not currently active) Ted Mielczarek]

(In reply to comment #6)
> Also, we can only use libxul, we should remove shared options from Makefiles.

That's covered in bug 648911.

Comment 11

[Benjamin Smedberg]

I am fine with PrefUtils.h/cpp... I'd kinda like us to move all the nsContentUtils callers to PrefUtils, so we don't end up with multiple identical abstraction layers.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: Part.1 Implement PrefUtils

This doesn't implement AddBoolPrefVarCache, AddIntPrefVarCache, RegisterPrefCallback and UnregisterPrefCallback. They are needed when all nsContentUtils functions for prefs replaced with this new class. RegisterPrefCallback and UnregisterPrefCallback were implemented for backward compatibility. So, I'm not sure whether we should implement it on PrefUtils too. Developers should use nsIObserver for them. And the current implementation of Add*PrefVarCache depends on RegisterPrefCallback.

They are not as important as the method which is implemented by this patch. We should discuss about it in another bug when we change nsContentUtils.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: Part.2 xpwidgets should use PrefUtils

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: Part.3 widget for Windows should use PrefUtils

Comment 15

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

(In reply to comment #12)
> This doesn't implement AddBoolPrefVarCache, AddIntPrefVarCache,
> RegisterPrefCallback and UnregisterPrefCallback. They are needed when all
> nsContentUtils functions for prefs replaced with this new class.
> RegisterPrefCallback and UnregisterPrefCallback were implemented for
> backward compatibility. So, I'm not sure whether we should implement it on
> PrefUtils too. Developers should use nsIObserver for them.

RegisterPrefCallback is easier to use than nsIObserver. I think we should have it in PrefUtils.

Comment 16

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

Having nsPrefService::GetInstance() call PrefUtils to create the instance of nsPrefService seems unnecessarily indirect. Why not have nsPrefService::GetInstance() just create the instance itself if needed, and have PrefUtils call GetInstance() as needed?

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

(In reply to comment #15)
> (In reply to comment #12)
> > This doesn't implement AddBoolPrefVarCache, AddIntPrefVarCache,
> > RegisterPrefCallback and UnregisterPrefCallback. They are needed when all
> > nsContentUtils functions for prefs replaced with this new class.
> > RegisterPrefCallback and UnregisterPrefCallback were implemented for
> > backward compatibility. So, I'm not sure whether we should implement it on
> > PrefUtils too. Developers should use nsIObserver for them.
> 
> RegisterPrefCallback is easier to use than nsIObserver. I think we should
> have it in PrefUtils.

But they cannot be tested with widget patches, so, I still think that we should implement them in PrefUtils later. Doesn't this make sense for you?

(In reply to comment #16)
> Having nsPrefService::GetInstance() call PrefUtils to create the instance of
> nsPrefService seems unnecessarily indirect. Why not have
> nsPrefService::GetInstance() just create the instance itself if needed, and
> have PrefUtils call GetInstance() as needed?

Don't we need to release the singleton instance at shutdown? At that time, should PrefUtils::Shutdown() call nsPrefService::Shutdown() and it releases the singleton instance?

Comment 18

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

(In reply to comment #17)
> But they cannot be tested with widget patches, so, I still think that we
> should implement them in PrefUtils later. Doesn't this make sense for you?

Yes.

> (In reply to comment #16)
> > Having nsPrefService::GetInstance() call PrefUtils to create the instance of
> > nsPrefService seems unnecessarily indirect. Why not have
> > nsPrefService::GetInstance() just create the instance itself if needed, and
> > have PrefUtils call GetInstance() as needed?
> 
> Don't we need to release the singleton instance at shutdown? At that time,
> should PrefUtils::Shutdown() call nsPrefService::Shutdown() and it releases
> the singleton instance?

Why not just have UnloadPrefsModule call nsPrefService::Shutdown directly?

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

(In reply to comment #18)
> > (In reply to comment #16)
> > > Having nsPrefService::GetInstance() call PrefUtils to create the instance of
> > > nsPrefService seems unnecessarily indirect. Why not have
> > > nsPrefService::GetInstance() just create the instance itself if needed, and
> > > have PrefUtils call GetInstance() as needed?
> > 
> > Don't we need to release the singleton instance at shutdown? At that time,
> > should PrefUtils::Shutdown() call nsPrefService::Shutdown() and it releases
> > the singleton instance?
> 
> Why not just have UnloadPrefsModule call nsPrefService::Shutdown directly?

Oh, yes.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: Part.1 Implement PrefUtils v1.1

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: Part.1 Implement PrefUtils v1.1

Sorry for the spam. I posted wrong file.

Comment 22

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

I think Get*Pref and Set*Pref names are redundant, we can just have GetBool(...), GetChar(...), SetBool(...), SetChar(...) etc.

I don't think we need the "prefs" namespace, "mozilla" will do.

	return sRootPrefBranch ? PR_TRUE : PR_FALSE;

return sRootPrefBranch != nsnull;

nsPrefService::GetInstance should not addref.

Why does nsPrefService::GetInstance initialize PrefUtils?

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

(In reply to comment #22)
> nsPrefService::GetInstance should not addref.

Really??

http://mxr.mozilla.org/mozilla-central/source/xpcom/components/ModuleUtils.h#103

Looks like we need to addref.

Comment 24

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

OK, the thing that PrefUtils calls to get the instance doesn't need to addref.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: Part.1 Implement PrefUtils v1.2

Hmm... is PrefUtils::ClearUser() good name? Is ClearUserSetValue() or something better name?

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Comment on attachment 533216 [details] [diff] [review]
Part.1 Implement PrefUtils v1.2

Sorry, this has a bug.

Comment 27

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

ClearUser could just be Clear.

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: Part.1 Implement PrefUtils v1.3

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: Part.2 xpwidgets should use PrefUtils

Comment 30

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

I have another question. Why not merge PrefUtils into nsPrefService?

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Merge? I'm not sure what image you have.

PrefUtils.h defines static members. nsPrefService.h defines instance members.

From them, I can guess:

class PrefUtils public : nsPrefService {};

But PrefUtils.h must not include nsPrefService.h because it cannot be included when PrefUtils.h is included from other modules.

Comment 32

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

Can you add the static members of PrefUtils directly to nsPrefService?

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Hmm, so, do you think following style?

PrefUtils::GetBool() just calls nsPrefService::GetBool()?

Comment 34

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

No, I think everyone just calls nsPrefService::GetBool().

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Hmm... The concrete class for a service becomes public?

It sounds strange for me, but it's okay.

For that, I need to rename modules/libpref/src/nsPrefService.h to modules/libpref/public/nsPrefService.h.  At this time, we have a change to rename nsPrefService to mozilla::PrefService and the file name to PrefService.h. Should I do it?

Comment 36

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

It sounds good to me. Benjamin?

Comment 37

[Benjamin Smedberg]

If we're going to expose it, please just make it mozilla::Preferences.

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Let me make sure. Is the Preferences of mozilla::Preferences a class name? Then, the new header file is Preferences.h?

Comment 39

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

Yes

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: Part.1 Rename nsPrefService to Preferences

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: Part.2 Rename nsPrefService.cpp and move nsPrefService.h to public

Comment 42

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: Part.3 Implement static utility methods for Preferences

AddPrefObserver and RemovePrefObserver has "Pref" in their names. However, it's needed for avoiding conflict with nsIPrefBranch2 methods.

Comment 43

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: Part.4 xpwidgets should use new pref utilities

Comment 44

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

Comment on attachment 533896 [details] [diff] [review]
Part.1 Rename nsPrefService to Preferences

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

Comment 45

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

Comment on attachment 533898 [details] [diff] [review]
Part.3 Implement static utility methods for Preferences

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

r+ with those changes

::: modules/libpref/public/Preferences.h
@@ +100,5 @@
> +  /**
> +   * Gets int or bool type pref value with default value if failed to get
> +   * the pref.
> +   */
> +  static PRPackedBool GetBool(const char* aPref, PRBool aDefault = PR_FALSE)

Should just return PRBool

::: modules/libpref/src/Preferences.cpp
@@ +111,5 @@
> +    return sPreferences != nsnull;
> +  }
> +
> +  sPreferences = new Preferences();
> +  NS_ENSURE_TRUE(sPreferences, PR_FALSE);

Unnecessary, 'new' can't return null.

Comment 46

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

Comment on attachment 533899 [details] [diff] [review]
Part.4 xpwidgets should use new pref utilities

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

Comment 47

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

Comment on attachment 533898 [details] [diff] [review]
Part.3 Implement static utility methods for Preferences

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

r+ with those changes

::: modules/libpref/public/Preferences.h
@@ +158,5 @@
> +  static nsresult AddPrefObserver(const char* aPref,
> +                                  nsIObserver* aObserver,
> +                                  PRBool aHoldWeak);
> +  static nsresult RemovePrefObserver(const char* aPref,
> +                                     nsIObserver* aObserver);

Actually, these should just be AddObserver/RemoveObserver.

Comment 48

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

Also, boolean parameters like aHoldWeak are quite bad. How about separate functions AddObserver, AddWeakObserver?

Comment 49

[Josh Matthews [:jdm]]

Could we also standardize on the order of arguments for the Add/RemoveObserver functions, since they're the reverse of nsObserverService's corresponding functions? That always trips me up.

Comment 50

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

(In reply to comment #45)
> Comment on attachment 533898 [details] [diff] [review] [review]
> Part.3 Implement static utility methods for Preferences
> 
> Review of attachment 533898 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> r+ with those changes
> 
> ::: modules/libpref/public/Preferences.h
> @@ +100,5 @@
> > +  /**
> > +   * Gets int or bool type pref value with default value if failed to get
> > +   * the pref.
> > +   */
> > +  static PRPackedBool GetBool(const char* aPref, PRBool aDefault = PR_FALSE)
> 
> Should just return PRBool

nsContentUtils::GetBoolPref() uses PRPackedBool. Isn't PRPackedBool better for replacing the nsContentUtils::GetBoolPref()?
And when we store PRPackedBool member, e.g.,
mPrefVal = Preferences::GetBool("foo.bar");
Then, if the result was PRBool, can't we see warning at build time? (I'm not sure actually)

> ::: modules/libpref/src/Preferences.cpp
> @@ +111,5 @@
> > +    return sPreferences != nsnull;
> > +  }
> > +
> > +  sPreferences = new Preferences();
> > +  NS_ENSURE_TRUE(sPreferences, PR_FALSE);
> 
> Unnecessary, 'new' can't return null.

How about OOM?

Comment 51

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

(In reply to comment #47)
> Comment on attachment 533898 [details] [diff] [review] [review]
> Part.3 Implement static utility methods for Preferences
> 
> Review of attachment 533898 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> r+ with those changes
> 
> ::: modules/libpref/public/Preferences.h
> @@ +158,5 @@
> > +  static nsresult AddPrefObserver(const char* aPref,
> > +                                  nsIObserver* aObserver,
> > +                                  PRBool aHoldWeak);
> > +  static nsresult RemovePrefObserver(const char* aPref,
> > +                                     nsIObserver* aObserver);
> 
> Actually, these should just be AddObserver/RemoveObserver.

As I said in comment 42, Prefrences already have AddObserver and RemoveObserver with same arguments for instance method which is inherited from nsIPrefBranch2.

(In reply to comment #48)
> Also, boolean parameters like aHoldWeak are quite bad. How about separate
> functions AddObserver, AddWeakObserver?

It's okay, but we cannot resolve the name conflict issue for RemoveObserver.

Comment 52

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

(In reply to comment #49)
> Could we also standardize on the order of arguments for the
> Add/RemoveObserver functions, since they're the reverse of
> nsObserverService's corresponding functions? That always trips me up.

Indeed. If we use nsIObserver's order, i.e., (nsIObserver, const char*),
we can resolve the name conflict issue, maybe.

Comment 53

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: Part.3 Implement static utility methods for Preferences

Removed OOM check, changed the order of AddObserver and RemoveObserver and added AddObserverWeak.

I didn't change PRPackedBool to PRBool for the result of GetBool() yet. I wait your reply.

Comment 54

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

(In reply to comment #50)
> nsContentUtils::GetBoolPref() uses PRPackedBool. Isn't PRPackedBool better
> for replacing the nsContentUtils::GetBoolPref()?

I don't think that matters.

> And when we store PRPackedBool member, e.g.,
> mPrefVal = Preferences::GetBool("foo.bar");
> Then, if the result was PRBool, can't we see warning at build time? (I'm not
> sure actually)

I don't think so. We assign PRBools to PRPackedBools all over the place.

> > ::: modules/libpref/src/Preferences.cpp
> > @@ +111,5 @@
> > > +    return sPreferences != nsnull;
> > > +  }
> > > +
> > > +  sPreferences = new Preferences();
> > > +  NS_ENSURE_TRUE(sPreferences, PR_FALSE);
> > 
> > Unnecessary, 'new' can't return null.
> 
> How about OOM?

No. It would abort on OOM.

(In reply to comment #51)
> As I said in comment 42, Prefrences already have AddObserver and
> RemoveObserver with same arguments for instance method which is inherited
> from nsIPrefBranch2.

OK, call them AddStrongObserver/AddWeakObserver. And fix the argument order like Josh suggested!

Comment 55

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: Part.3 Implement static utility methods for Preferences (mq)

Thank you, roc. Carried over the r=roc from attachment 533898 [details] [diff] [review].

Comment 56

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: Part.4 xpwidgets should use new pref utilities (mq)

Comment 57

[Benjamin Smedberg]

Comment on attachment 533897 [details] [diff] [review]
Part.2 Rename nsPrefService.cpp and move nsPrefService.h to public

Please add an include guard of the form:

#ifndef MOZILLA_INTERNAL_API
#error "This header is only usable from within libxul (MOZILLA_INTERNAL_API)."
#endif

to Preferences.h

Comment 58

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 533897 [details] [diff] [review]
Part.2 Rename nsPrefService.cpp and move nsPrefService.h to public

>--- a/modules/libpref/src/nsPrefService.h
>+++ b/modules/libpref/public/Preferences.h
>+#ifndef mozilla_Preferences_h__
>+#define mozilla_Preferences_h__
>+#endif // mozilla_Preferences_h__

Can you please drop the trailing underscores here? Identifiers with two or more consecutive underscores are reserved, and mozilla_Preferences_h would match prevailing style for namespaced headers.

Comment 59

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

http://hg.mozilla.org/mozilla-central/rev/3c9878376550
http://hg.mozilla.org/mozilla-central/rev/0d754af939ec
http://hg.mozilla.org/mozilla-central/rev/83eafb414d2b
http://hg.mozilla.org/mozilla-central/rev/8b60dc275a20

Thank you, roc, Benjamin, Josh and Ms2ger.

Comment 60

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

Don't we need more followup bugs to convert all the nsContentUtils users, and probably most of the other non-scripted pref users, to the new API?

Comment 61

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

(In reply to comment #60)
> Don't we need more followup bugs to convert all the nsContentUtils users,
> and probably most of the other non-scripted pref users, to the new API?

Yes. I'll file them.

Comment 62

[[e-mail]]

Comment on attachment 534219 [details] [diff] [review]
Part.3 Implement static utility methods for Preferences (mq)

>+Preferences::GetChar(const char* aPref, nsCString* aResult)
Why not nsCString& or nsACString& ? (Same goes for other functions)

>+  nsAdoptingCString result;
You only need to use nsAdoptingCString for its char* constructor. Otherwise it works the same way as a regular nsCString.

>+  nsresult rv =
>+    sPreferences->mRootBranch->GetCharPref(aPref, getter_Copies(result));
I take it this is this to avoid writing to aResult if GetCharPref fails?

>+    CopyUTF8toUTF16(result, *aResult);
This is normally a violation of the preferences API, but I guess since this is part of the preferences module it's OK to do this.

>+  if (NS_SUCCEEDED(rv)) {
>+    if (prefLocalString) {
Nit: GetComplexValue never succesfully returns null.

>+Preferences::SetChar(const char* aPref, const nsCString &aValue)
nsCString is already flat. Did you mean const nsACString &aValue?

>+  return SetChar(aPref, nsPromiseFlatCString(aValue).get());
The name of the function is PromiseFlatCString. nsPromiseFlatCString is an implementation detail that you should avoid using directly.

Comment 63

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

(In reply to comment #62)
> Comment on attachment 534219 [details] [diff] [review] [review]
> Part.3 Implement static utility methods for Preferences (mq)
> 
> >+Preferences::GetChar(const char* aPref, nsCString* aResult)
> Why not nsCString& or nsACString& ? (Same goes for other functions)

Because if I used nsA*String, I cannot use ns*AutoString for the param.

> >+  nsAdoptingCString result;
> You only need to use nsAdoptingCString for its char* constructor. Otherwise
> it works the same way as a regular nsCString.

Do you mean I should use nsCString there?

> >+  nsresult rv =
> >+    sPreferences->mRootBranch->GetCharPref(aPref, getter_Copies(result));
> I take it this is this to avoid writing to aResult if GetCharPref fails?

Yes. I think that the parameter shouldn't be modified when the method fails.

> >+  if (NS_SUCCEEDED(rv)) {
> >+    if (prefLocalString) {
> Nit: GetComplexValue never succesfully returns null.

Thanks.

> >+Preferences::SetChar(const char* aPref, const nsCString &aValue)
> nsCString is already flat. Did you mean const nsACString &aValue?

No, intentionally due to the above problem.

> >+  return SetChar(aPref, nsPromiseFlatCString(aValue).get());
> The name of the function is PromiseFlatCString. nsPromiseFlatCString is an
> implementation detail that you should avoid using directly.

Okay.

Comment 64

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

(In reply to comment #63)
> (In reply to comment #62)
> > Comment on attachment 534219 [details] [diff] [review] [review] [review]
> > Part.3 Implement static utility methods for Preferences (mq)
> > 
> > >+Preferences::GetChar(const char* aPref, nsCString* aResult)
> > Why not nsCString& or nsACString& ? (Same goes for other functions)
> 
> Because if I used nsA*String, I cannot use ns*AutoString for the param.

Um, I may be wrong for this. I'll confirm this again later.

Comment 65

[[e-mail]]

(In reply to comment #63)
> (In reply to comment #62)
> > (From update of attachment 534219 [details] [diff] [review])
> > >+Preferences::GetChar(const char* aPref, nsCString* aResult)
> > Why not nsCString& or nsACString& ? (Same goes for other functions)
> Because if I used nsA*String, I cannot use ns*AutoString for the param.
I wasn't too worried about the type (although AutoString derives from String, so that's not actually a problem), but the use of pointers looks odd.

> > >+  nsAdoptingCString result;
> > You only need to use nsAdoptingCString for its char* constructor. Otherwise
> > it works the same way as a regular nsCString.
> Do you mean I should use nsCString there?
My mistake, it works the same way as an nsXPIDLCString, which means that its .get() can return null if the string is void, and it defaults to void. I'm not sure whether you're expecting that behaviour or not.

> > >+Preferences::SetChar(const char* aPref, const nsCString &aValue)
> > nsCString is already flat. Did you mean const nsACString &aValue?
> No, intentionally due to the above problem.
In that case the (ns)PromiseFlatCString is unnecessary.

Comment 66

[[e-mail]]

Comment on attachment 534220 [details] [diff] [review]
Part.4 xpwidgets should use new pref utilities (mq)

>+      if (NS_HexToRGB(nsDependentString(
>+                        Substring(colorStr, 1, colorStr.Length() - 1)),
>+                      &thecolor)) {
You normally don't want to make a dependent string from a substring. I know it's safe in this case, but it's not in general. Maybe I should file a bug to we make Substring(colorStr, 1) return an nsDependentString directly. In the mean time the most accurate code I could come up with was nsDependentString(colorStr.get() + 1, colorStr.Length() - 1)

Comment 67

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

(In reply to comment #62)
> >+Preferences::GetChar(const char* aPref, nsCString* aResult)
> Why not nsCString& or nsACString& ? (Same goes for other functions)
> 
> >+  nsAdoptingCString result;
> You only need to use nsAdoptingCString for its char* constructor. Otherwise
> it works the same way as a regular nsCString.
> 
> >+  if (NS_SUCCEEDED(rv)) {
> >+    if (prefLocalString) {
> Nit: GetComplexValue never succesfully returns null.
> 
> >+Preferences::SetChar(const char* aPref, const nsCString &aValue)
> nsCString is already flat. Did you mean const nsACString &aValue?

These issues will be fixed by the patch for bug 659820 (part.1 and part.2).

Comment 68

[[e-mail]]

(In reply to comment #66)
>(From update of attachment 534220 [details] [diff] [review])
>>+      if (NS_HexToRGB(nsDependentString(
>>+                        Substring(colorStr, 1, colorStr.Length() - 1)),
>>+                      &thecolor)) {
>You normally don't want to make a dependent string from a substring. I know
>it's safe in this case, but it's not in general. Maybe I should file a bug
>to we make Substring(colorStr, 1) return an nsDependentString directly. In
>the mean time the most accurate code I could come up with was
>nsDependentString(colorStr.get() + 1, colorStr.Length() - 1)
I'm consdering filing a bug so you can write nsDependentString(colorStr, 1)

Comment 69

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

(In reply to comment #67)
> (In reply to comment #62)
> > >+Preferences::GetChar(const char* aPref, nsCString* aResult)
> > Why not nsCString& or nsACString& ? (Same goes for other functions)
> > 
> > >+  nsAdoptingCString result;
> > You only need to use nsAdoptingCString for its char* constructor. Otherwise
> > it works the same way as a regular nsCString.
> > 
> > >+  if (NS_SUCCEEDED(rv)) {
> > >+    if (prefLocalString) {
> > Nit: GetComplexValue never succesfully returns null.
> > 
> > >+Preferences::SetChar(const char* aPref, const nsCString &aValue)
> > nsCString is already flat. Did you mean const nsACString &aValue?
> 
> These issues will be fixed by the patch for bug 659820 (part.1 and part.2).

and

>>+  return SetChar(aPref, nsPromiseFlatCString(aValue).get());
> The name of the function is PromiseFlatCString. nsPromiseFlatCString is an implementation detail that you should avoid using directly.

are now fixed.

Comment 70

[[e-mail]]

Comment on attachment 534220 [details] [diff] [review]
Part.4 xpwidgets should use new pref utilities (mq)

> static bool
>-GetPrefValueForDriverVersion(nsACString& aVersion)
>+GetPrefValueForDriverVersion(nsCString& aVersion)
> {
>-  nsCOMPtr<nsIPrefBranch2> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
>-  if (prefs) {
>-    nsXPIDLCString version;
>-    if (NS_SUCCEEDED(prefs->GetCharPref(SUGGESTED_VERSION_PREF,
>-                                        getter_Copies(version)))) {
>-      aVersion = version;
>-      return true;
>-    }
>-  }
>-
>-  return false;
>+  return NS_SUCCEEDED(Preferences::GetChar(SUGGESTED_VERSION_PREF, &aVersion));
> }
> 
> static void
>-SetPrefValueForDriverVersion(const nsAString& aVersion)
>+SetPrefValueForDriverVersion(const nsString& aVersion)
> {
>-  nsCOMPtr<nsIPrefBranch2> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
>-  if (prefs) {
>-    nsCAutoString ver = NS_LossyConvertUTF16toASCII(aVersion);
>-    prefs->SetCharPref(SUGGESTED_VERSION_PREF,
>-                       PromiseFlatCString(ver).get());
>-  }
>+  Preferences::SetChar(SUGGESTED_VERSION_PREF, aVersion);
> }
Confusingly, GetPrefValueForDriverVersion returns an nsACString which the caller then passes to NS_LossyConvertASCIItoUTF16, while SetPrefValueForDriverVersion used to do the wide/narrow conversion itself. Now it the string really is ASCII then the switch from NS_LossyConvertUTF16toASCII to Preferences::SetChar (which is a UTF8 conversion) won't actually break anything, but I guess it's not a good idea to mix the two. I'll file a bug.

Comment 71

[Eric Shepherd [:sheppy]]

So the gist of this -- from a docs standpoint -- is to document the new static preference routines, correct?

Comment 72

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Yes, and maybe a note in the style guide to prefer those over handling prefs through the XPCOM interfaces.

Comment 73

[Eric Shepherd [:sheppy]]

Are the static functions preferred for both app code and add-ons, or just app code?

Comment 74

[Benjamin Smedberg]

The static functions are only available to internal code (libxul-internal), and are not available to app code or addons.

Comment 75

[Eric Shepherd [:sheppy]]

That sounds like it's pretty low priority to document, then. I'll leave it on the list but won't likely do it very soon.

Comment 76

[Eric Shepherd [:sheppy]]

I've written a page to help people find this API; it's not complete documentation because, well, only a few people really need this, and there's so much other stuff to do. :)

https://developer.mozilla.org/en/Preferences/Using_preferences_from_application_code

Added a link to:

https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide

And mentioned on Firefox 6 for developers.