Thanks Daniel. Suggested changes made and new webrev at:
http://cr.openjdk.java.net/~michaelm/8199849/webrev.4/
- Michael.
On 06/08/2019, 14:37, Daniel Fuchs wrote:
Hi Michael, Patrick,
BasicAuthentication.java:
The two constructors might benefit from a bit of refactoring
allowing them to share the code that encode the credentials.
BasicAuthenticator.java:
76 if ("".equals(realm))
I'd suggest using
if (realm.isEmpty())
If you do so you might also decide to add a comment that
this is an implicit null check and decide to remove the
previous line: Objects.requiresNonNull(realm).
BasicAuthenticatorCharset.java:
This appears as a new test, yet it has a copyright
line 2005, 2019, and two @bug refs?
typo: setAuthenticatonPW
146 // should fail once with UNICODE_PW and unsupporting
character set
147 if (failCount != 1)
148 throw new RuntimeException("Fail count exceeded: 1 !=
" + failCount);
well... OK. But technically the test could succeed
if the failing charset didn't fail but the passing
charset did.
It would be much better to test that the charset that
is expected to fail fails and that the charset that
is expected to pass passes...
TestHttpUnicode.java
Are the copyright years OK or is this just
copy-paste mistake?
44 static class testAuthenticator extends
java.net.Authenticator {
can this class start with an upper case please?
it's a bit confusing given that the server authenticator is a
stored in a variable named `testAuthenticator` as well.
Otherwise, looks good to me!
best regards,
-- daniel
On 06/08/2019 12:44, Michael McMahon wrote:
This change went through a couple of rounds of review before, and I
got diverted to something else
at the time. So, just picking it up now.
Bug: https://bugs.openjdk.java.net/browse/JDK-8199849
Webrev: http://cr.openjdk.java.net/~michaelm/8199849/webrev.3/index.html
CSR: https://bugs.openjdk.java.net/browse/JDK-8215275
The two tests were contributed by Patrick Concannon.
Thanks,
Michael.
