Widget should use the newer nsIPrefService APIs instead of nsIPref

RESOLVED FIXED

Status

()

RESOLVED FIXED
14 years ago
12 years ago

People

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

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

14 years ago
Rid the source of nsIPref!
(Assignee)

Comment 1

14 years ago
Created attachment 173571 [details] [diff] [review]
version 0.1

First attempt to get it all.
Attachment #173571 - Flags: review?(bryner)
(Assignee)

Comment 2

14 years ago
Created attachment 173574 [details] [diff] [review]
version 0.2

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

14 years ago
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+
(Assignee)

Comment 4

14 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 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

13 years ago
Created attachment 195053 [details] [diff] [review]
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>
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+
(Assignee)

Comment 8

13 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

13 years ago
Created attachment 195724 [details] [diff] [review]
version 0.4 (for checkin)

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
http://lxr.mozilla.org/seamonkey/source/modules/libpref/public/nsIPrefBranchInternal.idl)

Status: NEW → RESOLVED
Last Resolved: 13 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
(Assignee)

Comment 12

13 years ago
Created attachment 195728 [details] [diff] [review]
bustage fix?
Attachment #195728 - Flags: review?(cbiesinger)
(Assignee)

Comment 13

13 years ago
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.