Redesign nsPrefService for helpful static utility methods

RESOLVED FIXED in mozilla6

Status

()

Core
Preferences: Backend
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({dev-doc-complete})

Trunk
mozilla6
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 14 obsolete attachments)

14.38 KB, patch
roc
: review+
bsmedberg
: review+
Details | Diff | Splinter Review
4.47 KB, patch
roc
: review+
bsmedberg
: review+
Details | Diff | Splinter Review
10.10 KB, patch
Details | Diff | Splinter Review
45.99 KB, patch
Details | Diff | Splinter Review
Created attachment 532097 [details] [diff] [review]
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.
Attachment #532097 - Flags: review?(roc)
Created attachment 532098 [details] [diff] [review]
Part.2 xpwidgets should use WidgetUtils for accessing prefs v1.0
Attachment #532098 - Flags: review?(roc)
Created attachment 532100 [details] [diff] [review]
Part.3 windows should use WidgetUtils for accessing prefs v1.0

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.
Component: Widget → Preferences: Backend
QA Contact: general → preferences-backend
Summary: Implement pref utils on WidgetUtils → Implement PrefUtils
Created attachment 533100 [details] [diff] [review]
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.
Attachment #532097 - Attachment is obsolete: true
Attachment #532097 - Flags: review?(roc)
Attachment #533100 - Flags: review?(roc)
Created attachment 533101 [details] [diff] [review]
Part.2 xpwidgets should use PrefUtils
Attachment #532098 - Attachment is obsolete: true
Attachment #532098 - Flags: review?(roc)
Created attachment 533102 [details] [diff] [review]
Part.3 widget for Windows should use PrefUtils
Attachment #532100 - Attachment is obsolete: true
(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.
Created attachment 533173 [details] [diff] [review]
Part.1 Implement PrefUtils v1.1
Attachment #533100 - Attachment is obsolete: true
Attachment #533100 - Flags: review?(roc)
Attachment #533173 - Flags: review?(roc)
Created attachment 533175 [details] [diff] [review]
Part.1 Implement PrefUtils v1.1

Sorry for the spam. I posted wrong file.
Attachment #533173 - Attachment is obsolete: true
Attachment #533173 - Flags: review?(roc)
Attachment #533175 - Flags: review?(roc)
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.
Created attachment 533216 [details] [diff] [review]
Part.1 Implement PrefUtils v1.2

Hmm... is PrefUtils::ClearUser() good name? Is ClearUserSetValue() or something better name?
Attachment #533175 - Attachment is obsolete: true
Attachment #533175 - Flags: review?(roc)
Attachment #533216 - Flags: review?(roc)
Comment on attachment 533216 [details] [diff] [review]
Part.1 Implement PrefUtils v1.2

Sorry, this has a bug.
Attachment #533216 - Flags: review?(roc) → review-
ClearUser could just be Clear.
Created attachment 533270 [details] [diff] [review]
Part.1 Implement PrefUtils v1.3
Attachment #533216 - Attachment is obsolete: true
Created attachment 533272 [details] [diff] [review]
Part.2 xpwidgets should use PrefUtils
Attachment #533101 - Attachment is obsolete: true
Attachment #533272 - Flags: review?(roc)
Attachment #533270 - Flags: review?(roc)
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
Created attachment 533896 [details] [diff] [review]
Part.1 Rename nsPrefService to Preferences
Attachment #533102 - Attachment is obsolete: true
Attachment #533270 - Attachment is obsolete: true
Attachment #533272 - Attachment is obsolete: true
Attachment #533270 - Flags: review?(roc)
Attachment #533272 - Flags: review?(roc)
Attachment #533896 - Flags: review?(roc)
Attachment #533896 - Flags: review?(benjamin)
Created attachment 533897 [details] [diff] [review]
Part.2 Rename nsPrefService.cpp and move nsPrefService.h to public
Attachment #533897 - Flags: review?(roc)
Attachment #533897 - Flags: review?(benjamin)
Created attachment 533898 [details] [diff] [review]
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.
Attachment #533898 - Flags: review?(roc)
Created attachment 533899 [details] [diff] [review]
Part.4 xpwidgets should use new pref utilities
Attachment #533899 - Flags: review?(roc)
Comment on attachment 533896 [details] [diff] [review]
Part.1 Rename nsPrefService to Preferences

Review of attachment 533896 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #533896 - Flags: review?(roc) → review+
Attachment #533897 - Flags: review?(roc) → 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.
Attachment #533898 - Flags: review?(roc) → review+
Comment on attachment 533899 [details] [diff] [review]
Part.4 xpwidgets should use new pref utilities

Review of attachment 533899 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #533899 - Flags: review?(roc) → 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.
Blocks: 658703
Blocks: 658704
Blocks: 658705
Created attachment 534157 [details] [diff] [review]
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.
Attachment #533898 - Attachment is obsolete: true
Attachment #534157 - Flags: review?(roc)
Summary: Implement PrefUtils → Redesign nsPrefService for helpful static utility methods
(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!
Created attachment 534219 [details] [diff] [review]
Part.3 Implement static utility methods for Preferences (mq)

Thank you, roc. Carried over the r=roc from attachment 533898 [details] [diff] [review].
Attachment #534157 - Attachment is obsolete: true
Attachment #534157 - Flags: review?(roc)
Created attachment 534220 [details] [diff] [review]
Part.4 xpwidgets should use new pref utilities (mq)
Attachment #533899 - Attachment is obsolete: true
Attachment #533896 - Flags: review?(benjamin) → 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
Attachment #533897 - Flags: review?(benjamin) → review+
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
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.
Blocks: 659533
Blocks: 659536
(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.
Blocks: 659820
Blocks: 659821
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)
Blocks: 659913
(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)
Blocks: 660121
(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.

Updated

6 years ago
Keywords: dev-doc-needed
Blocks: 660742
Blocks: 660743
Blocks: 660768
Blocks: 660770
Blocks: 660771
Blocks: 661101
Blocks: 663036
Blocks: 663039
Blocks: 663041
Blocks: 664437
Blocks: 664906
Blocks: 664908
Blocks: 664969
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.

Updated

6 years ago
Depends on: 666381
Blocks: 666901
Blocks: 666903
Blocks: 668157
Blocks: 668931
Blocks: 668933
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.
Keywords: dev-doc-needed → dev-doc-complete

Updated

6 years ago
Blocks: 720134
You need to log in before you can comment on or make changes to this bug.