Summary: | [WebSocket] CloseEvent's code and reason properties support | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Takashi Toyoshima <toyoshim> | ||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, ap, tkent, webkit.review.bot, yutak | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | 66294 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Takashi Toyoshima
2011-08-16 22:49:53 PDT
Created attachment 104158 [details]
Patch
Current patch includes 66294, so I didn't set commit-queue flag. 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? Created attachment 104475 [details]
Patch
Created attachment 104478 [details]
Patch
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. Comment on attachment 104478 [details] Patch Clearing flags on attachment: 104478 Committed r93393: <http://trac.webkit.org/changeset/93393> All reviewed patches have been landed. Closing bug. |