Hi Pavel,
Initial comments, bottom up.
It would be helpful if the classnames/filenames reflected the
participation in the WebSocket implementation
to keep them distinct from the HTTP 2.0 implementation in the same
directory.
For example, Writer, Reader, etc. perhaps a 'Ws' prefix would be
sufficient.
This would be alleviated if websocket was in its in its own package.
java.net.ws
(though I realize that may already have been settled, and I'm sure there
are some dependencies
that would be nice to keep as package private).
Logging is sprinkled throughout the APIs of the implementation; it would
be cleaner to have
a shared WS logging class with a static logger similar to the Http Log
(or perhaps some sharing and reuse)
so the logging does not clutter the implementation interfaces. It would
rare that separate logger
instances are needed.
Writer.java:
- line 143: keep track of the current/next buffer index of the first
non-empty buffer; instead of replacing with NULL_BYTE_BUFFER;
a combination of the buffers array index vs length and hasRemaining
could eliminate the need to copy the buffers array
and perform the initialization to determine the total length (though
that is informative for logging)
- not sure why you are using the 3 arg form of GatheringWriter since
the byteBuffers array is exactly the length.
- line 181: the assert would be more useful inside the loop; catching an
error when it occurs.
Assert reason is more useful with a short descriptive text.
- line 184: editorial, remove "from" from the logger
- line 228: can the Throwable be discarded? Does it contain no information?
Reader.java:
- line 137: Why should the reader force a new task? Should it be
left to the subscriber to
decide whether its processing is lightweight and can be done
immediately or to re-submit to its own executor.
java.net.Utils:
- line 231: SATURATING_INCREMENT (and its uses) is wasted code; long
is not going to roll over.
- line 236: NULL_BYTE_BUFFER isn't null - prehaps rename to
EMPTY_BYTE_BUFFER
$.02, Roger
On 3/25/2016 12:21 PM, Pavel Rappo wrote:
Hi,
Could you please review my change for JDK-8087113
http://cr.openjdk.java.net/~prappo/8087113/webrev.03/
This webrev contains initial implementation and basic tests for WebSocket API.
Specification conformance & interoperability testing has been performed with
Autobahn Testsuite [1]. As for API tests, I will provide more of them later.
Thanks,
-Pavel
--------------------------------------------------------------------------------
[1] http://autobahn.ws/testsuite/
