Last Comment Bug 659820 - Replace nsContentUtils::GetCharPref() and nsContentUtils::GetStringPref() with Preferences::GetCString() and Preferences::GetString()
: Replace nsContentUtils::GetCharPref() and nsContentUtils::GetStringPref() wit...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
Mentors:
Depends on: 656826 660401
Blocks: 660121
  Show dependency treegraph
 
Reported: 2011-05-25 16:47 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2011-06-25 03:11 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part.1 Change the string params of Preferences class from ns*String to nsA*String (4.56 KB, patch)
2011-05-25 16:47 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
roc: review+
Details | Diff | Review
Part.2 Replace (23.26 KB, patch)
2011-05-25 16:48 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review
Part.2 Re-implement Preferences::Get(Char|LocalizedString)() (5.58 KB, patch)
2011-05-26 05:47 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review
Part.3 Replace (20.54 KB, patch)
2011-05-26 05:48 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review
Part.2 Re-implement Preferences::Get(Char|LocalizedString)() (18.68 KB, patch)
2011-05-26 05:55 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review
Part.2 Reimplement Preferences::(G|S)et(Char|LoclizedString)() to Preferences::(G|S)et(Localized)CString() (22.54 KB, patch)
2011-05-26 19:13 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
roc: review+
Details | Diff | Review
Part.1 Change the string params of Preferences class from ns*String to nsA*String (mq) (3.28 KB, patch)
2011-05-26 19:40 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review
Part.2 Refactor char/string/localizedstring pref methods of Preferences (mq) (15.91 KB, patch)
2011-05-26 19:43 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review
Part.2 Add Preferences::GetLocalizedString() for nsACString (mq) (1.34 KB, patch)
2011-05-26 19:46 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review
Part.3 Refactor char/string/localizedstring pref methods of Preferences (mq) (15.91 KB, patch)
2011-05-26 19:47 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review
Part.4 Do replace (13.38 KB, patch)
2011-05-26 20:11 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
roc: review+
Details | Diff | Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-25 16:47:30 PDT
Created attachment 535230 [details] [diff] [review]
Part.1 Change the string params of Preferences class from ns*String to nsA*String
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-25 16:48:44 PDT
Created attachment 535231 [details] [diff] [review]
Part.2 Replace
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-25 17:26:16 PDT
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]:
-----------------------------------------------------------------
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-25 17:30:29 PDT
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?
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-26 05:47:07 PDT
Created attachment 535307 [details] [diff] [review]
Part.2 Re-implement Preferences::Get(Char|LocalizedString)()

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.
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-26 05:48:33 PDT
Created attachment 535308 [details] [diff] [review]
Part.3 Replace
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-26 05:55:48 PDT
Created attachment 535310 [details] [diff] [review]
Part.2 Re-implement Preferences::Get(Char|LocalizedString)()

Sorry, I posted old patch.
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-26 17:02:31 PDT
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 8 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-26 19:13:58 PDT
Created attachment 535538 [details] [diff] [review]
Part.2 Reimplement Preferences::(G|S)et(Char|LoclizedString)() to Preferences::(G|S)et(Localized)CString()
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-26 19:18:26 PDT
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]:
-----------------------------------------------------------------
Comment 10 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-26 19:40:37 PDT
Created attachment 535539 [details] [diff] [review]
Part.1 Change the string params of Preferences class from ns*String to nsA*String (mq)
Comment 11 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-26 19:43:22 PDT
Created attachment 535540 [details] [diff] [review]
Part.2 Refactor char/string/localizedstring pref methods of Preferences (mq)
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-26 19:46:30 PDT
Created attachment 535541 [details] [diff] [review]
Part.2 Add Preferences::GetLocalizedString() for nsACString (mq)

Moved from bug 659821 for clearing order of the patches.
Comment 13 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-26 19:47:08 PDT
Created attachment 535542 [details] [diff] [review]
Part.3 Refactor char/string/localizedstring pref methods of Preferences (mq)
Comment 14 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-26 20:11:58 PDT
Created attachment 535548 [details] [diff] [review]
Part.4 Do replace
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-26 20:16:55 PDT
Comment on attachment 535548 [details] [diff] [review]
Part.4 Do replace

Review of attachment 535548 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-06-24 11:55:07 PDT
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?
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-24 16:15:12 PDT
This approach makes for shorter caller code.
Comment 19 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-06-24 16:47:57 PDT
Why?  Passing |str| and passing |&str| should be the same speed....
Comment 20 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-06-24 16:48:17 PDT
I meant the same length...
Comment 21 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-24 18:26:39 PDT
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?
Comment 22 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-06-24 18:45:40 PDT
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...
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-24 19:45:07 PDT
(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?
Comment 24 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-06-24 19:46:01 PDT
The second argument for both of those, yes.
Comment 25 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-25 03:11:28 PDT
That makes sense.

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