Not really an LDAP C SDK bug; a new bugzilla component should be appearing shortly. I'll move this once it's ready. I recent sent shaver the following mail: So here's our theory: right now, all LDAP code is turned on with --enable-ldap. We'd like to land minimal ldap support in the default build as soon as we reasonably can that we'll then base autocomplete work off of. Specfically, the what we'd like to do is turn on the code in directory/xpcom/base/src/nsLDAP{Connection, ConnectionCallbacks, Internal, Message, Operation, ProtocolHandler, ProtocolModule, URL}* (along with the corresponding interfaces in "public") in the default build, and still have all the code enabled in --enable-ldap builds. ---- In fact, ProtocolHandler should stay turned off unless --enable-ldap is set, since it's really only a stub to get an nsLDAPChannel, and that is definitely not ready for prime time. But we'd like to get everything else turned on.

I like the loss of interfaces where the component independence is a fiction. Bully for you. ( > > nsILDAPMessageListener.idl: > > > > - no comments > > ?? It does have comments. In the patch, I've fixed changed one of them > from C++ style to doxygen style, if that makes any difference. Actually, I meant ``I have no comments on this file''. =) ) I dispute your Good Form(TM) defense of break-after-return, even more so than of pervasive null-checking: if someone's going to change the code to remove a return, the last thing we're worried about is switch-fallthrough errors, no? And when I was reading that code, I spent a little while on each case trying to figure out if we'd ever hit the break ("is the indentation just wrong?"). If they're going to stay, please comment them as /* never hit -- defensive only */ or something to save poor reviewers unacquainted with that brand of future-proofing. Kill the fprintfs! Spill their blood! |const| doesn't (and had better not!) imply |static| to a C compiler! Your adaptive XXX-as-bug strategy is righteous. Go with my blessing. I'm pretty happy with this code now; great work, and thanks for enduring my picky review with such grace. Lemme scan the new diff and check that license issue with Mitchell, and I'll have an sr= to you today, or you'll know why!

+ * XXXdmose - should use wstring here + * + * @return unicode encoded string representation. + */ + wstring toUnicode(); Lose the outdated comment. - nsCOMPtr<nsILDAPConnection> mConnection; // LDAP connection for this channel + nsCOMPtr<nsILDAPConnection> mConnection; // LDAP connection ... - PRInt32 lderrno = ldap_get_lderrno(mConnectionHandle, NULL, NULL); + PRInt32 lderrno = ldap_get_lderrno(mConnectionHandle, 0, 0); Mismatched indent? + if ( mConnectionHandle == NULL ) ... + if ((mDesc->lud_filter == NULL) || You were making them all implicit, but seem to have missed these. Also, use nsnull or 0, but never NULL. (It busts on some platforms, where it's |(void *)0|.) + (nsCRT::strlen(mDesc->lud_filter) == 0)) Nit: |!mDesc->lud_filter[0]| is a faster test. + flex="2" id="testTree" ref="ldap://nsdirectory.netscape.com:389/dc=netscape,dc=com??sub?(sn=Smith)"> Can we point to a public server for the test? It'll make it a lot easier for us non-Netscape folks. Other than these bits, it looks _great_. I wish we had more code like it. sr=shaver if you correct those; don't bother reposting a patch.

OK, the latest round of comments is all sorted out. I only put the patch back in the bug because that's the easiest way to for me to make it available to download and test on all three boxes.

Patches checked in. Reassigning to leif, as he has a patch in progress that will actually turn on the appropriate bits in the default build.

Please check out the changes I made to the build system and some source to enable LDAP by default. You can disable LDAP features by using the --disable-ldap (or env DISABLE_LDAP on Windows).

One change was required for the LDAP_EXPERIMENTAL stuff work on windows: resetting $(LCFLAGS) to include the newly modified $(DEFINES). I built and tested this stuff on both linux and windows (this stuff is not turned on on the mac yet. Seems to be working nicely. So, with the above change, r=dmose@netscape.com. On to super-review...

peachy. sr=alecf

The Unix/Windows version of the build patch was checked in before the 0.8.1 tree closure. Up next: mac version, thanks to help from peterv.

Together with changes to the mac project file, the above patch splits ldap out into ldap and ldap_experimental options, with ldap on in the default build, and ldap_experimental off.

Forgot to mention that the heavy lifting on that patch was done by peterv. Thanks, Peter!

sr=sfraser on the mac build patches

Patch to turn on the basic LDAP infrastructure in the mac build are now checked in.

OK, just got r=cls on this last windows patch, and he said I should also get sr=leaf. leaf?

sr=leaf for patch 28374

Patch to turn on in windows build by default checked in.

r=leif

sr=cls

Patch to turn LDAP on for autoconf platforms checked in.

reassign to Olga as QA contact

verified on Win/Unix/Mac