Request for Review 6882594

[Alan Bateman]

Christopher Hegarty - Sun Microsystems Ireland wrote:
> 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/
Looks reasonable to me. I see you've reverted 
tryTransparentNTLMServer/Proxy to two booleans which makes it a bit 
easier to understanding One small suggestion is to add a description to 
NTLMAuthenticationProxy's static methods to make it easier for future 
maintainers.

-Alan.