Hi Pavel, I have only cosmetic comments. Please feel free to ignore anything you don’t like. ;-)
> Could you please review my change for JDK-8087113 > > http://cr.openjdk.java.net/~prappo/8087113/webrev.03/ src/java.httpclient/share/classes/java/net/http/RawChannel.java When you don’t plan to have any subclasses of RawChannel, then please mark it as final. 43 private boolean closed; I think this field should be volatile. src/java.httpclient/share/classes/java/net/http/Utils.java This class should be final. And please add a private empty constructor. 215 .append(" rem=").append(b.remaining()).append("]").toString(); Please use ']' instead of "]”. 222 .append("[len=").append(s.length()).append("]").toString(); Please use ']' instead of "]”. 229 final static LongBinaryOperator SATURATING_INCREMENT It should be “static final” and not “final static”. 232 static final System.Logger logger = System.getLogger("java.net.http.WebSocket”); logger should be in uppercase. 234 static final ByteBuffer NULL_BYTE_BUFFER = ByteBuffer.allocate(0); Maybe EMPTY_BYTE_BUFFER is a better name. Without seeing the code I would expect that NULL_BYTE_BUFFER is the same as null and not an empty buffer. src/java.httpclient/share/classes/java/net/http/package-info.java 31 * WebSocket works is asynchronous mode only. The main types defined are: Typo: "works is asynchronous” -> "works in asynchronous”. src/java.httpclient/share/classes/java/net/http/BuffersSubscriber.java 393 (this.subscription = requireNonNull(subscription)).request(1); Code like this is difficult to read and understand. Maybe you should rewrite it using two lines of code. src/java.httpclient/share/classes/java/net/http/CharsetToolkit.java Maybe the lines 36-45 should be converted to a JavaDocs comment. 52 static class Verifier { The class should be final. 92 public final CoderResult encode(CharBuffer in, ByteBuffer out, boolean endOfInput) The class Encoder is final. What is the purpose to mark this method as final? And I think it should not be public. 132 public final CoderResult decode(ByteBuffer in, CharBuffer out, boolean endOfInput) The class Decoder is final. What is the purpose to mark this method as final? And I think it should not be public. src/java.httpclient/share/classes/java/net/http/MessagePublisher.java 107 // message and and enqueueing the pair. Typo: one “and” too much. 177 throw new IllegalArgumentException(String.valueOf(n)); Could you please provide a better message for exception here? 192 private static class CheckingVisitor implements Visitor<Void, Void> { This class can be final. 264 try { 265 encodedReason = StandardCharsets.UTF_8.newEncoder() 266 .encode(CharBuffer.wrap(close.reason)); 267 } catch (CharacterCodingException e) { 268 throw new IllegalArgumentException( 269 "The closure reason is a malformed UTF-16 sequence", e); 270 } I think it should be "a malformed UTF-8 sequence” as you use UTF-8 charset. 287 private void checkSize(int size, int maxSize) { 288 if (size > maxSize) { 289 throw new IllegalArgumentException 290 ("The message is too long: " + size); 291 } 292 } Please add maxSize to the exception message too. src/java.httpclient/share/classes/java/net/http/MessageSender.java 116 throw new IllegalArgumentException(String.valueOf(n)); Could you please provide a better message for exception here? 340 throw new IllegalArgumentException("Malformed UTF-16 characters", e); I think the message of the exception should be "Malformed UTF-8 characters”. 388 (this.subscription = Objects.requireNonNull(subscription)).request(1); Objects.requireNonNull is already imported using a static import. But again I would rewrite it and use two lines of code as code like this is difficult to read and understand. src/java.httpclient/share/classes/java/net/http/OpeningHandshake.java 57 private final static SecureRandom srandom; 58 private final static MessageDigest sha1; It should be “private static final”. 96 webSocketUri.getScheme().equals("ws") ? "http" : "https”; Maybe you should use here equalsIgnoreCase(). src/java.httpclient/share/classes/java/net/http/Reader.java 46 private volatile Flow.Subscriber<? super Shared<ByteBuffer>> subscriber; I would move this line after the line with the last final field (private final RawChannel channel). 55 private volatile boolean started, closed; Please use separate declaration for every field. 93 throw new IllegalStateException(); Please add a message to the exception. 192 throw new IllegalArgumentException(String.valueOf(n)); Could you please provide a better message for exception here? src/java.httpclient/share/classes/java/net/http/SequentialExecutor.java Maybe the lines 9-12 should be converted to a JavaDocs comment. src/java.httpclient/share/classes/java/net/http/Shared.java Maybe the lines 33-43 should be converted to a JavaDocs comment. 156 // We don't work with buffers of other types 157 throw new IllegalArgumentException(); Please add a message to the exception. The same applies for the lines 171 and 184. src/java.httpclient/share/classes/java/net/http/SignalHandler.java Maybe the lines 33-46 should be converted to a JavaDocs comment. 112 switch (s) { 113 case RUNNING: 114 return RERUN; 115 case DONE: 116 return RUNNING; 117 case RERUN: 118 return RERUN; 119 default: 120 throw new InternalError(String.valueOf(s)); 121 } Please fix the indentation. 120 throw new InternalError(String.valueOf(s)); Could you please provide a better message for exception here? src/java.httpclient/share/classes/java/net/http/WebSocket.java Could you please add an #of-method to CloseCode which also takes a description as parameter, e.g.: public static CloseCode of(int code, String description) 1221 return Objects.hash(code); I think it should be changed to return just the code. src/java.httpclient/share/classes/java/net/http/WebSocketBuilderImpl.java 45 private final static Set<String> forbiddenHeadersLowerCased = Set.of( It should be “private static final”. And forbiddenHeadersLowerCased should be in uppercase. 74 if (forbiddenHeadersLowerCased.contains(name.toLowerCase())) { Please use name.toLowerCase(Locale.ROOT). Another solution is to use a TreeSet with the String.CASE_INSENSITIVE_ORDER comparator. src/java.httpclient/share/classes/java/net/http/WebSocketFrame.java 161 private final static class Masker64 extends Masker { It should be “private static final”. 195 private final static class Masker32 extends Masker { It should be “private static final”. 244 if (value) 245 firstChar |= 0b10000000_00000000; 246 else 247 firstChar &= ~0b10000000_00000000; Please add curly braces. The same applies to the lines 256-259, 268-271, 280-283, 300-305. 393 // A private buffer used to simplify multi-byte integers reading 394 private final ByteBuffer accumulator = ByteBuffer.allocate(8); The declaration of this field should be moved after the declaration of constants in the Reader class. src/java.httpclient/share/classes/java/net/http/WebSocketHandshakeException.java 36 private transient HttpResponse response; The field can be final. src/java.httpclient/share/classes/java/net/http/WebSocketImpl.java 310 throw new IllegalStateException(); Please add a message to the exception. 359 private enum State { 360 CONNECTED, 361 CLOSED_REMOTELY, 362 CLOSED_LOCALLY, 363 CLOSED { 364 @Override 365 boolean isClosed() { return true; } 366 }, 367 ABORTED { 368 @Override 369 boolean isClosed() { return true; } 370 }; 371 372 boolean isClosed() { return false; } 373 } When you change the implementation of the isClosed()-method as follows: boolean isClosed() { return this == CLOSED || this == ABORTED; } then you can avoid anonymous inner sub-classes. src/java.httpclient/share/classes/java/net/http/Writer.java 207 (this.subscription = requireNonNull(subscription)).request(1); Code like this is difficult to read and understand. Maybe you should rewrite it using two lines of code. test/java/net/httpclient/HandshakePhase.java The copyright header is missed. Best regards, Andrej Golovnin
