Comment on attachment 535230 [details] [diff] [review] Part.1 Change the string params of Preferences class from ns*String to nsA*String Review of attachment 535230 [details] [diff] [review]: -----------------------------------------------------------------

Hmm. Actually, can we have these methods (and those in bug 659821) return a string as a direct result, the way the nsContentUtils methods do?

GetChar() and GetLocalizedChar() are for nsCString. GetString() and GetLocalizedString() are for nsString. nsAdopting*String steals the buffer of original class. So, they are better for the return value.

Sorry, I posted old patch.

Comment on attachment 535310 [details] [diff] [review] Part.2 Re-implement Preferences::Get(Char|LocalizedString)() Review of attachment 535310 [details] [diff] [review]: ----------------------------------------------------------------- I think the GetChar versions should be GetCString.

Comment on attachment 535538 [details] [diff] [review] Part.2 Reimplement Preferences::(G|S)et(Char|LoclizedString)() to Preferences::(G|S)et(Localized)CString() Review of attachment 535538 [details] [diff] [review]: -----------------------------------------------------------------

Moved from bug 659821 for clearing order of the patches.

Comment on attachment 535548 [details] [diff] [review] Part.4 Do replace Review of attachment 535548 [details] [diff] [review]: -----------------------------------------------------------------

http://hg.mozilla.org/try/rev/8d902bc01cc2 http://hg.mozilla.org/try/rev/bb51adca98df http://hg.mozilla.org/try/rev/47df42ac63af http://hg.mozilla.org/try/rev/8db79b27469e

Is there a reason the GetString() functions take a string pointer instead of a writable string reference like all other string getters in our code?

This approach makes for shorter caller code.

Why? Passing |str| and passing |&str| should be the same speed....

I meant the same length...

GetBool() and GetInt() have two style methods: static PRBool GetBool(const char* aPref, PRBool aDefault = PR_FALSE); static PRInt32 GetInt(const char* aPref, PRInt32 aDefault = 0); and static nsresult GetBool(const char* aPref, PRBool* aResult); static nsresult GetInt(const char* aPref, PRInt32* aResult); For overloading, the aResult must be pointer. On the other hand, string related methods have following two styles: static nsAdoptingCString GetCString(const char* aPref); static nsAdoptingString GetString(const char* aPref); and static nsresult GetCString(const char* aPref, nsACString* aResult); static nsresult GetString(const char* aPref, nsAString* aResult); So, string related methods don't have aDefault. However, if we change aResult from pointer to reference, doesn't that make API users confused?

This particular user was confused by the current API... the pointer version is the standard out param variant for ints and bools throughout the codebase, bug XPCOM string out params are references...

(In reply to comment #21) > static nsresult GetCString(const char* aPref, nsACString* aResult); > static nsresult GetString(const char* aPref, nsAString* aResult); Hmm, Boris, are you saying that these should be references instead of pointers?

The second argument for both of those, yes.

That makes sense.