Last Comment Bug 656826 - Redesign nsPrefService for helpful static utility methods
: Redesign nsPrefService for helpful static utility methods
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Preferences: Backend (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
Mentors:
Depends on: 666381
Blocks: 658703 658704 658705 659533 659536 659820 659821 659913 660121 660742 660743 660768 660770 660771 661101 663036 663039 663041 664437 664906 664908 664969 666901 666903 668157 668931 668933 720134
  Show dependency treegraph
 
Reported: 2011-05-12 19:20 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2012-01-21 08:21 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part.1 Implement the pref related methods v1.0 (12.51 KB, patch)
2011-05-12 19:20 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part.2 xpwidgets should use WidgetUtils for accessing prefs v1.0 (58.48 KB, patch)
2011-05-12 19:21 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part.3 windows should use WidgetUtils for accessing prefs v1.0 (16.75 KB, patch)
2011-05-12 19:22 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part.1 Implement PrefUtils (17.59 KB, patch)
2011-05-17 15:58 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part.2 xpwidgets should use PrefUtils (59.06 KB, patch)
2011-05-17 15:59 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part.3 widget for Windows should use PrefUtils (17.17 KB, patch)
2011-05-17 16:00 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part.1 Implement PrefUtils v1.1 (22.33 KB, patch)
2011-05-17 21:15 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part.1 Implement PrefUtils v1.1 (18.16 KB, patch)
2011-05-17 21:17 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part.1 Implement PrefUtils v1.2 (16.57 KB, patch)
2011-05-18 02:10 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
masayuki: review-
Details | Diff | Splinter Review
Part.1 Implement PrefUtils v1.3 (16.43 KB, patch)
2011-05-18 06:47 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part.2 xpwidgets should use PrefUtils (58.47 KB, patch)
2011-05-18 06:50 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part.1 Rename nsPrefService to Preferences (14.38 KB, patch)
2011-05-20 00:21 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
roc: review+
benjamin: review+
Details | Diff | Splinter Review
Part.2 Rename nsPrefService.cpp and move nsPrefService.h to public (4.47 KB, patch)
2011-05-20 00:23 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
roc: review+
benjamin: review+
Details | Diff | Splinter Review
Part.3 Implement static utility methods for Preferences (11.45 KB, patch)
2011-05-20 00:26 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
roc: review+
Details | Diff | Splinter Review
Part.4 xpwidgets should use new pref utilities (58.90 KB, patch)
2011-05-20 00:27 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
roc: review+
Details | Diff | Splinter Review
Part.3 Implement static utility methods for Preferences (11.56 KB, patch)
2011-05-20 17:00 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part.3 Implement static utility methods for Preferences (mq) (10.10 KB, patch)
2011-05-21 04:35 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part.4 xpwidgets should use new pref utilities (mq) (45.99 KB, patch)
2011-05-21 04:36 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-12 19:20:00 PDT
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.
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-12 19:21:17 PDT
Created attachment 532098 [details] [diff] [review]
Part.2 xpwidgets should use WidgetUtils for accessing prefs v1.0
Comment 2 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-12 19:22:53 PDT
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.
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-12 20:05:15 PDT
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] (Mozilla Japan) 2011-05-12 20:22:53 PDT
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] 2011-05-12 21:14:00 PDT
(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] 2011-05-12 21:14:50 PDT
Also, we can only use libxul, we should remove shared options from Makefiles.
Comment 7 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-12 21:24:07 PDT
Can we make modules/libpref/public/PrefUtils.h and modules/libpref/src/PrefUtils.cpp?
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-12 21:43:31 PDT
There's no such thing as a shared build anymore. libxul is required.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-12 21:43:58 PDT
(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 Ted Mielczarek [:ted.mielczarek] 2011-05-13 04:51:58 PDT
(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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2011-05-13 07:10:15 PDT
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] (Mozilla Japan) 2011-05-17 15:58:43 PDT
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.
Comment 13 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-17 15:59:57 PDT
Created attachment 533101 [details] [diff] [review]
Part.2 xpwidgets should use PrefUtils
Comment 14 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-17 16:00:57 PDT
Created attachment 533102 [details] [diff] [review]
Part.3 widget for Windows should use PrefUtils
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-17 16:07:39 PDT
(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) (Exited; email my personal email if necessary) 2011-05-17 16:23:01 PDT
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] (Mozilla Japan) 2011-05-17 16:53:40 PDT
(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) (Exited; email my personal email if necessary) 2011-05-17 17:02:17 PDT
(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] (Mozilla Japan) 2011-05-17 17:44:35 PDT
(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] (Mozilla Japan) 2011-05-17 21:15:56 PDT
Created attachment 533173 [details] [diff] [review]
Part.1 Implement PrefUtils v1.1
Comment 21 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-17 21:17:20 PDT
Created attachment 533175 [details] [diff] [review]
Part.1 Implement PrefUtils v1.1

Sorry for the spam. I posted wrong file.
Comment 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-17 21:31:54 PDT
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] (Mozilla Japan) 2011-05-17 21:34:27 PDT
(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) (Exited; email my personal email if necessary) 2011-05-17 21:47:11 PDT
OK, the thing that PrefUtils calls to get the instance doesn't need to addref.
Comment 25 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-18 02:10:55 PDT
Created attachment 533216 [details] [diff] [review]
Part.1 Implement PrefUtils v1.2

