RFR JDK-8156742: Miscellaneous WebSocket API improvements

[Roger Riggs]

+1 with Chris' suggested rewording.

Roger



On 6/21/2016 7:45 AM, Chris Hegarty wrote:
>> 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
>>>