[webkit-reviews] review denied: [Bug 66362] [WebSocket] CloseEvent's code and reason properties support : [Attachment 104158] 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 66362: [WebSocket] CloseEvent's code and reason properties support
https://bugs.webkit.org/show_bug.cgi?id=66362

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

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


> Source/WebCore/ChangeLog:11
> +	   Current WebSocket implementation miss code and reason properties
> +	   in CloseEvent. This change expose incoming closing frame's code
> +	   and reason to JavaScript API.
> +	   https://bugs.webkit.org/show_bug.cgi?id=66362
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Tests: http/tests/websocket/tests/hybi/close-code-and-reason.html
> +		 
http/tests/websocket/tests/hybi/workers/close-code-and-reason.html

The ChangeLog format should be:
  <summary>
  <bug URL>

  Reviewed by ...

  <Details>

  Tests: ...

> Source/WebCore/ChangeLog:38
> +	   * websockets/CloseEvent.h:
> +	   (WebCore::CloseEvent::create):
> +	   (WebCore::CloseEvent::initCloseEvent):
> +	   (WebCore::CloseEvent::code):
> +	   (WebCore::CloseEvent::reason):
> +	   (WebCore::CloseEvent::CloseEvent):
> +	   * websockets/CloseEvent.idl:
> +	   * websockets/ThreadableWebSocketChannelClientWrapper.cpp:
> +	   (WebCore::ThreadableWebSocketChannelClientWrapper::didClose):
> +	  
(WebCore::ThreadableWebSocketChannelClientWrapper::didCloseCallback):
> +	   * websockets/ThreadableWebSocketChannelClientWrapper.h:
> +	   * websockets/WebSocket.cpp:
> +	   (WebCore::WebSocket::didConnect):
> +	   (WebCore::WebSocket::didClose):
> +	   * websockets/WebSocket.h:
> +	   * websockets/WebSocketChannel.cpp:
> +	   (WebCore::WebSocketChannel::WebSocketChannel):
> +	   (WebCore::WebSocketChannel::didCloseSocketStream):
> +	   (WebCore::WebSocketChannel::processFrame):
> +	   * websockets/WebSocketChannel.h:
> +	   * websockets/WebSocketChannelClient.h:
> +	   (WebCore::WebSocketChannelClient::didClose):
> +	   * websockets/WorkerThreadableWebSocketChannel.cpp:
> +	   (WebCore::workerContextDidClose):
> +	   (WebCore::WorkerThreadableWebSocketChannel::Peer::didClose):
> +	   * websockets/WorkerThreadableWebSocketChannel.h:

Write what is changed as possible.

> Source/WebCore/websockets/WebSocket.cpp:381
> +	   didClose(0, ClosingHandshakeIncomplete, 0, "");

What does this '0'  mean?

> Source/WebCore/websockets/WebSocketChannel.cpp:101
> +    , m_closeEventCode(1005) // Indicate that no status code was actually
present.

No test for 1005.
We prefer symbols to comments. We should define a const variable or an enum
value for 1005, and remove the comment.

> Source/WebCore/websockets/WebSocketChannel.cpp:632
> +	       if (frame.payloadLength >= 3)
> +		   m_closeEventReason = String::fromUTF8(&frame.payload[2],
frame.payloadLength - 2);

Question: Should we clear m_closeEventReason if frame.payloadLength < 3?