[webkit-reviews] review denied: [Bug 82307] [WebSocket]Browser must fail connection if Sec-WebSocket-Protocol mismatched. : [Attachment 134001] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied joey <li.yin at intel.com>'s
request for review:
Bug 82307: [WebSocket]Browser must fail connection if Sec-WebSocket-Protocol
mismatched.
https://bugs.webkit.org/show_bug.cgi?id=82307

Attachment 134001: Patch
https://bugs.webkit.org/attachment.cgi?id=134001&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=134001&action=review


As for the previous patches. I do not know enough about the state of WebSocket
to r+ so I'll leave the final review to a more knowledgeable reviewer.

> Source/WebCore/ChangeLog:9
> +	   From RFC6455, if the WebSocket openhanding respond included the
mismatched
> +	   Sec-WebSocket-Protocol header field, the client must fail the
WebSocket Connection.

I think it is useful when you also add a link to the spec.

> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:729
> +	       size_t i;

The variable name "i" is a bad name in this context. We use "i" when it is
stopped an obvious what it is.

Here, you should use something like serverWebSocketProtocolIndex or better.

>
LayoutTests/http/tests/websocket/tests/hybi/handshake-fail-by-mismatch-protocol
-header.html:24
> +    if (protocolCase[curTestNumber] === "")
> +	   ws = new WebSocket(url);

How does this case happen?

All the tests are run with integers. The strict equality does not seems to mach
any of the invocation.

>
LayoutTests/http/tests/websocket/tests/hybi/handshake-fail-by-mismatch-protocol
-header_wsh.py:13
> +    raise handshake.AbortedByUserException('Abort the connection') #
Prevents pywebsocket from sending its own handshake message.

The comment should probably be on the above line if you want to follow python's
style.