On 25 Mar 2016, at 16:21, Pavel Rappo <pavel.ra...@oracle.com> wrote:
> Hi, > > Could you please review my change for JDK-8087113 > > http://cr.openjdk.java.net/~prappo/8087113/webrev.03/ I’ve looked at the API a number of times, and overall I’m happy with it ( modulo the known open issues ). Some initial comments. Generally, I prefer javadoc style comments on members. Even it they are not public, but that could be just me. In addition of the comment regarding more detailed exception messages, maybe a little more detail in assert message would be good, e.g. MessagePublisher - assert offered : backlog.size(); WebSocketBuilderImpl.java - forbidden headers could use a TreeSet with a String.CASE_INSENSITIVE_COMPARATOR and avoid toLowerCase ( and remove your TODO comment ;-) ) OpeningHandshake.java - Are you going to add the URL permission check ? - the use of a private method, prepareForRequest, in the constructor means that nonce and subprotocols cannot be final. Maybe it doesn’t matter, but the private method gains you little over a comment and inlining the method body. - // FIXME: ensure there's exactly 1 header with this name A simple size() == 1 check should suffice. WebSocketImpl.java - v? maybe find a better name, stateVisitor ? - We flip-flopped on the constraint of a single outstanding write, which is still in the API, but MessagePublisher has a default of 10 permits? MessagePublisher.java - this::react feels like a sneaky way of exposing a PRIVAT, not-really-private, method. I’m still reviewing, and will send more comments later. -Chris.
