Request for Review: 6893238

[Christopher Hegarty -Sun Microsystems Ireland]

CR 6893238: Move NTLM and SPNEGO implementations into separate packages

Webrev:
   http://cr.openjdk.java.net/~chegar/6893238/webrev.0/webrev/

Following 688259, NTLM and SPNEGO authentication implementations are now
runtime dependencies. There is no reason that their implementations
should reside in the same package as the protocol handler.

Moving them out of sun.net.www.protocol.http and into say,
sun.net.www.protocol.http.ntlm and sun.net.www.protocol.http.spnego will
simplify the modularization effort by making it easier to define module
definitions (by package name as opposed to a regular/glob expression).
These schemes will not be in the base package.

-Chris.

[michael.mcmahon]

Christopher Hegarty - Sun Microsystems Ireland wrote:

> CR 6893238: Move NTLM and SPNEGO implementations into separate packages
>
> Webrev:
>   http://cr.openjdk.java.net/~chegar/6893238/webrev.0/webrev/
>
> Following 688259, NTLM and SPNEGO authentication implementations are
> now runtime dependencies. There is no reason that their
> implementations should reside in the same package as the protocol
> handler.
>
> Moving them out of sun.net.www.protocol.http and into say,
> sun.net.www.protocol.http.ntlm and sun.net.www.protocol.http.spnego
> will simplify the modularization effort by making it easier to define
> module definitions (by package name as opposed to a regular/glob
> expression). These schemes will not be in the base package.
>
> -Chris.

Looks fine.

- Michael.

[Alan Bateman]

Christopher Hegarty - Sun Microsystems Ireland wrote:

> CR 6893238: Move NTLM and SPNEGO implementations into separate packages
>
> Webrev:
>   http://cr.openjdk.java.net/~chegar/6893238/webrev.0/webrev/
>
> Following 688259, NTLM and SPNEGO authentication implementations are
> now runtime dependencies. There is no reason that their
> implementations should reside in the same package as the protocol
> handler.
>
> Moving them out of sun.net.www.protocol.http and into say,
> sun.net.www.protocol.http.ntlm and sun.net.www.protocol.http.spnego
> will simplify the modularization effort by making it easier to define
> module definitions (by package name as opposed to a regular/glob
> expression). These schemes will not be in the base package.
>
> -Chris.

This mostly looks good to me. One area that could be a bit cleaner is
where Negotiator.getSupported throws CNF (say where kerberos is not
present). It looks like this is caught in
NegotiateAuthentication.isSupported and maybe it would be nicer if
getSupported were renamed to getNegotiator and have it return null if
kerberos is not present. Just a suggestion (I realize some of this
pre-dates your changes).

-Alan

[Christopher Hegarty -Sun Microsystems Ireland]

On 21/10/2009 15:05, Alan Bateman wrote:

> Christopher Hegarty - Sun Microsystems Ireland wrote:
>> CR 6893238: Move NTLM and SPNEGO implementations into separate packages
>>
>> Webrev:
>>   http://cr.openjdk.java.net/~chegar/6893238/webrev.0/webrev/
>>
>> Following 688259, NTLM and SPNEGO authentication implementations are
>> now runtime dependencies. There is no reason that their
>> implementations should reside in the same package as the protocol
>> handler.
>>
>> Moving them out of sun.net.www.protocol.http and into say,
>> sun.net.www.protocol.http.ntlm and sun.net.www.protocol.http.spnego
>> will simplify the modularization effort by making it easier to define
>> module definitions (by package name as opposed to a regular/glob
>> expression). These schemes will not be in the base package.
>>
>> -Chris.
> This mostly looks good to me. One area that could be a bit cleaner is
> where Negotiator.getSupported throws CNF (say where kerberos is not
> present). It looks like this is caught in
> NegotiateAuthentication.isSupported and maybe it would be nicer if
> getSupported were renamed to getNegotiator and have it return null if

Yes, this would certainly be cleaner. I've updated the webrev. Please
take a look.

   http://cr.openjdk.java.net/~chegar/6893238/webrev.1/webrev/

Note: The lack of the initial cause of the IOException in firstToken
should not be a problem since the Exception is swallowed in setHeaders (
a few lines above).

-Chris.

> kerberos is not present. Just a suggestion (I realize some of this
> pre-dates your changes).
>
> -Alan

[Alan Bateman]

Christopher Hegarty - Sun Microsystems Ireland wrote:
> :
> Yes, this would certainly be cleaner. I've updated the webrev. Please
> take a look.
>
>   http://cr.openjdk.java.net/~chegar/6893238/webrev.1/webrev/
>
> Note: The lack of the initial cause of the IOException in firstToken
> should not be a problem since the Exception is swallowed in setHeaders
> ( a few lines above).
This looks much better - thanks for doing this. Minor bit is that the
L183 can simply be "throw new IOException(...)".

-Alan.

[Christopher Hegarty -Sun Microsystems Ireland]

On 21/10/2009 15:47, Alan Bateman wrote:

> Christopher Hegarty - Sun Microsystems Ireland wrote:
>> :
>> Yes, this would certainly be cleaner. I've updated the webrev. Please
>> take a look.
>>
>>   http://cr.openjdk.java.net/~chegar/6893238/webrev.1/webrev/
>>
>> Note: The lack of the initial cause of the IOException in firstToken
>> should not be a problem since the Exception is swallowed in setHeaders
>> ( a few lines above).
> This looks much better - thanks for doing this. Minor bit is that the
> L183 can simply be "throw new IOException(...)".

Done.

Thanks,
-Chris.

>
> -Alan.