If I use a LDAP server from a migrated profile, it won't works. Apparently the prefs for the server URI has changed since 4.x

I think Profile Migration is a better component for this.

reassigning to srilatha

Here is was I have in my prefs: user_pref("ldap_2.servers.InfoSpaceDirectory.csid", "UTF-8"); user_pref("ldap_2.servers.InfoSpaceDirectory.description", "InfoSpace Directory"); user_pref("ldap_2.servers.InfoSpaceDirectory.filename", "ldap.infospace.com4"); user_pref("ldap_2.servers.InfoSpaceDirectory.position", 5); user_pref("ldap_2.servers.InfoSpaceDirectory.replication.lastChangeNumber", 0); user_pref("ldap_2.servers.InfoSpaceDirectory.searchBase", "c=US"); user_pref("ldap_2.servers.InfoSpaceDirectory.serverName", "ldap.infospace.com"); and here is was I think we are suppose to have: user_pref("ldap_2.servers.infospace.csid", "UTF-8"); user_pref("ldap_2.servers.infospace.filename", "ldap.infospace.com9"); user_pref("ldap_2.servers.infospace.replication.lastChangeNumber", 0); user_pref("ldap_2.servers.infospace.searchBase", "o=netscape communications corp.,c=us"); user_pref("ldap_2.servers.infospace.uri", "ldap://ldap.infospace.com:389/o=netscape communications corp.,c=us");

only the first directory server is getting migrated and the rest are not. In your prefs after migration this is what you were supposed to have. user_pref("ldap_2.servers.InfoSpaceDirectory.csid", "UTF-8"); user_pref("ldap_2.servers.InfoSpaceDirectory.description", "InfoSpace Directory"); user_pref("ldap_2.servers.InfoSpaceDirectory.filename", "ldap.infospace.com4"); user_pref("ldap_2.servers.infospace.replication.lastChangeNumber", 0); user_pref("ldap_2.servers.infospace.uri", "ldap://ldap.infospace.com:389/o=netscape communications corp.,c=us"); I have patch. will post it right away.

ducarroz, can you try the patch and see if this will fix the problem. You need to set the preference ldap_2.prefs_migrated to false.

Cc in bhuvan for a review

ccing Seth for a sr

Priority P2, TFV 0.9.1.

note, we doing some sort of ldap pref migration already. see nsPrefMigration.cpp but, that only happens at profile migration. and if someone migrated from 4.x to mozilla 0.9 or netscape 6.0, they won't go through that code again when they run a newer build. question: your code only gets run when ldap pref UI is brought up. what happens I had certain prefs in 4.x, migrate to 6.x, but don't bring up the pref UI first? won't things fail since my prefs aren't migrated? or are those prefs only involved with the UI?

You are right about code gets run when ldap pref UI is brought up. Since, by default no server is selected for autocompletion, you need to go to preferences to select a server. So, the prefs get migrated before you use it.

> by default no server is selected for autocompletio what if I used an ldap server for autocomplete in 4.x, and then I migrated over to 6.x? does that case work as expected?

Actually, the preference has changed from 4.x In 4.x we used to have user_pref("ldap_2.servers.<server-name>.autocomplete.enabled", true); but in 6.x it is user_pref("ldap_2.autocomplete.directoryServer", <server-name>); So the first time when you use a migrated profile, this preference does not exist. you will need to select a server again.

if that's the case, then you haven't fixed the problem. LDAP prefs are still not correctly migrated. instead of doing the migration in the .js for the UI, you should figure out the proper place to migrate the prefs so that if I used LDAP autocomplete in 4.x, it works in 6.x. nsPrefMigration.cpp is not the correct place for that, since if the user has already migrated (to mozilla 0.9 or netscape 6.0) they won't be going through that code again. if we do it right, a user will get and install mozilla 1.0 / 6.x, and LDAP autocomplete will just start working as it did in 4.x, assuming they migrated their 4.x profile.

Migrating the LDAP prefs the first time compose window, And in migrate() checking for the autocompletion.enabled pref for each server and initialize ldap_2.autocomplete.directoryServer should fix this.

that makes sense. instead of duplicating the code, make sure to put the code in the proper service (any ideas where?) and then call it from both places.

Srilatha, Your changes look good. Couple of things from my end : * Don't you still need to incorporate your new LDAP js file in (xpinstall) packages files to have it exported properly into the builds * looks like LDAPPrefsService is defined but not used in MsgComposeCommands.js file.. r=bhuvan It will be better to get Candice's review on LDAP specific code segments as I am no expert there.

You are right, I need add the js file to packages. Eventhough we are not using LDAPPrefsService in MsgComposeCommands.js, when we call the constructor we are migrating the 4.x LDAP prefs and initializing the nsLDAPService. This is essential when the user opens the mail compose window before going to preferences. I am building on mac and I have some mac specific stuff to add to the patch. Will post an updated patch soon.

Setting target milestone to 0.9.2 (check it in anytime, even before, when the tree is open for). Per PDT triage.

