RFR JDK-8087113: Websocket API and implementation

[Chris Hegarty]

On 25 Mar 2016, at 16:21, Pavel Rappo <pavel.rappo at 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.