281298 - Widget should use the newer nsIPrefService APIs instead of nsIPref

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Vidar Haarr (not reading bugmail)]

Rid the source of nsIPref!

Comment 1

[Vidar Haarr (not reading bugmail)]

Attachment: version 0.1

First attempt to get it all.

Comment 2

[Vidar Haarr (not reading bugmail)]

Attachment: version 0.2

Some suggestions by biesi; use NS_ARRAY_LENGTH and just return void in the
methods.

Comment 3

[Brian Ryner (not reading)]

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.

Comment 4

[Vidar Haarr (not reading bugmail)]

Comment on attachment 173574 [details] [diff] [review]
version 0.2

bz, can you sr XP Toolkit/Widgets ?

Comment 5

[Boris Zbarsky [: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);

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.

Comment 6

[Vidar Haarr (not reading bugmail)]

Attachment: version 0.3

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>

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[Vidar Haarr (not reading bugmail)]

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!

Comment 9

[Vidar Haarr (not reading bugmail)]

Attachment: version 0.4 (for checkin)

biesi or bz: Could you be so kind as to check this in?

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in on trunk, with s/nsIPrefBranchInternal/nsIPrefBranch2/. (see the
comments in
http://lxr.mozilla.org/seamonkey/source/modules/libpref/public/nsIPrefBranchInternal.idl)

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 12

[Vidar Haarr (not reading bugmail)]

Attachment: bustage fix?

Comment 13

[Vidar Haarr (not reading bugmail)]

Yes ... Thanks :-)

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

this seems to have missed a few occurrences, see bug 357616