general comments ----------------- Any LDAP dependencies need to be conditionalized, so that building without MOZ_LDAP_XPCOM defined generates a still-working, LDAP-free build. This means that most Makefile.in and makefile.in changes will need to be tweaked, as well as some Mac magic. On Unix, be sure to conditionally tweak the REQUIRES lines as well, so that MOZ_TRACK_MODULE_DEPS builds continue to work. See xpfe/components/{build,autocomplete/*} for examples. It may also be necessary to have JS detect whether or not this is an LDAP build before deciding to execute LDAP-dependant code. When adding new files, at least, can you wrap your code at 80 columns for readability? Unfortunately, dump() sends stuff to the console not just in debug builds, but in release builds as well. So if you could take out the calls to dump(), that would be good. There seem to be a fair number of explicit |catch| clauses which don't do anything but silently ignore the error, and attempt to continue executing with obviously horked data (eg null interfaces) rather than returning. In many of these cases, I presume the right thing to do is to either not |catch| at all and let the exception unwind up the stack, or else give the user some sort of other feedback that there was a problem. Can you go through the catches and fix these things in the appropriate ways? nsILDAPPrefsService.idl: ------------------------ license nits: you've got an extra < before your name; the copyright year is wrong. misc nits: I think "6.x format" by the migrate decl() should really be "mozilla format"; the "serves" at the interface comment should be "servers" There appear to be spacing problems in and around the comments; I bet there are some tabs there. nsISupportsArrays do not map directly to JS arrays. If you want to use them from JS, you need createInstance the nsSupportsArray contractid then call the nsISupportsArray interface methods to manipulate it. So if anyone tries to use the getter and setter for availDirectories, it'll do something evil and won't work. The arrays that do map to JS arrays directly are raw XPCOM arrays; see nsILDAPMessage.idl and nsLDAPDatasource.js for examples of that in use. Note that XPIDL doesn't currently supports arrays being attributes, so getters and setters need to be declared directly. All that said, as far as I can tell, none of the attributes listed in nsILDAPPrefsService.js are actually used outside of that file -- they seem to be just private internal properties of that component. So could they actually be removed from the interface altogether -- or do you anticipate needing to access them from outside in some case? It looks to me like the deleteServer method is just there to provide symmetry with the addServer method. Rather than hanging these two methods off nsILDAPPrefsService service, how about just adding a createServer helper method onto nsILDAPService itself, and then calling into the nsILDAPService directly for all three things? .migrate(): It'd be nice if there were a comment somewhere that explained what the significance of the 2 in ldap_2 is ... >+ ldapUrl = Components.classes["@mozilla.org/network/ldap-url;1"]; >+ ldapUrl = ldapUrl.getService(nsILDAPURL); LDAP URLs are not singletons, so instead of getService, use createInstance. It looks like at most one directory server will be enabled due to the use of the enable variable. I assume that's intentional, but it's not obvious from reading the code. If it is intentional, can you please a comment indicating that? In general, more comments are likely to make for faster reviewer turnaround, I suspect.

PDT+ per 6/12 mtg.

Few things in this patch that are different from the previous one nsLDPAPrefsService does not have the nsLDAPService stuff since we decided to drop ldapservice. added ifdef MOZ_LDAP_XPCOM to the makefiles. removed dumps. probably added a couple ( when prefs service or ldap url fails). Fixed the "Catch" caluses Fixed license brb

Because you're calling migrate() from the constructor, when getService() is called from MsgComposeCommands.js, it may have the side effect of doing the migration. This seems a bit unintuitive to me; if you could add a comment to MsgComposeCommands.js explicitly mentioning that just getting the service may have this possible side-effect, you've got r=dmose@netscape.com. No need to post another patch here. I don't think it matters a lot in this particular case, so it's not important to me that you change this here, but this is worth keeping in mind going forward: One issue with the XPCOM model is that it's impossible to communicate any sort of failure back from an object's constructor, because the failure can only get communicated back to the factory, not to whoever actually asked for the component to be instantiated. The general workaround for this is that in XPCOM components, one never does anything that could fail inside the constructor, but instead moves such initialization to a separate init() method which any clients call manually.

made change to Makefile.in so that it builds on linux Modified pref calls so that we save Multibyte character values right. I have built this on mac too. Will post a patch for that soon

I notice that this version no longer modifies pref-addressing.xul. I assume this is intentional? My only concern is this: ++ if (dn && ldapService) ++ ldapUrl.dn = ldapService.UCS2toUTF8(dn); This means that if the LDAP Service is unavailable, it will migrate everything _except_ the DN, but mark the preferences as migrated. This seems like confusing behavior to me. Aborting the entire migration if ldapService is unavailable seems more reasonable to me, since it doesn't leave the user with half-baked prefs. r=dmose@netscape.com if you address these concerns; no new patch necessary if those are the only changes.

pref-addressing.xul: Yes it is intentional. I had this change in my tree (removes javascript error). It is a fix for bug 79780. Will make the change such that we return if ldapservice is not available if (dn && ldapService) ldapUrl.dn = ldapService.UCS2toUTF8(dn); else return;

Please do not use the nsIPref interface. It has been depricated in favor of nsIPrefService and nsIPrefBranch.

After talking to Brian, he gave me an o.k on using nsIPref since these methods in nsIPrefBranch and nsIPrefService are not usable from js.

looks good. I'd add a call to flush out the prefs file to disk after you are done migrating. something like: + gPrefInt.SetBoolPref("ldap_2.prefs_migrated", true); + gPrefInt.savePrefFile(null); that's been the common practice: migrate, save prefs. do that, and sr=sspitzer

a=dbaron for trunk checkin (on behalf of drivers) assuming above comments have been addressed

Fix checked in

This fix as caused regression bug 86996!

oops, wrong bug number! This is causing regression bug 86966

Verified on Windows, Linux and Mac platforms.