346599 - customizeCharset.js uses deprecated nsIPref

Status: RESOLVED FIXED

(Toolkit :: Preferences, defect)

Description

[Joel Nackman]

customizeCharset.js uses nsIPref on line 15 instead of nsIPrefService/nsIPrefBranch.  nsIPref is deprecated.

Comment 1

[Joel Nackman]

Attachment: Patch

I believe that this patch will fix the problem, but I was not sure about one of the changes that I made.  On line 27, getLocalizedUnicharPref is called.  I replaced this with getComplexValue from nsIPrefBranch, because I found that this is what is called anyway here: http://lxr.mozilla.org/seamonkey/source/toolkit/obsolete/content/nsUserSettings.js#90

Is this the correct approach to handling the Unichar value, and is there any documentation that somebody could point me to that would help me understand the differences?

Comment 2

[Nickolay_Ponomarev]

Thanks for the patch, but you also need to request a review from a toolkit reviewer in order to get it in. The approach you took looks fine.

about 'localized prefs' see:
http://kb.mozillazine.org/Dev_:_Using_preferences#nsIPrefLocalizedString

Comment 3

[[e-mail]]

Comment on attachment 231352 [details] [diff] [review]
Patch

I don't think you should use setCharPref. I patched the xpfe version of this file in bug 230778 but if you clone my code you can't then ask me for review ;-)

Comment 4

[Joel Nackman]

Attachment: Patch w/o setCharPref

Comment 5

[Nickolay_Ponomarev]

I don't think that works, did you actually test it? You want to create an nsIPrefLocalizedString object, set its value, and pass it to setComplexPref, like here: http://kb.mozillazine.org/Dev_:_Using_preferences#nsIPrefLocalizedString

Or just do what Neil did for the suite.

Comment 6

[Joel Nackman]

Attachment: Fixed patch

Whoops.  I tested the wrong thing, which is why it appeared to work to me.  I think I've straigtened it out now (hopefully).

Comment 7

[Neil Deakin]

Comment on attachment 242573 [details] [diff] [review]
Fixed patch

I'm not actually the same Neil. I could review this, but it seems more logical to have him do it if he wrote the original xpfe code.

Comment 8

[Neil Deakin]

Comment on attachment 242573 [details] [diff] [review]
Fixed patch

OK, I'll review it anyway ;) I misread "can't" above as "can".

Comment 9

[Nickolay_Ponomarev]

To get a patch checked in you normally need to put "[checkin needed]" into status whiteboard and/or ask someone to check it in for you if you want to wait less than a few weeks :)

Comment 10

[:Gavin Sharp [email: [e-mail]]]

mozilla/toolkit/content/customizeCharset.js 	1.3