[webkit-reviews] review denied: [Bug 66925] [WebSocket] WebSocket::close didn't handle code and reason arguments : [Attachment 105767] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Takashi Toyoshima
<toyoshim at chromium.org>'s request for review:
Bug 66925: [WebSocket] WebSocket::close didn't handle code and reason arguments
https://bugs.webkit.org/show_bug.cgi?id=66925

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

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=105767&action=review


r- for some style issues.

> LayoutTests/http/tests/websocket/tests/hybi/close-expected.txt:7
> +invalid code test: 0

nit: Capitalize: invalid -> Invalid

> LayoutTests/http/tests/websocket/tests/hybi/close-expected.txt:8
> +code 999 must cause INVALID_ACCESS_ERR

nit: This is a sentence.  So "code" should be "Code", and should end with '.'.

> LayoutTests/http/tests/websocket/tests/hybi/close.html:48
> +var onopen = function ()

The names onopen, onerror, onclose, onmessage are confusing.
I recommend to rename them like handleOpen, handleError, ....

> LayoutTests/http/tests/websocket/tests/hybi/close.html:68
> +var set_defaults = function (ws)

We usually use the naming rule same as C++.  So this should be "setDefaults".
Also, there is no reason to use "var".

function setDefaultHandlers(ws)
{
  ....

> LayoutTests/http/tests/websocket/tests/hybi/close.html:76
> +var run_code_test = function ()

ditto.

function runCodeTest() ...

> LayoutTests/http/tests/websocket/tests/hybi/close.html:106
> +var run_invalid_string_test = function ()

ditto.

function runInvalidStringTest() ...

> LayoutTests/http/tests/websocket/tests/hybi/close.html:113
> +var run_reason_test = function (next_test)

ditto.

function runReasonTest() ...

> Source/WebCore/bindings/js/JSWebSocketCustom.cpp:109
> +	       // in optional DOMString reason

It seems that this comment adds no information to the code.
It should be removed.

> Source/WebCore/bindings/v8/custom/V8WebSocketCustom.cpp:131
> +	       // in optional DOMString reason

ditto.