70933 - [FEATURE] basic nsLDAPAutoCompleteSession implementation

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

Code is started and I've got it populating a menu on the first search.  Need to
make it use params and populate menus on subsequent searches.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: nsILDAPAutoCompleteSession & code that implements it

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: update the autocomplete test xul and make it build

Comment 3

[Blake Ross]

r=blake on the test

Will get to the first patch tomorrow, it's long.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

ducarroz was kind enough to come over and review both patches, and he had the
following comments:

* add trailing newlines to files that are missing them

* when implementing the local search, make sure that any previous results are 
  finished and didn't abort or hit the size limit before using them in the next 
  local search.  since this code isn't yet written, add a comment to this
  effect.  may require extending nsIAutoCompleteResults.

* StartLDAPSearch() doesn't create a new mResults if one already
  exists.  but that one could conceivably be stale.  why was this written like
  that?  

He said that once these comments are addressed, r=ducarroz.

I also noticed that I forgot to add a makefile.win for the test subdirectory;
I'll attach a patch that deals all these things shortly.

Comment 5

[Leif Hedstrom]

Well, I don't have a lot of comments, the good looks good to me. I have some
very minor things I'd like to point out, but this is r=leif.


* As mentioned before, I still believe we need to be careful when we
autocomplete - if a user types just one or two characters ( "j" / "jo"), that
could quite likely generate LDAP searches that are unindex on many LDAP servers.
iPlanet's LDAP servers would most certainly treat such searches (e.g. "cn=j*")
as an unindexed search, since it only index substrings that are 3 characters or
longer.

*I think we should be consistent when using NS_ERROR_NULL_POINTER vs 
NS_ERROR_INVALID_POINTER, even though they are the same.

* In OnLDAPMessage(), it's not clear to me what the "default" case actually
catches. Is it something we can always safely ignore (like we do now), or does
it need to be documented?

* As pointed out by the comments in the code, if there are multiple "mail"
attribute values, we just ignore everything but the first value. This might not
be a reasonable assumption at all times. Similar assumptions are done with CNs,
which are also allowed to be multi value. As an example when this is an issue,
people getting married are typically allowed to have multiple CNs, like "Dan
Fell" and "Dan Mosedale". Someone searching on "dmos" or something like that 
might not understand that "Dan Fell" really is "Dan Mosedale", if we only show
the first. This becomes even more convoluted when someone has multiple e-mail
addresses and CNs... *uck*

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: nsLDAPAutoComplete session patch V2

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: autocomplete test xul & build changes, V2

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

OK, new versions of the patches are attached.

ducarroz:

* the trailing newline complained about in the patch was actually in
  the CVS repository; the patch has the corrected version.

* added a comment about constraints when reusing previous search
  results

* the |if (!mResults)| bracketing the creation of mResults was bogus.
  Changed it to be created in all cases.

leif:

* switched all uses of NS_ERROR_INVALID_POINTER to NS_ERROR_NULL_POINTER

* in OnLDAPMessage, changed the NS_WARNING() to an NS_ERROR and
  improved the comment.

* filed bugs on minimum string length (76593) and multi-valued
  attribute handling (76595).

misc other changes:

* added xpfe/components/autocomplete/makefile.win

* added |#ifdef PR_LOGGING| code to initialize gLDAPLogModule in
  OnStartLookup if it hasn't already been initialized

* added test in InitConnection to ensure that mServerURL has been
  properly initialized

* updated autocomplete_test.xul to work with hewitt's latest patch from
  (bug 43189).  Note that the tip has diverged enough that that patch
  will no longer apply, so we'll actually need a newer patch than
  that.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: nsLDAPAutoCompleteSession patch, v3

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: autocomplete test xul & build changes, v3

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Minor changes:

* fixed the test XUL to work with hewitt's most recent patch from 43189

* removed a couple instances of the break-after-return pattern (hi shaver :-)

* tweaked a few comments

* by the time OnStopLookup is executed on the main thread, there may already be
OnLDAPMessage calls on the main thread's event queue.  Be sure that these
OnLDAPMessage calls check for this state and quietly abort instead of trying to
deref a null pointer.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: nsLDAPAutoCompleteSession patch v4

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

OK, I think I'm done with revisions, modulo any super-review comments.  I
realized that my race fix from the last iteration still left a smaller race
open.  Opened a separate bug on that (the fix requires other code to land), and
added XXX comments.  Also changed the current hardcoded search filter to be more
like what people will expect.  v3 of the autocomplete test patch should still
work fine.

