> On 21 Jun 2016, at 12:21, Pavel Rappo <pavel.ra...@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.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
>>
>
