Closed
Bug 281298
Opened 20 years ago
Closed 19 years ago
Widget should use the newer nsIPrefService APIs instead of nsIPref
Categories
(Core :: XUL, defect)
Core
XUL
Tracking
()
RESOLVED
FIXED
People
(Reporter: vhaarr+bmo, Assigned: vhaarr+bmo)
References
()
Details
Attachments
(2 files, 3 obsolete files)
28.51 KB,
patch
|
vhaarr+bmo
:
review+
vhaarr+bmo
:
superreview+
|
Details | Diff | Splinter Review |
773 bytes,
patch
|
Details | Diff | Splinter Review |
Rid the source of nsIPref!
Assignee | ||
Comment 1•20 years ago
|
||
First attempt to get it all.
Attachment #173571 -
Flags: review?(bryner)
Assignee | ||
Comment 2•20 years ago
|
||
Some suggestions by biesi; use NS_ARRAY_LENGTH and just return void in the methods.
Attachment #173571 -
Attachment is obsolete: true
Attachment #173574 -
Flags: review?(bryner)
Assignee | ||
Updated•20 years ago
|
Attachment #173571 -
Flags: review?(bryner)
Comment 3•19 years ago
|
||
Comment on attachment 173574 [details] [diff] [review] version 0.2 I'm not really crazy about having to loop through all of the prefs in Observe, but this code should be hit rarely. r=me.
Attachment #173574 -
Flags: review?(bryner) → review+
Assignee | ||
Comment 4•19 years ago
|
||
Comment on attachment 173574 [details] [diff] [review] version 0.2 bz, can you sr XP Toolkit/Widgets ?
Attachment #173574 -
Flags: superreview?(bzbarsky)
Comment 5•19 years ago
|
||
Comment on attachment 173574 [details] [diff] [review] version 0.2 >Index: src/gtk/nsGtkIMEHelper.cpp > char *input_style; >- rv = prefs->CopyCharPref(PREF_XIM_INPUTSTYLE, &input_style); >+ rv = prefs->GetCharPref(PREF_XIM_INPUTSTYLE, &input_style); How about using nsXPIDLCString while you're here? > char *preeditstyle_type; >- rv = prefs->CopyCharPref(PREF_XIM_PREEDIT, &preeditstyle_type); >+ rv = prefs->GetCharPref(PREF_XIM_PREEDIT, &preeditstyle_type); Same. > char *statusstyle_type; >- rv = prefs->CopyCharPref(PREF_XIM_STATUS, &statusstyle_type); >+ rv = prefs->GetCharPref(PREF_XIM_STATUS, &statusstyle_type); Same. >Index: src/xpwidgets/nsXPLookAndFeel.cpp >+nsXPLookAndFeel::ColorPrefChanged (unsigned int index, const char *prefName) >- rv = prefService->CopyCharPref(newpref, getter_Copies(colorStr)); >- if (NS_SUCCEEDED(rv) && colorStr[0]) { >+ prefService->GetCharPref(prefName, getter_Copies(colorStr)); >+ if (colorStr[0]) { This change is no good -- if the pref is not set, this should crash..... (and it should _definitely_ assert). Why not use !colorStr.IsEmpty() ? >+nsXPLookAndFeel::InitColorFromPref(PRInt32 i, nsIPrefBranch* aPrefBranch) > { > char *colorStr = 0; nsXPIDLCString, please! The rest looks ok.
Attachment #173574 -
Flags: superreview?(bzbarsky) → superreview-
Assignee | ||
Comment 6•19 years ago
|
||
Uses nsXPIDLCString and IsEmpty. bz: Care to take a second look? When I use getter_Copies, do the strings free themselves when they go out of scope? I read[1] that they do, so I removed the nsCRT:free / PL_strfree calls. 1: <http://www.mozilla.org/projects/xpcom/string-guide.html#Callee_Allocated>
Attachment #173574 -
Attachment is obsolete: true
Attachment #195053 -
Flags: superreview?(bzbarsky)
Attachment #195053 -
Flags: review+
Comment 7•19 years ago
|
||
Comment on attachment 195053 [details] [diff] [review] version 0.3 > nsXPLookAndFeel::Init() >+ nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID)); >+ if (!prefs) >+ return; >+ nsCOMPtr<nsIPrefBranchInternal> prefBranchInternal(do_GetService(NS_PREFSERVICE_CONTRACTID)); Why not just QI |prefs| to nsIPrefBranchInternal? sr=bzbarsky with that change. And yes, an nsXPIDLC?String that got written into via getter_Copies will handle deallocating itself.
Attachment #195053 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Comment 8•19 years ago
|
||
So the line would be nsCOMPtr<nsIPrefBranchInternal> prefBranchInternal(do_QueryInterface(prefs)); right? Would anyone want to check this in and doing this change manually, or do you want an updated patch? biesi? ;-) Thanks for the reviews!
Assignee | ||
Comment 9•19 years ago
|
||
biesi or bz: Could you be so kind as to check this in?
Attachment #195053 -
Attachment is obsolete: true
Attachment #195724 -
Flags: superreview+
Attachment #195724 -
Flags: review+
Comment 10•19 years ago
|
||
checked in on trunk, with s/nsIPrefBranchInternal/nsIPrefBranch2/. (see the comments in http://lxr.mozilla.org/seamonkey/source/modules/libpref/public/nsIPrefBranchInternal.idl)
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 11•19 years ago
|
||
fwiw, I had to check this in to fix mac bustage: +++ widget/src/mac/nsToolkitBase.h 12 Sep 2005 13:00:37 -0000 -class nsToolkitBase : public nsIToolkit : public nsIObserver +class nsToolkitBase : public nsIToolkit, public nsIObserver
Assignee | ||
Comment 12•19 years ago
|
||
Attachment #195728 -
Flags: review?(cbiesinger)
Assignee | ||
Comment 13•19 years ago
|
||
Yes ... Thanks :-)
Updated•19 years ago
|
Attachment #195728 -
Flags: review?(cbiesinger)
Comment 14•18 years ago
|
||
this seems to have missed a few occurrences, see bug 357616
You need to log in
before you can comment on or make changes to this bug.
Description
•