Comment 14

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

+     * A template for formatting the autocompletion results. 

Is a template better than a formatter object with a callback?  I guess I don't
know how you're going to specify the format, but I'm concerned about i18n issues
WRT positional parameters, and the like.  Assuage me, Dan.

+     * @exception NS_ERROR_NULL_POINTER  NULL pointer passed to getter

I'll cave on doing the NULL checks, but I draw the line at documenting the
belt-and-suspenders defense against API abuse.  Who's going to both pass a NULL
pointer to a getter, _and_ read the documentation?

+     * If non-zero, limit the search to at most this many entries.  If this
+     * limit is hit, simply return |nsIAutoCompleteStatus::ignored|.

Return from what?  That comment applies to an attribute, so it's not clear.  (In
fact, this interface has no non-inherited methods, so it's really unclear.)


+// XXXdmose check and see how much space NS_ERROR_MODULE_LDAP is actually 
+// allocated and be sure we're not overflowing that.
+//

Looks from nsError.h like you get 16 bits for your error codes.  Should be fine.

+REQUIRES
= xpcom string necko appcomps
+REQUIRES
+= mimetype

What is appcomps?  I'm starting to wonder if this code doesn't belong under
xpfe, rather than under directory/xpcom; it seems like the LDAP-XPCOM stuff is
useful without Mozilla as a whole, until you hit these dependencies.  What say you?

+    switch (mState) {
+    case UNBOUND:
 ...
+        break;
+    default:
+        // we should never get here
+        NS_ERROR("nsLDAPAutoCompleteSession::OnStartLookup(): unexpected "
+                 "value of mStatus");
+        return NS_ERROR_UNEXPECTED;
+    }
+
+    return NS_OK;

How about moving the |return NS_OK| up into the UNBOUND case, which is the only
one without an unconditional return?  I think it makes the control flow easier
to analyze.

+    switch (mState) {
+    case UNBOUND:

Could you add an empty |default:| case to this?  It makes it clearer that you
haven't forgotten the other cases -- unless there are no other cases, in which
case you should remove the |default:| in the previous switch, for consistency. 
(Hobgoblin of little minds, especially mine!)

+ * @arg aMessage  The message that was returned, NULL if none was.

+    // figure out what sort of message was returned
+    //
+    nsresult rv = aMessage->GetType(&messageType);

So this is the _one_ time you don't check for a NULL pointer, and it's actually
part of the API!  Anyway, you'll crash right there if someone hands you one.

+    case nsILDAPMessage::RES_BIND:
+
+        // a bind has completed
+        //
+        return OnLDAPBind(aMessage);
+        break;

Ahem.  Also, consider bringing the NS_OK into the |default:| case, because
that's the only time you'll hit it.  Though, really, if it's an error, maybe you
should return NS_ERROR_UNEXPECTED?

I found the FinishSearch calls in OnLDAPBind to be a bit confusing, since it
didn't seem like we'd _started_ any searches on entry (and the comment after the
two error-handling blocks seems to agree).  Should it maybe be called
ResetSearchState, or something?  As it stands, the fallthrough from the 

+    nsresult rv = aMessage->GetErrorCode(&errCode);
+    if (NS_FAILED(rv)) {

case to the one that checks the likely-uninitialized errCode against ::SUCCESS
seems wrong to me.  Should you bail after resetting search state, I mean,
finishing your search? =)  (And in the non-SUCCESS case, as well.)

If the nsILDAPErrors constants are nsresults, using NS_FAILED and NS_SUCCEEDED
would be better that explicit comparisons against ::SUCCESS.

+        // don't call FinishSearch(), as this could conceivably be an
+        // anomaly, and perhaps the next message will be ok.  if this
+        // really was a problem, this search should eventually get
+        // reaped by a timeout (once that code gets implemented).

Does this mean that we currently leak searches, in the admittedly unlikely case
that we can't create an autocomplete item?  Just making sure I understand the
code; it's not necessarily a problem.

+    default:
+        NS_ERROR("nsLDAPAutoCompleteSession::OnLDAPSearchEntry(): "
+                 "unexpected return code from aMessage->getValues()");
+        return NS_ERROR_UNEXPECTED;
+    }

