Bug 66362 – [WebSocket] CloseEvent's code and reason properties support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Takashi Toyoshima]

I took over this work from yutak.

Current WebSocket implementation miss CloseEvent's code and reason properties handling for updated WebSocket API.
First, I make changes to show incoming closing frame's code and reason as a CloseEvent's properties.

[Takashi Toyoshima]

Created attachment 104158 [details]
Patch

[Takashi Toyoshima]

Current patch includes 66294, so I didn't set commit-queue flag.

[Kent Tamura]

Comment on attachment 104158 [details]
Patch

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?

[Takashi Toyoshima]

Created attachment 104475 [details]
Patch

[Takashi Toyoshima]

Created attachment 104478 [details]
Patch

[Kent Tamura]

Comment on attachment 104478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104478&action=review

Looks ok.

> Source/WebCore/ChangeLog:30
> +        Add implement to handle code and reason.

nit: implement -> implementation ?

> Source/WebCore/ChangeLog:43
> +        Add implement to handle code and reason.

ditto.

[WebKit Review Bot]

Comment on attachment 104478 [details]
Patch

Clearing flags on attachment: 104478

Committed r93393: <http://trac.webkit.org/changeset/93393>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.