RFR JDK-8156742: Miscellaneous WebSocket API improvements

[Chris Hegarty]

> On 21 Jun 2016, at 12:21, Pavel Rappo <pavel.rappo at oracle.com> wrote:
> 
> Hi, 
> 
> Let me try again to propose the following set of changes:
> 
> http://cr.openjdk.java.net/~prappo/8156742/webrev.02/

This mainly looks fine, just a small comment:

 - WebSocket.java
   "Or to close abruptly call {@link #abort()}.” Rather than start a sentence
    with ‘Or’, maybe “Alternatively, the {@link #abort() abort} method can be
    invoked/used to close abruptly”.


> 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.)

Yes, if it becomes clear that this is indeed useful, then I agree
it can be added later.

> 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

Sounds fine.

-Chris.

>> On 31 May 2016, at 18:25, Pavel Rappo <pavel.rappo at 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
>> 
>