Do you really want to give that on an unexpected success code?  What about
testing NS_FAILED(rv) before switch()ing, and removing the NS_OK case?

+    rv = item->SetValue(NS_ConvertUTF8toUCS2(values[0]).GetUnicode());
+    if (NS_FAILED(rv)) {
+        NS_ERROR("nsLDAPAutoCompleteSession::OnLDAPSearchEntry(): "
+                 "item->SetValue failed");
+        NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(numVals, values);
+        return NS_ERROR_FAILURE;
+    }
+
+    // we're done with the values; free them
+    //
+    NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(numVals, values);

You can reduce the duplication here by NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAYing
values after the call to SetValue, and before the test for failure, since you're
going to want to free them either way.  (I always prefer to put common,
unconditional code before conditionally executed code, because I find it easier
to analyze later.)

+    rv = mResults->SetSearchString(mSearchString.GetUnicode());

It looks, to the untrained eye, like those interfaces could use some AString
loving in their IDL.  What do you think?

+    mOperation = do_CreateInstance(
+        "@mozilla.org/network/ldap-operation;1", &rv);

Nit: split the line after the |=| to keep the function with its arguments?

+    rv = NS_GetProxyForObject(NS_UI_THREAD_EVENTQ, 
+                              NS_GET_IID(nsILDAPMessageListener), 

Righteous helper!

How do you decide whether to return NS_ERROR_UNEXPECTED or NS_ERROR_FAILURE from
StartLDAPSearch?  If you pick one, then you can put the common and oft-repeated
error logic of

+        mState = BOUND;
+        FinishSearch(nsIAutoCompleteStatus::failed);
+        return NS_ERROR_FAILURE;

in an |error:|-labelled block at the end of the method, and goto it from your
many error cases.  (If you're into that sort of thing.)

+    rv = mConnection->Init(host, port, 0);
+    switch (rv) {
+    case NS_OK:
+        break;

More explicit comparisons against NS_OK.  I really prefer NS_SUCCEEDED here.

Also, why do you want to convert NS_ERROR_ILLEGAL_VALUE to NS_ERROR_UNEXPECTED?
 The former seems more informative, especially since the illegal value is likely
to be one ultimately supplied by the caller.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

> +     * A template for formatting the autocompletion results. 
>
> Is a template better than a formatter object with a callback? 

Yes, because it needs to be string that can be specified as a preference.

> I guess I don't know how you're going to specify the format, but I'm 
> concerned about  i18n issues WRT positional parameters, and the like.  
> Assuage me, Dan.

That's not fully decided yet, but if it really turns out that a
template is the wrong thing, it's easy to change the interface after
this lands, given that noone is using it yet because the template code
isn't implemented.  One possibility is to have a template specified as
a regexp, which should have all the necessary flexibility.

> +     * @exception NS_ERROR_NULL_POINTER  NULL pointer passed to getter
>
> I'll cave on doing the NULL checks, but I draw the line at documenting the
> belt-and-suspenders defense against API abuse.  Who's going to both
> pass a NULL pointer to a getter, _and_ read the documentation?

The idea is that all possible exceptions in the interface are
documented.  This means that you can test for all the documented
returns yourself in a switch statement with the default: case being
something that should never happen and indicates a bug in the code
that you're calling.

> +     * If non-zero, limit the search to at most this many entries.  If this
> +     * limit is hit, simply return |nsIAutoCompleteStatus::ignored|.
>
> Return from what?  That comment applies to an attribute, so it's not
> clear.  (In fact, this interface has no non-inherited methods, so
> it's really unclear.)

Reading the idl for an interface that inherits from something other
than nsISupports without having the idl for the parent interface(s)
nearby to refer to generally not very useful.  Anyway, comment clarified:

    /**
     * If non-zero, limit lookups to at most this many entries.  If this
     * limit is hit during a lookup, the search will simply return 
     * |nsIAutoCompleteStatus::ignored|.
     *

> // XXXdmose check and see how much space NS_ERROR_MODULE_LDAP is actually 
> // allocated and be sure we're not overflowing that.
> //
> 
> Looks from nsError.h like you get 16 bits for your error codes.
> Should be fine.

Excellent; thanks.  XXX removed.

> +REQUIRES
> = xpcom string necko appcomps
> +REQUIRES
> += mimetype
>
> What is appcomps?  I'm starting to wonder if this code doesn't belong under
> xpfe, rather than under directory/xpcom; it seems like the
> LDAP-XPCOM stuff is useful without Mozilla as a whole, until you hit
> these dependencies.  What say you?

appcomps == xpfe/components

You're probably right, especially since it (and therefore any
dependency on LDAP it instantiates) can be turned off with build time
switches.  I guess I'd propose integrating this stuff into the
xpfe/components/autocomplete infrastruct.  ducarroz, hewitt, any
thoughts on this?

> +    switch (mState) {
> +    case UNBOUND:
>  ...
> +        break;
> +    default:
> +        // we should never get here
> +        NS_ERROR("nsLDAPAutoCompleteSession::OnStartLookup(): unexpected "
> +                 "value of mStatus");
> +        return NS_ERROR_UNEXPECTED;
> +    }
> +
> +    return NS_OK;
> 
> How about moving the |return NS_OK| up into the UNBOUND case, which
> is the only one without an unconditional return?  I think it makes
> the control flow easier to analyze.

Done.  And I even deleted the break.  :-)

> +    switch (mState) {
> +    case UNBOUND:
>
> Could you add an empty |default:| case to this?  It makes it clearer that you
> haven't forgotten the other cases -- unless there are no other
> cases, in which case you should remove the |default:| in the
> previous switch, for consistency. (Hobgoblin of little minds,
> especially mine!)

There are no other cases; previous default: removed. 

> + * @arg aMessage  The message that was returned, NULL if none was.
> 
> +    // figure out what sort of message was returned
> +    //
> +    nsresult rv = aMessage->GetType(&messageType);
>
> So this is the _one_ time you don't check for a NULL pointer, and
> it's actually part of the API!  Anyway, you'll crash right there if 
> someone hands you one.

Heh.  Whoops.  OK, just before calling GetType, I've added this code:

    // just in case.
    // XXXdmose the semantics of NULL are currently undefined, but are likely
    // to be defined once we insert timeout handling code into the XPCOM SDK
    // At that time we should figure out if this still the right thing to do.
    // 
    if (!aMessage) {
        return NS_OK;
    }

> +    case nsILDAPMessage::RES_BIND:
> +
> +        // a bind has completed
> +        //
> +        return OnLDAPBind(aMessage);
> +        break;
>
> Ahem.  Also, consider bringing the NS_OK into the |default:| case, because
> that's the only time you'll hit it.  Though, really, if it's an
> error, maybe you should return NS_ERROR_UNEXPECTED?

Threw out the break.  Moved the NS_OK into the |default:| case.  The
return code is sort of immaterial because this code is called through
an ASYNC_PROXY, and those throw out all return codes.

Note that I tend to use NS_ERROR_UNEXPECTED to mean "it should
literally be impossible to get here unless there is a bug here or in
the underlying code".  This code could conceivably be reached even if
there were nothing wrong on the client side at all, but the LDAP server
did something strange.

> I found the FinishSearch calls in OnLDAPBind to be a bit confusing, since it
> didn't seem like we'd _started_ any searches on entry (and the
> comment after the two error-handling blocks seems to agree).  Should
> it maybe be called ResetSearchState, or something? 

We haven't started an LDAP search, but we have started an autocomplete
search.  They are referred to as "lookups" in the interface, so I've
changed it to FinishAutoCompleteLookup().

> As it stands, the fallthrough from the 
>
> +    nsresult rv = aMessage->GetErrorCode(&errCode);
> +    if (NS_FAILED(rv)) {
>
> case to the one that checks the likely-uninitialized errCode against
> ::SUCCESS seems wrong to me.  Should you bail after resetting search
> state, I mean, finishing your search? =)  (And in the non-SUCCESS
> case, as well.)

Good catch; I made both instances return NS_ERROR_FAILURE.

> If the nsILDAPErrors constants are nsresults, using NS_FAILED and 
> NS_SUCCEEDED would be better that explicit comparisons against ::SUCCESS.

They are not nsresults.

> +        // don't call FinishSearch(), as this could conceivably be an
> +        // anomaly, and perhaps the next message will be ok.  if this
> +        // really was a problem, this search should eventually get
> +        // reaped by a timeout (once that code gets implemented).
>
> Does this mean that we currently leak searches, in the admittedly unlikely 
> case that we can't create an autocomplete item?  Just making sure I 
> understand the code; it's not necessarily a problem.

Yes; until timeouts are implemented (they're high on the to-do list;
bug 78232), some error conditions may cause leakage.

> +    default:
> +        NS_ERROR("nsLDAPAutoCompleteSession::OnLDAPSearchEntry(): "
> +                 "unexpected return code from aMessage->getValues()");
> +        return NS_ERROR_UNEXPECTED;
> +    }
>
> Do you really want to give that on an unexpected success code?  What about
> testing NS_FAILED(rv) before switch()ing, and removing the NS_OK case?

The switch statement tests against exactly the set of returns
documented in the interface (which is why I always document the
NULL_POINTER and OUT_OF_MEMORY exceptions).  However, since some lamer
could potentialy add a new success code to the component in the future
and forget to add doxygen comment to the interface, and since we can't
use compiler-based exceptions to catch that kinda of lossage, I suppose I
should do the NS_FAILED thing.  Sigh.  Fixed.

> +    rv = item->SetValue(NS_ConvertUTF8toUCS2(values[0]).GetUnicode());
> +    if (NS_FAILED(rv)) {
> +        NS_ERROR("nsLDAPAutoCompleteSession::OnLDAPSearchEntry(): "
> +                 "item->SetValue failed");
> +        NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(numVals, values);
> +        return NS_ERROR_FAILURE;
> +    }
> +
> +    // we're done with the values; free them
> +    //
> +    NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(numVals, values);
>
> You can reduce the duplication here by 
> NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAYing values after the call to SetValue, 
> and before the test for failure, since you're going to want to free them 
> either way.  (I always prefer to put common, unconditional code before 
> conditionally executed code, because I find it easier to analyze later.)
>

Ah, good point.  Done.

> +    rv = mResults->SetSearchString(mSearchString.GetUnicode());
>
> It looks, to the untrained eye, like those interfaces could use some AString
> loving in their IDL.  What do you think?

I wasn't aware that IDL AStrings were already available for general
use.  Undoubtedly they would be a fine change here.  However, I'm
under a lot of pressure to land this code, and making that change
would also require a whack to global history, and would require extra
coordination with hewitt's autocomplete changes.  As such, I'd prefer
to not do that here and now.

>    mOperation = do_CreateInstance(
>         "@mozilla.org/network/ldap-operation;1", &rv);
>
> Nit: split the line after the |=| to keep the function with its arguments?

Done.

> +    rv = NS_GetProxyForObject(NS_UI_THREAD_EVENTQ, 
> +                              NS_GET_IID(nsILDAPMessageListener), 
> 
> Righteous helper!

Word.

> How do you decide whether to return NS_ERROR_UNEXPECTED or 
> NS_ERROR_FAILURE from StartLDAPSearch?  

I use NS_ERROR_FAILURE to mean: "something or other failed, possibly
in a call that made to some method which doesn't document the possible
failure codes that it might return".  I use NS_ERROR_UNEXPECTED to
mean "if we got here, we've almost certainly hit a bug".

> If you pick one, then you can put 
> the common and oft-repeated error logic of
>
> +        mState = BOUND;
> +        FinishSearch(nsIAutoCompleteStatus::failed);
> +        return NS_ERROR_FAILURE;
>
>
> in an |error:|-labelled block at the end of the method, and goto it from 
> your many error cases.  (If you're into that sort of thing.)

There is something to that.  But |goto|s make my skin crawl, and I would
hope that most modern optimizers would coalesce a lot of these similar
cases under the hood for me anyway.

> +    rv = mConnection->Init(host, port, 0);
> +    switch (rv) {
> +    case NS_OK:
> +        break;
> 
> More explicit comparisons against NS_OK.  I really prefer NS_SUCCEEDED here.

As mentioned above, I only do this for methods where all the return
codes are documented and NS_OK is the only possible success code.
However, since we don't have compiler-based exceptions, I'll switch.

> Also, why do you want to convert NS_ERROR_ILLEGAL_VALUE to 
> NS_ERROR_UNEXPECTED?  The former seems more informative, especially
> since the illegal value is likely to be one ultimately supplied by
> the caller.

You're right; fixed.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: nsLDAPAutoCompleteSession patch, v5

Comment 17

[Jean-Francois Ducarroz]

After speaking with dmose about the location where should leave the autocomplete LDAP implementation, we have 
decided that xpfe/components/autocomplete would be the best place.

Comment 18

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Fair enough.  I'm not sure that adding a success code is an incompatible
interface change, but I haven't made my mind up about that.

WRT the goto-to-common-error-block, I'm less concerned about the compiler's
ability to optimize -- even gcc gets that right, usually -- than I am about ease
of human analysis.  I had to read each case to make sure that they were
identical, and not just similar, and it was lots of state to track in my little
mind.  You're not writing the code for the compiler, you're writing it for
people like me!

I think the Java notion of checked vs unchecked exceptions are useful here, for
documentation and API specification purposes.  Some (NS_ERROR_NOT_INITIALIZED,
NS_ERROR_LDAP_FUZZY_PUPPY) are part of the API, and indicate something about the
operation of the called method to the caller, and are analogous to the Java
checked exceptions.  Others (NS_ERROR_NULL_POINTER, NS_ERROR_FAILURE,
NS_ERROR_OUT_OF_MEMORY) are ways to signal failure in the underlying XPCOM
runtime, or similar out-of-API-constraints cases.  Documenting the former cases
is valuable, but I think that documenting the XPCOM runtime failures is just noisy.

Could you file a bug on the AString IDL stuff?  The new strings are peppier and
prettier, and it'd be good to put it on the radar in case someone like jag or
timeless wants to jihad for you.

xpfe/autocomplete seems like a much better place for it, yes.  Thanks.

sr=shaver.

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: LDAP XPCOM SDK changes to support autocompletion

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: LDAP XPCOM SDK changes to support autocompletion, v6

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: xpfe/components changes for LDAP autocompletion

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

Argh, that last patch was supposed to say v6 also.  Anyway, the last two
attachments (32799, 32800) have all the nsLDAPAutoCompleteSession stuff moved
over to xpfe/components.  Otherwise, pretty much everything should be the same.
 If we're very lucky, these will be the final changes.

Comment 23

[cls]

Wrap those xul/js dump()s in a if (DEBUG) (see bug 76720) && r=cls on attach
32799 & 32800
 

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: LDAP XPCOM SDK changes to support autocomplete, v7

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: xpfe/components changes for LDAP autocompletion, v7

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

OK, the last patches wouldn't have worked, because I forgot to supply -N to the
diff command.  Talked to cls, and he has withdrawn his DEBUG request for the JS,
since JS doesn't have a DEBUG switch that's tied to the build system, and
because this is only in test chrome (ie --enable-tests) that isn't actually
linked to from anywhere.  So assuming things look good on Windows and Mac, this
will be what I checkin.

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: xpfe/components changes for LDAP autocomplete, v8

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

This last xpfe/components patch has two tiny changes: 

* installs in the autocomplete/test subdir if DISABLE_TESTS is unset.  Thanks to
srilatha for the patch.

* changed a ? : usage that confused codewarrior into a full if {} else {}

Another patch with mac buildsystem related changes will be added shortly.

Comment 29

[Dan Mosedale (:dmosedale, :dmose)]

The patches currently in this bug are checked in and part of the default build
on Windows and Unix (though not currently actually used).  I'm still working on
making the Mac build scripts do the right thing.

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch to build on mac, part 1

Comment 31

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch to build on mac, part 2

Comment 32

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: mac build patch 2, version 2 (with trailing linefeed)

Comment 33

[Dan Mosedale (:dmosedale, :dmose)]

Attachments 33136 and 33138, along with ldapAutoCompleteIDL.mcp and the addition 
of nsLDAPAutoComplete.cpp to appcomps.cpp, make this build with and without the 
"ldap" option set on Mac.

Comment 34

[Scott Collins]

sr=scc

Comment 35

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: mac build patch 2, version 3

Comment 36

[Dan Mosedale (:dmosedale, :dmose)]

Whoops, part 2 of the patch was screwed up and only would have built on Mac, not 
on any other platforms.  I've attached a better version, so now 33136 and 33219 
are the ones to be checked in.

Comment 37

[Simon Fraser [no longer active]]

Mac patches look OK, r=sfraser. Of course you'll be checking in some projects 
too, don't forget.

Comment 38

[Peter Van der Beken [:peterv]]

r=peterv for attachment 33136 [details] [diff] [review] and attachment 33219 [details] [diff] [review].

Comment 39

[Dan Mosedale (:dmosedale, :dmose)]

Mac changes checked in.

Comment 40

[olga charapaev]

verified on Mac OS 9.1, WinNT 4.0, Linux Red Hat 7.0
with May 30 builds