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.

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

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?

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.

(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.

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

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

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

(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.

(In reply to comment #6) > Also, we can only use libxul, we should remove shared options from Makefiles. That's covered in bug 648911.

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.

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.

(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.

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?

(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?

(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?

(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.

Sorry for the spam. I posted wrong file.

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?

(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.

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

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

Comment on attachment 533216 [details] [diff] [review] Part.1 Implement PrefUtils v1.2 Sorry, this has a bug.

ClearUser could just be Clear.

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

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.

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

Hmm, so, do you think following style? PrefUtils::GetBool() just calls nsPrefService::GetBool()?

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

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?

It sounds good to me. Benjamin?

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

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

Yes

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

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

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 on attachment 533899 [details] [diff] [review] Part.4 xpwidgets should use new pref utilities Review of attachment 533899 [details] [diff] [review]: -----------------------------------------------------------------

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.

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

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.

(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?

(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.

(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.

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.

(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!

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

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 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.

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.

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?

(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 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.

(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.

(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.

(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 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)

(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).

(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)

(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 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.

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

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

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

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

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.

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.