Simone, I'm having second thoughts on it. I think you're right about `onClose` behaviour. Moreover I now understand `onPing` should behave similarly.
The only difference is that replying with Pong should not be correlated with an any feedback (e.g. completion, return, etc.) from `onPing` method. Like when an application has received the `onPing` invocation, the corresponding Pong may or may not be already sent. On the stream thing, well... If you say it's a rare use case, I say let's hear more people. I don't have much experience working with WebSocket as a user, so I would like to listen to people who have it (like yourself). YAGNI is one of my favorite arguments though! It makes the implementation effort feel lighter. Thanks, -Pavel > On 31 May 2016, at 23:05, Simone Bordet <simone.bor...@gmail.com> wrote: > > 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
