Request for Review 6882594

[Christopher Hegarty - Sun Microsystems Ireland]

On 17/09/2009 21:18, Alan Bateman wrote:
> Christopher Hegarty -Sun Microsystems Ireland wrote:
>> Max (Weijun) Wang wrote:
>>> HttpURLConnection.java:
>>>
>>>    I think "if (authScheme.equals(BASIC))" can be written as 
>>> "authScheme == BASIC", and possibly you can use switch/case in 
>>> several places
>>
>> Thanks Max, these changes certainly make the code more readable.
>>
>> Updated webrev can be found at:
>>   http://cr.openjdk.java.net/~chegar/6882594/webrev.1/webrev/
> This looks much better.
> 
> In HttpURLConnection there are a few places where there are have 
> expressions like
>  !(proxyAuthenitcation.getAuthScheme() == NTLM)
> I assume this would be neater as:
>  (proxAuthenitcation.getAuthScheme() != NTLM)
Good catch! Done.

> Are tryTransparentNTLMServer/Proxy needed? I'm not too familiar with the 
> NTLM code but it looks like they are always initialized to 
> NTLMAuthentication.supportsTransparentAuth which makes me wonder why the 
> code can't just be:
>   if (NTLMAuthentication.supportsTransparentAuth()) { ... }

What we're trying to do here is to check if the platform supports 
transparent authentication and then try it once only (if transparent 
fails it will prompt the user for credentials). I reuse the 
tryTransparentNTLMServer/Proxy as it can have 3 states, null (first time 
through), true, false.

If this looks a little messy/confusing I can swap it out use a pair of 
booleans to represent this.

> For the logging is it necessary to check the logger level? (just 
> wondering if HttpCapture can be called directly).
No I don't believe it is necessary to check the level, but I see this 
kind of thing all the time (just recently when looking at Mandys 
PlatformLogger webrevs). I don't see why we ever check the log level 
since Logger.fine will do that anyway.

> In NTLMAuthenticationProxy should you be using the 3-arg Class.forName? 
You mean Class.forName(clazzStr, true, null), right? To force the class 
to be loaded with the boot classloader. Ditto for Negotiate. Done.

> Also, if the constructors aren't present then I would think it is a 
> fatal error.

Yes, good point. Now I log the CNFE and throw an AssertionError for 
every thing else. Ditto for Negotiate. Done.

Updated webrev can be found at:
   http://cr.openjdk.java.net/~chegar/6882594/webrev.1/webrev/


-Chris.


> 
> Otherwise looks okay to me.
> 
> -Alan.
> 
> 
> 
> 
>