Closed Bug 281298 Opened 20 years ago Closed 19 years ago

Widget should use the newer nsIPrefService APIs instead of nsIPref


(Core :: XUL, defect)

Not set





(Reporter: vhaarr+bmo, Assigned: vhaarr+bmo)





(2 files, 3 obsolete files)

Rid the source of nsIPref!
Attached patch version 0.1 (obsolete) — Splinter Review
First attempt to get it all.
Attachment #173571 - Flags: review?(bryner)
Attached patch version 0.2 (obsolete) — Splinter Review
Some suggestions by biesi; use NS_ARRAY_LENGTH and just return void in the
Attachment #173571 - Attachment is obsolete: true
Attachment #173574 - Flags: review?(bryner)
Attachment #173571 - Flags: review?(bryner)
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+
Comment on attachment 173574 [details] [diff] [review]
version 0.2

bz, can you sr XP Toolkit/Widgets ?
Attachment #173574 - Flags: superreview?(bzbarsky)
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);


>     char *statusstyle_type;
>-    rv = prefs->CopyCharPref(PREF_XIM_STATUS, &statusstyle_type);
>+    rv = prefs->GetCharPref(PREF_XIM_STATUS, &statusstyle_type);


>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-
Attached patch version 0.3 (obsolete) — Splinter Review
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: <>
Attachment #173574 - Attachment is obsolete: true
Attachment #195053 - Flags: superreview?(bzbarsky)
Attachment #195053 - Flags: review+
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+
So the line would be
nsCOMPtr<nsIPrefBranchInternal> prefBranchInternal(do_QueryInterface(prefs));

Would anyone want to check this in and doing this change manually, or do you
want an updated patch? biesi? ;-)

Thanks for the reviews!
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+
checked in on trunk, with s/nsIPrefBranchInternal/nsIPrefBranch2/. (see the
comments in

Closed: 19 years ago
Resolution: --- → FIXED
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
Attached patch bustage fix?Splinter Review
Attachment #195728 - Flags: review?(cbiesinger)
Yes ... Thanks :-)
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.