*****Observed with linux 05/07 trunk build***** If I enter non-ASCII chars for directory name and base DN, it's not displayed correctly after I save and come back to the edit window. Steps to reproduce: 1. Launch Mail with a profile file which already setup a mail account. 2. Select Edit | Mail/News Account Settings. 3. Select Server Settings under an account. 4. Check on "Override my global LDAP directory server..." 5. Click on Edit Directories 6. Click on Add. 7. Enter non-ASCII characters for Name and base DN like o=<non-ascii chars>, and click on OK button to save it. You'll see the direcotry name is displayed grabled on the list and if you click on Edit to open it, both directory name and base DN are garbled.

Not sure if it's save problem. Changed the summary.

Xianglan, please attach a screen shot. Reassign to putterman.

reassigning to srilatha

Xianglan, for the test case of the screen shot, what characters did you actually input?

Since I'm on linux, it's EUC.

Same problem on Windows, which uses Shift-JIS.

I think this is also reproducible with Latin1. Looks like raw UTF-8 is diaplayed.

Latin-1 seems okay.

This bug depends on the wstring landing in 71247.

Actually, I think this two separate bugs. One, the base DN not being displayed (or used!) correctly is really just an instance of bug 71427. The other, the directory name, is something else. I'm narrowing this bug to just be about the directory name, removing the dependency on 71247, making block 17880, the master autocomplete tracking bug.

Is directory name saved into the same file as base DN?

They're both saved into prefs.js, I believe.

There are three visual problems here: 1. The directory name is garbled on server settings window. 2. The directory name is garbled on LDAP directory server list window. 3. Both directory name and base DN are garbled on Edit window. Do you think I need to file a seperate bug for base DN display problem on #3?

I think annotating 71247 with a reminder to be sure and verify that the base DN problem is fixed when that bug is being verified should be sufficient.

PDT+ per 6/12 mtg.

- CopyUnichar and SetUniChar are depricated. They are replaced by Get/SetComplexValue. In fact, the whole nsIPref interface is depricated and is replaced by nsIPrefService and nsIPrefBranch. I think it's better if you replace it nsIPref with nsIPrefService. CCing bnesse for his view. - You may want to change CopyCharPref to CopyUniCharPref for 'dn' attribute at line:116 in pref-directory.js ldapUrl.dn = gPrefInt.CopyCharPref(pref_string + ".searchBase") same at line: 128 , setting spec in migrate()

migrate() will be removed from pref-directory.js and moved to nsLDAPPrefsService.js (bug # 79252). That is the reason why I did change these calls in here.

I think it's ok to use nsIPref at this point. It will be converted to nsIPrefService during the whole tree cleanup. Since migrate() is moving from this file, it should be fine, but still I think it's better to convert it to *UniChar* with a comment, in case someone cuts and pastes it to the new file.

This is fine if you fix in nsLDAPPrefsService.js, but your final patch in bug 79252 (attachment 38951 [details] [diff] [review]) is still using nsIPref also.

with the comments in migrate() code, r=mitesh

modified the patch based on comments from mitesh. ccing Seth for an sr

two comments: 1) + ldapUrl = ldapUrl = ldapUrl.createInstance(). + QueryInterface(Components.interfaces.nsILDAPURL); should be + ldapUrl = ldapUrl.createInstance(). + QueryInterface(Components.interfaces.nsILDAPURL); 2) + try { + var ldapService = Components.classes[... should be var ldapService; try { ldapService = ... since you refer to ldapService outside the scope of the try {} block. you did this in two places. I didn't check to see if moving it out of the try block would make ldapService defined twice. since it is a services, perhaps having and using gLDAPService would be better. (like you have gPrefInt)

one last comment: + ldapUrl.dn = gPrefInt.CopyUnicharPref(pref_string + ".searchBase"); dn is a string (not wstring) attribute on nsILDAPURL.idl in other parts of the patch you convert dn by using UTF8toUCS2. (and when going back, UCS2toUTF8) did the nsILDAPURL.idl interface change?

+ ldapUrl.dn = gPrefInt.CopyUnicharPref(pref_string + ".searchBase"); I do not need to convert from UCS2 to UTF8. This should be something like pref_string_content = gPrefInt.CopyUnicharPref(pref_string + ".searchBase"); ldapUrl.dn = ldapService.UCS2toUTF8(pref_string_content); I did not pay much attention to this code since this part of migrate() and it is going to go away. (see bug # 79252) I will post a new patch will the changes right away

Regarding, ------------------ + try { + var ldapService = Components.classes[... should be var ldapService; try { ldapService = ... since you refer to ldapService outside the scope of the try {} block. --------------------- I think in Javascript there is no block level scope. So if a variable is defined anywhere in a function with 'var ...', it's available as a local variable throughout the function. There are few other places in both the files where try { var ldapUrl = ... } ldapUrl = ... syntax has been used.

I saw the other comment Re: changing |DN| in nsILDAPURL to a |wstring|. I need to talk to dmose about this, I left it |string| intentionally, since other parts of the URL methods (e.g. |SetSpec| and |GetSpec|) can't be changed to |wstring|. My patch for this has a doxygen comment mentioning that all |string| in the URLs are supposed to be UTF8 encoded. Again, let me talk to dmose, and we can decide if |DN| and |Filter| should be |wstring| in nsILDAPURL.idl. Thanks, -- Leif

are you running with js warnings enabled? user_pref("javascript.options.strict", true); (in your prefs.js file?) it looks like you'll get a js warning in pref-directory.js because ldapService is not declared I'm not sure about the UTF8toUCS2 and UCS2toUTF8 usage in your patch. dn is a string (a c string) so setting it to a UTF8 string seems like you are going to lose the high bits. dmose / leif, can you verify that the usage in the last is correct?

I believe Srilatha is doing the right thing, |string| (C-strings) can hold UTF8 encoded strings. -- Leif

wait, are you sure about that? I thought that bytes in a UTF8 string can have the high bit set, but when you cross the XPConnect boundry for ASCII string, we'll loose the high bit. dveditz / nhotta, can you comment?

As leif mentioned, |string| can hold UTF-8 string like char* can hold Latin1 characters. Usually problem happens when AssignWithConversion or NS_ConvertASCIItoUCS2 are used for UTF-8 strings. So it's okay as long as proper UTF-8 to UCS2 conversion is applied (e.g NS_ConvertUTF8toUCS2).

dbradley just verified by reading the code that XPConnect will not munch the high bit on |string| values, so I think we're good.

sr=sspitzer on the last patch, but there appears to be some strict js warnings. feel free to find them and fix them with a new patch now, or land this patch now and log a bug and fix the strict js warnings after 0.9.2.

a= asa@mozilla.org for checkin to the trunk. (on behalf of drivers)

Fix checked in

Verified with 06/20 windows and mac trunk build. It's fixed. Will verify with linux build when a testable build is available.

Verified with linux 06-22-11 trunk build. It's fixed.