399136 - 396745 broke nsLDAPChannel.cpp

Status: RESOLVED FIXED

(Directory :: LDAP XPCOM SDK, defect)

Description

[Howard Chu]

The patch for bug #396745 missed an invocation of nsILDAPConnection:Init...
A fix is attached here.

Comment 1

[Howard Chu]

Attachment: Fix for nsLDAPChannel.cpp

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 284105 [details] [diff] [review]
Fix for nsLDAPChannel.cpp

Guess I missed this as its in the experimental code.

With this change, you need to make some additional changes:

We no longer need to get options out of the mLDAPURL so that code/variable can be dropped.

 534     if (port == -1)
 535         port = LDAP_PORT;

Here you need to make an additional change to set LDAP_PORT in the mURI for the -1 case. Otherwise this won't be picked up by mLDAPURL.

Additionally the

rv = NS_CheckPortSafety(port, "ldap");

call can just be changed to 

rv = NS_CheckPortSafety(mURI);

Although its not really required for this patch, I think it will make the code a bit cleaner here.

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #2)
> (From update of attachment 284105 [details] [diff] [review])
>  534     if (port == -1)
>  535         port = LDAP_PORT;
> Here you need to make an additional change to set LDAP_PORT in the mURI for the
> -1 case. Otherwise this won't be picked up by mLDAPURL.

Actually, nsLDAPConnection already does the port == -1 check:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/directory/xpcom/base/src/nsLDAPConnection.cpp&rev=1.65&mark=940-941#940

So drop that from nsLDAPChannel.cpp, and change the NS_CheckPortSafety to the mURI option like I suggested before (which also handles the -1 case), and then we don't need to get the port or the options in nsLDAPChannel.cpp.

Comment 4

[Howard Chu]

Attachment: cleaned up patch

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 284105 [details] [diff] [review] [details])
> >  534     if (port == -1)
> >  535         port = LDAP_PORT;
> > Here you need to make an additional change to set LDAP_PORT in the mURI for the
> > -1 case. Otherwise this won't be picked up by mLDAPURL.
> 
> Actually, nsLDAPConnection already does the port == -1 check:

Yeah, just noticed that. OK, cleaned up. Also moved the empty host check up, no point in doing the other setup if the URL is invalid.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 284124 [details] [diff] [review]
cleaned up patch

That's better, r=me. I'm requesting sr for you, not sure if its really necessary as its NPOTB code, but we may as well do it.

Comment 6

[Mark Banner (:standard8)]

Howard are you able to check this in, or do you want me to?

Comment 7

[Howard Chu]

(In reply to comment #6)
> Howard are you able to check this in, or do you want me to?
> 
Please go ahead, I don't have commit access. Thanks.

Comment 8

[Mark Banner (:standard8)]

I just checked this in. Marking as fixed. Thanks for the patch Howard.