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.
