RFR [9] 8179021: Latest bugfixes to WebSocket/HPACK from the sandbox repo
Daniel Fuchs
daniel.fuchs at oracle.com
Wed May 10 12:31:34 UTC 2017
Hi Pavel,
Looks good.
Some files didn't have their copyright years updated - maybe
you could fix that before pushing.
jdk/incubator/http/internal/websocket/OpeningHandshake.java
line 121: It might be worth mentioning that '13' is mandated by
[RFC 6455](https://tools.ietf.org/html/rfc6455)
best regards,
-- daniel
On 10/05/2017 12:03, Pavel Rappo wrote:
> Daniel, thanks a lot for looking into this! I've addressed most of your suggestions:
>
> http://cr.openjdk.java.net/~prappo/8179021/webrev.01/
>
>> On 9 May 2017, at 15:49, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
>>
>> Hi Pavel,
>>
>> A few nits:
>>
>> jdk/incubator/http/WebSocket.java:
>>
>> line 501: should use {@linkplain
>>
>> Also (line 46 and 501) the link target #sendClose may cause trouble
>> if sendClose is overloaded - so I wonder if it may be better to
>> have the full prototype there:
>> #sendClose(int, String)
>> (though I guess it's OK as long as there is only 1 sendClose method).
>>
>> jdk/incubator/http/internal/websocket/OutgoingMessage.java
>>
>> 75 // maskingKeys, CharsetDecoder and should be here
>>
>> what does this means? are some words missing?
>>
>> jdk/incubator/http/internal/websocket/Transmitter.java
>>
>> 58 * The supplied handler may be invoked in the calling thread, so watch out
>> 59 * for stack overflow, if there's a possibility of send being called again
>> 60 * from the handler.
>>
>> Can we reformulate this again?
>> Something like:
>>
>> * The supplied handler may be invoked in the calling thread.
>> * A {@code StackOverflowException} may thus occur if there's a
>> * possibility that this method is called again by the supplied
>> * handler.
>>
>> 93 // or tuned off
>>
>> I guess you meant 'turned off'
>>
>> jdk/incubator/http/internal/websocket/WebSocketImpl.java
>>
>> 196 if (alreadyCompleted) {
>> 197 throw new InternalError();
>> ...
>> 321 if (!added) {
>> 322 throw new InternalError();
>>
>> A comment similar to the other places where InternalError is thrown
>> would be nice.
>>
>> best regards,
>>
>> -- daniel
>>
>>
>> On 09/05/2017 14:52, Pavel Rappo wrote:
>>> Hello,
>>>
>>> Please review the following change:
>>>
>>> http://cr.openjdk.java.net/~prappo/8179021/webrev.00/
>>>
>>> Along with refactoring this change contains a number of critical fixes for
>>> WebSocket. Critical fixes are applied to Receiver, WebSocketImpl and
>>> OpeningHandshake.
>>>
>>> Also this change removes 2 convenience methods and 1 constant from WebSocket
>>> interface. Since this interface is a part of incubation feature there shouldn't
>>> be any problem. Those members were initially controversial.
>>>
>>> Thanks,
>>> -Pavel
>>>
>>
>
More information about the net-dev
mailing list