Pavel, On Tue, May 31, 2016 at 7:25 PM, Pavel Rappo <pavel.ra...@oracle.com> wrote: > Hi, > > Could you please review the following change for JDK-8156742? > > http://cr.openjdk.java.net/~prappo/8156742/webrev.01/
Comments on the API only. Looks much cleaner, good job. I like WebSocket.request(long) being reinstated, and the Listener methods taking a WebSocket as first parameter, Builder.header() being sane :) Further comments: * What is interface Text for ? If it does perform a bytes-to-chars conversion, then offering asByteBuffer() can be easily done by the application. A websocket implementation must check that the incoming text bytes are indeed UTF-8. Doing the check is equivalent to creating the corresponding String, so I'm not sure Text is of any help. * CloseReason I would rename it to CloseInfo, as CloseReason hints to me of the reason only, not the code. I think this class exposes too many failure codes that applications *must not* be able to use. For example, 1002 is not something that the application can ever send, only the implementation can, and having a public API to create a 1002 CloseInfo is not something you I'd like to see exposed. Same goes with 1007, which the implementation must detect, not the application; etc. I would probably just leave CloseInfo.of(), with the current checks you are doing extended. * onClose() semantic. I am not sure why CloseInfo is wrapped with an Optional ? Can't the implementation just synthesize a (1005, "") instead and get rid of the Optional ? Also, I think it should return a CF, for the following reason. Notification of onClose() is a half-close. Applications are entitled to send data from within onClose(). For such reason, the implementation cannot send the response close frame just after the method returns. It should wait until the application has finished writing, and hence the need for the CF. * sendText(Stream<? extends CharSequence> message); I think it's too much for a utility method. It's a rare use case, I don't think it's worth it. Applications that wrap the default WebSocket object will have to implement it. ws.sendText(stream.collect(joining())) is equivalent and as short. If the goal was to send one frame per string, there is almost zero chance that the exact fragmentation is maintained at the server, so once again I don't see the reason for this method. * Extensions I don't recall if extensions have been ruled out ? Browsers seem to have settled at implementing permessage-deflate. Thanks ! -- Simone Bordet http://bordet.blogspot.com --- Finally, no matter how good the architecture and design are, to deliver bug-free software with optimal performance and reliability, the implementation technique must be flawless. Victoria Livschitz
