Hi, Let me try again to propose the following set of changes:
http://cr.openjdk.java.net/~prappo/8156742/webrev.02/ The difference between this version and the previous one is that there are no controversial changes to onClose method [*]. This version also removes `sendText(Stream<? extends CharSequence> message)` which is not really essential and indeed can be added later (if needed.) In other words, this version address what's been discussed in the thread after the previous version has been sent. Thanks for your help, -Pavel -------------------------------------------------------------------------------- [*] Please note that onClose/sendClose/onPing changes will be addressed in the upcoming https://bugs.openjdk.java.net/browse/JDK-8159053 > On 31 May 2016, at 18:25, 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/ > > This change addresses the first group of WebSocket API refinements and > enhancements from [1]. > > 1. Change method `Builder#connectTimeout(long, TimeUnit)` to > `Builder#connectTimeout(Duration)` > > Make use of convenience introduced with java.time API. The builder is not a > performance-critical place, so there's no harm in constructing an object of > `java.time.Duration` each time it's needed. Moreover, since 9, there's a > bridge > between TimeUnit and Duration: java.util.concurrent.TimeUnit.toChronoUnit > > 2. Change method `long WebSocket#request(long)` to `void > WebSocket#request(long)` > > Otherwise a detail of implementation becomes a part of the spec. In this case > it's not desirable, since we'll have to specify the behaviour in the corner > case > (long wrap) and force future implementations to maintain this abstraction. > > 3. Remove method `WebSocket#sendBinary(byte[], boolean)` > > This method provides not enough convenience to justify its existence. > > 4. Change type `CloseCode` for `CloseReason` that aggregates both status code > and close reason. > > Current `Listener.onClose` looks ugly. It hides the otherwise explicit to all > WebSocket users knowledge that 'reason' string can't go without the > 'status code', i.e.: > > (statusCode reason?)? > > CloseReason types fuses both entities into a single type. As a bonus all > knowledge about status code and reason string formats is now bound to a single > place. > > 5. Specify `WebSocket#sendClose` idempotency > > Not producing IllegalStateException upon an attempt to close an already closed > WebSocket seems to be a user-friendly solution. It's already an established > practice > in the JDK, e.g. java.nio.channels.SocketChannel.shutdownOutput > > 6. A number of miscellaneous editorial changes, missing copyright headers, > tests. > > Thanks, > -Pavel > > -------------------------------------------------------------------------------- > [1] https://bugs.openjdk.java.net/browse/JDK-8155621 >
