Expose reusable WebSocket code for WebSocketServer.
Created attachment 117914 [details] Patch
Comment on attachment 117914 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117914&action=review > Source/WebCore/websockets/WebSocketChannel.cpp:959 > +void WebSocketChannel::buildFrame(OpCode opCode, bool maskPayload, const char* data, size_t dataLength, Vector<char>& frame) Could you elaborate this change? I couldn't see any reason to have an option to allow non-masked frames.
Hi yutak@, toyoshim@, do you have any opinion about this? Because this change is superficial and I'm OK to rubber-stamping this Once you guys agree this.
Comment on attachment 117914 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117914&action=review >> Source/WebCore/websockets/WebSocketChannel.cpp:959 >> +void WebSocketChannel::buildFrame(OpCode opCode, bool maskPayload, const char* data, size_t dataLength, Vector<char>& frame) > > Could you elaborate this change? > I couldn't see any reason to have an option to allow non-masked frames. WebSocket's spec say only upstream frames must be masked. I guess that's why he added this option and agree with it.
I have no objection to this change.
Comment on attachment 117914 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117914&action=review >>> Source/WebCore/websockets/WebSocketChannel.cpp:959 >>> -bool WebSocketChannel::sendFrame(OpCode opCode, const char* data, size_t dataLength) >>> +void WebSocketChannel::buildFrame(OpCode opCode, bool maskPayload, const char* data, size_t dataLength, Vector<char>& frame) Adding a bool argument is not good. We had better update the following functions: * Adding buildFrameHeader() * Adding buildUnmaskedFrame() calling buildFrameHeader() and frame.append(data, dataLength) * Updating sendFrame() calling buildFrameHeader(), masking the payload, and sending it.
Created attachment 118841 [details] Patch Thanks for having a look. I checked and I would still need a boolean flag in buildFrameHeader to set the mask bit and append the making key. Here is a patch that replaces the boolean flag by an option enum that contains all the flag bit for the frame and use it in FrameData as well instead of individual booleans. Another option would be to keep it that way and add an enum flag just for the masking, or tell me if you have another idea.
By the way, there is a patch in bug #73092 that goes along the same line as this one. If one of you guys could have a look that would probably be the best.
Comment on attachment 118841 [details] Patch Attachment 118841 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10848074 New failing tests: http/tests/websocket/tests/hybi/reserved-bits.html
Comment on attachment 118841 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118841&action=review The patch generally looks fine to me. If the failure from EWS is real, you need to investigate and fix it. [Note: I'm not a WebKit reviewer.] > Source/WebCore/websockets/WebSocketChannel.h:145 > + static ParseFrameResult parseFrame(const char* data, size_t dataLength, FrameData&, String& errorString); // May modify part of "data" to unmask the frame. The type of |data| should be char* (non-const) to indicate the content of |data| may be modified.
Comment on attachment 118841 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118841&action=review > Source/WebCore/websockets/WebSocketChannel.cpp:634 > + if (isControlOpCode(frame.opCode) && !frame.options & FrameOptionFinal) { Do you mean !(frame.options & FrameOptionFinal) ?
(In reply to comment #7) > Thanks for having a look. I checked and I would still need a boolean flag in buildFrameHeader to set the mask bit and append the making key. Here is a patch that replaces the boolean flag by an option enum that contains all the flag bit for the frame and use it in FrameData as well instead of individual booleans. > > Another option would be to keep it that way and add an enum flag just for the masking, or tell me if you have another idea. ok, I see. The FrameOptions idea looks nice.
(In reply to comment #10) > The patch generally looks fine to me. If the failure from EWS is real, you need to investigate and fix it. [Note: I'm not a WebKit reviewer.] This test was passing on other platform since WTF String would convert the bit-ANDed value automatically to bool for some reason. On chromium the error message was something like "One or more reserved bits are on: reserved1 = 2, reserved2 = 4, reserved3 = 8" instead of "...= 1, ...= 1, ...= 1". It should work now. > The type of |data| should be char* (non-const) to indicate the content of |data| may be modified. Good idea. (In reply to comment #11) > Do you mean !(frame.options & FrameOptionFinal) ? This is embarrassing, nice catch. A new patch is following.
Created attachment 119061 [details] Patch
Comment on attachment 119061 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119061&action=review > Source/WebCore/websockets/WebSocketChannel.cpp:629 > - fail("One or more reserved bits are on: reserved1 = " + String::number(frame.reserved1) + ", reserved2 = " + String::number(frame.reserved2) + ", reserved3 = " + String::number(frame.reserved3)); > + if (frame.options & (FrameOptionReserved1 | FrameOptionReserved2 | FrameOptionReserved3)) { > + fail("One or more reserved bits are on: reserved1 = " + String::number(!!(frame.options & FrameOptionReserved1)) + ", reserved2 = " + String::number(!!(frame.options & FrameOptionReserved2)) + ", reserved3 = " + String::number(!!(frame.options & FrameOptionReserved3))); nit: We have String::format().
(In reply to comment #15) > nit: We have String::format(). Thanks! Will do.
Created attachment 134613 [details] Patch Taking some time to iterate over the inspector server patches again. I had to rebase this patch. The code has now been moved to the new WebSocketFrame instead of exposing straight static methods.
Comment on attachment 134613 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134613&action=review > Source/WebCore/Modules/websockets/WebSocketFrame.cpp:2 > + * Copyright (C) 2012 Nokia Corporation and/or its subsidiary(-ies) Please respect the copyright of the original code.
Created attachment 134749 [details] Patch Preserve the copyrights for WebSocketFrame.cpp
Comment on attachment 134749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=134749&action=review > Source/WebCore/Modules/websockets/WebSocketChannel.cpp:589 > - if (WebSocketFrame::isControlOpCode(frame.opCode) && frame.payloadLength > maxPayloadLengthWithoutExtendedLengthField) { > + if (WebSocketFrame::isControlOpCode(frame.opCode) && frame.payloadLength > WebSocketFrame::maxPayloadLengthWithoutExtendedLengthField) { I recommend adding WebSocketFrame::isControlFrameWithTooLongPayload(), and hide maxPayloadLengthWithoutExtendedLengthField.
Created attachment 135010 [details] Patch Expose maxPayloadLengthWithoutExtendedLengthField as WebSocketFrame::needsExtendedLengthField to make it consistent with the other struct helpers. Tell me if that's alright. WebSocketFrame might also look better as a proper class instead of those helpers.
Comment on attachment 135010 [details] Patch ok
Committed r113025: <http://trac.webkit.org/changeset/113025>