Replace nsContentUtils::GetCharPref() and nsContentUtils::GetStringPref() with Preferences::GetCString() and Preferences::GetString()

RESOLVED FIXED in mozilla7

Status

()

Core
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

Trunk
mozilla7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 7 obsolete attachments)

3.28 KB, patch
Details | Diff | Splinter Review
1.34 KB, patch
Details | Diff | Splinter Review
15.91 KB, patch
Details | Diff | Splinter Review
13.38 KB, patch
roc
: review+
Details | Diff | Splinter Review
Created attachment 535230 [details] [diff] [review]
Part.1 Change the string params of Preferences class from ns*String to nsA*String
Attachment #535230 - Flags: review?
Created attachment 535231 [details] [diff] [review]
Part.2 Replace
Attachment #535231 - Flags: review?(roc)
Attachment #535230 - Flags: review? → review?(roc)
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]:
-----------------------------------------------------------------
Attachment #535230 - Flags: review?(roc) → 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?
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.
Attachment #535231 - Attachment is obsolete: true
Attachment #535231 - Flags: review?(roc)
Attachment #535307 - Flags: review?(roc)
Created attachment 535308 [details] [diff] [review]
Part.3 Replace
Attachment #535308 - Flags: review?(roc)
Created attachment 535310 [details] [diff] [review]
Part.2 Re-implement Preferences::Get(Char|LocalizedString)()

Sorry, I posted old patch.
Attachment #535307 - Attachment is obsolete: true
Attachment #535307 - Flags: review?(roc)
Attachment #535310 - Flags: review?(roc)
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.
Blocks: 660121
Created attachment 535538 [details] [diff] [review]
Part.2 Reimplement Preferences::(G|S)et(Char|LoclizedString)() to Preferences::(G|S)et(Localized)CString()
Attachment #535310 - Attachment is obsolete: true
Attachment #535310 - Flags: review?(roc)
Attachment #535538 - Flags: review?(roc)
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]:
-----------------------------------------------------------------
Attachment #535538 - Flags: review?(roc) → review+
Created attachment 535539 [details] [diff] [review]
Part.1 Change the string params of Preferences class from ns*String to nsA*String (mq)
Attachment #535230 - Attachment is obsolete: true
Created attachment 535540 [details] [diff] [review]
Part.2 Refactor char/string/localizedstring pref methods of Preferences (mq)
Attachment #535538 - Attachment is obsolete: true
Created attachment 535541 [details] [diff] [review]
Part.2 Add Preferences::GetLocalizedString() for nsACString (mq)

Moved from bug 659821 for clearing order of the patches.
Attachment #535308 - Attachment is obsolete: true
Attachment #535540 - Attachment is obsolete: true
Attachment #535308 - Flags: review?(roc)
Created attachment 535542 [details] [diff] [review]
Part.3 Refactor char/string/localizedstring pref methods of Preferences (mq)
Created attachment 535548 [details] [diff] [review]
Part.4 Do replace
Attachment #535548 - Flags: review?(roc)
Comment on attachment 535548 [details] [diff] [review]
Part.4 Do replace

Review of attachment 535548 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #535548 - Flags: review?(roc) → 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
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Summary: Replace nsContentUtils::GetCharPref() and nsContentUtils::GetStringPref() with Preferences::GetChar() → Replace nsContentUtils::GetCharPref() and nsContentUtils::GetStringPref() with Preferences::GetCString() and Preferences::GetString()
Depends on: 660401
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.
You need to log in before you can comment on or make changes to this bug.