[Webkit-unassigned] [Bug 66362] [WebSocket] CloseEvent's code and reason properties support

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=66362


Kent Tamura <tkent at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #104158|review?                     |review-
               Flag|                            |




--- Comment #3 from Kent Tamura <tkent at chromium.org>  2011-08-18 00:35:08 PST ---
(From update of attachment 104158)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.