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.