Hmm... is PrefUtils::ClearUser() good name? Is ClearUserSetValue() or something better name?
Comment 26 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-18 02:13:08 PDT
Comment on attachment 533216 [details] [diff] [review]
Part.1 Implement PrefUtils v1.2

Sorry, this has a bug.
Comment 27 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-18 02:23:57 PDT
ClearUser could just be Clear.
Comment 28 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-18 06:47:10 PDT
Created attachment 533270 [details] [diff] [review]
Part.1 Implement PrefUtils v1.3
Comment 29 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-18 06:50:23 PDT
Created attachment 533272 [details] [diff] [review]
Part.2 xpwidgets should use PrefUtils
Comment 30 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-18 14:42:34 PDT
I have another question. Why not merge PrefUtils into nsPrefService?
Comment 31 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-18 16:23:43 PDT
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) (Exited; email my personal email if necessary) 2011-05-18 16:24:30 PDT
Can you add the static members of PrefUtils directly to nsPrefService?
Comment 33 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-18 16:48:56 PDT
Hmm, so, do you think following style?

PrefUtils::GetBool() just calls nsPrefService::GetBool()?
Comment 34 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-18 17:21:10 PDT
No, I think everyone just calls nsPrefService::GetBool().
Comment 35 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-18 17:39:34 PDT
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) (Exited; email my personal email if necessary) 2011-05-18 17:44:42 PDT
It sounds good to me. Benjamin?
Comment 37 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2011-05-18 17:45:36 PDT
If we're going to expose it, please just make it mozilla::Preferences.
Comment 38 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-18 17:54:19 PDT
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) (Exited; email my personal email if necessary) 2011-05-18 18:32:19 PDT
Yes
Comment 40 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-20 00:21:55 PDT
Created attachment 533896 [details] [diff] [review]
Part.1 Rename nsPrefService to Preferences
Comment 41 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-20 00:23:37 PDT
Created attachment 533897 [details] [diff] [review]
Part.2 Rename nsPrefService.cpp and move nsPrefService.h to public
Comment 42 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-20 00:26:36 PDT
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.
Comment 43 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-20 00:27:55 PDT
Created attachment 533899 [details] [diff] [review]
Part.4 xpwidgets should use new pref utilities
Comment 44 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-20 06:19:46 PDT
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) (Exited; email my personal email if necessary) 2011-05-20 06:25:07 PDT
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) (Exited; email my personal email if necessary) 2011-05-20 06:27:32 PDT
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) (Exited; email my personal email if necessary) 2011-05-20 06:28:51 PDT
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) (Exited; email my personal email if necessary) 2011-05-20 06:29:37 PDT
Also, boolean parameters like aHoldWeak are quite bad. How about separate functions AddObserver, AddWeakObserver?
Comment 49 Josh Matthews [:jdm] 2011-05-20 09:07:28 PDT
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] (Mozilla Japan) 2011-05-20 15:50:29 PDT
(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] (Mozilla Japan) 2011-05-20 15:54:10 PDT
(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] (Mozilla Japan) 2011-05-20 15:57:12 PDT
(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] (Mozilla Japan) 2011-05-20 17:00:18 PDT
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.
Comment 54 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-20 23:17:14 PDT
(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] (Mozilla Japan) 2011-05-21 04:35:53 PDT
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].
Comment 56 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-21 04:36:31 PDT
Created attachment 534220 [details] [diff] [review]
Part.4 xpwidgets should use new pref utilities (mq)
Comment 57 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2011-05-23 05:45:24 PDT
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 (⌚ UTC+1/+2) 2011-05-23 05:53:52 PDT
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 60 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-23 18:22:50 PDT
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] (Mozilla Japan) 2011-05-23 18:25:09 PDT
(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 neil@parkwaycc.co.uk 2011-05-24 03:17:15 PDT
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] (Mozilla Japan) 2011-05-24 05:10:17 PDT
(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] (Mozilla Japan) 2011-05-24 21:40:54 PDT
(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 neil@parkwaycc.co.uk 2011-05-25 02:03:25 PDT
(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 neil@parkwaycc.co.uk 2011-05-26 06:08:32 PDT
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] (Mozilla Japan) 2011-05-26 07:02:42 PDT
(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 neil@parkwaycc.co.uk 2011-05-26 08:09:04 PDT
(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] (Mozilla Japan) 2011-05-26 21:59:38 PDT
(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 neil@parkwaycc.co.uk 2011-06-22 13:59:49 PDT
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] 2011-07-14 10:06:24 PDT
So the gist of this -- from a docs standpoint -- is to document the new static preference routines, correct?
Comment 72 :Ms2ger (⌚ UTC+1/+2) 2011-07-14 10:26:21 PDT
Yes, and maybe a note in the style guide to prefer those over handling prefs through the XPCOM interfaces.
Comment 73 Eric Shepherd [:sheppy] 2011-07-14 10:27:40 PDT
Are the static functions preferred for both app code and add-ons, or just app code?
Comment 74 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2011-07-17 11:52:54 PDT
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] 2011-07-17 13:36:19 PDT
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] 2011-08-04 06:44:43 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.