Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11358190:
Add simple WebSocket server implementation. (Closed)
|
Created:
8 years, 1 month ago by Sergey Ulanov Modified:
8 years, 1 month ago Reviewers:
Wez CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd simple WebSocket server implementation.
WebSockets will be used in Chromoting host to communicate with the web app.
BUG=132904
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=169218
Patch Set 1 : #Patch Set 2 : #
Total comments: 31
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 47
Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #
Messages
Total messages: 21 (0 generated)
Hi Sergey, Currently, Chrome contains multiple WebSocket server implementations. 1.) HttpServer and WebSocket in net/server/ This is implementation for Inspector remote debugging. 2.) WebSocketProxy in chrome/browser/chromeos/ This is another implementation for WebSocket to TCP proxy on Chrome OS. If you can reuse 1.) with quick modifications, it must be better than introducing a new implementation. Also it will be nice to reuse client components in net/websockets for implementing your server. From a long term perspective, we should integrate these implementation to one proper server implementation which use net/websockets components. But, we don't have enough throughput to implement it so far.
Thank you Takashi! I looked for existing implementation before writing this code, but found only the one used for ChromeOS. Problem with it is that it's Linux-specific. I didn't see the code in net/socket (I was looking for it in net/websocket :-/). I completely agree that it's best if we didn't have more than one implementation. How stable is the implementation in net/server? Is it intended for production code, or just for tests? I took a brief look at it - it seems to be mostly what we need except the following problems: - Reads are synchronous and not-blocking, which means that receiver has to regularly poll for incoming messages. This would be the biggest problem for us reusing this code. - Support for some features of the protocol is missing, e.g. pings and fragmented messages are not supported. - Looks like not all details of the RFC are implemented correctly, e.g. RFC requires that server closes connection when it receives a message with a reserved bits set. There are some other minor issues like that. On 2012/11/12 04:58:59, Takashi Toyoshima (chromium) wrote: > Hi Sergey, > > Currently, Chrome contains multiple WebSocket server implementations. > > 1.) HttpServer and WebSocket in net/server/ > This is implementation for Inspector remote debugging. > > 2.) WebSocketProxy in chrome/browser/chromeos/ > This is another implementation for WebSocket to TCP proxy on Chrome OS. > > > If you can reuse 1.) with quick modifications, it must be better than > introducing a new implementation. Also it will be nice to reuse client > components in net/websockets for implementing your server. > > From a long term perspective, we should integrate these implementation to one > proper server implementation which use net/websockets components. But, we don't > have enough throughput to implement it so far.
net/server implementation is intended for production. But not implemented by us, WebSocket team. Chrome supports remote debugging using WebSocket via DevTools. AFAIK, It uses this server implementation. IIRC, it had a bug that the server sent masked frames though masking should be applied to client sending frames. I'm not sure if this problem was already fixed. If your schedule permits, modifying net/server implementation to support asynchronous interface and missing WebSocket feature is perfect solution. You can use net/websockets components to parse and build WebSocket frames. If you can not, we will merge server implementations in the future. FYI, for testing, net::TestServer is a generic test server and it becomes to support WebSocket recently. Its core implementation is written in python and support many useful features for testing.
It looks like a lot of functionality that Chromoting needs is missing from the implementation in net/server (see the issues I already mentioned, one more problem is that it doesn't process Origin header). We need to have WebSocket support in chromoting host ASAP, but fixing the code in net/server may take some time. I think the best way forward is to land this CL as is, so that I can proceed integrating it in Chromoting Host (I've already started that work). We can then fix net/server, integrate it with Chromoting and remove duplicate code later. On 2012/11/12 09:06:56, Takashi Toyoshima (chromium) wrote: > net/server implementation is intended for production. But not implemented by us, > WebSocket team. > Chrome supports remote debugging using WebSocket via DevTools. AFAIK, It uses > this server implementation. > > IIRC, it had a bug that the server sent masked frames though masking should be > applied to client sending frames. I'm not sure if this problem was already > fixed. > > If your schedule permits, modifying net/server implementation to support > asynchronous interface and missing WebSocket feature is perfect solution. You > can use net/websockets components to parse and build WebSocket frames. If you > can not, we will merge server implementations in the future. > > FYI, for testing, net::TestServer is a generic test server and it becomes to > support WebSocket recently. Its core implementation is written in python and > support many useful features for testing.
I see. It's ok to land this firstly, and back to integrating later. Thanks!
wez:ping
Some initial comments! https://chromiumcodereview.appspot.com/11358190/diff/5002/remoting/host/webso... File remoting/host/websocket_connection.h (right): https://chromiumcodereview.appspot.com/11358190/diff/5002/remoting/host/webso... remoting/host/websocket_connection.h:33: // |connected_callback| is called when handshake finishes or connection fails. Please reword this comment. :) https://chromiumcodereview.appspot.com/11358190/diff/5002/remoting/host/webso... remoting/host/websocket_connection.h:37: // Initialize WebSocket connection for the specified |socket|. nit: You're calling it WebSocket here, but the classes are WebsocketFoo; should the classes be WebSocketFoo? https://chromiumcodereview.appspot.com/11358190/diff/5002/remoting/host/webso... remoting/host/websocket_connection.h:45: const std::string& origin() { return origin_; } nit: Add a short comment to explain these getters. https://chromiumcodereview.appspot.com/11358190/diff/5002/remoting/host/webso... remoting/host/websocket_connection.h:50: void SendText(const std::string& text); If this implementation only supports text frames, add a comment mentioning that above the class? https://chromiumcodereview.appspot.com/11358190/diff/5002/remoting/host/webso... remoting/host/websocket_connection.h:51: void Close(); nit: Add short comments to explain these methods. https://chromiumcodereview.appspot.com/11358190/diff/5002/remoting/host/webso... remoting/host/websocket_connection.h:79: void UnmaskPayload(const char* mask, char* payload, int payload_length); Again, short comments for each method, please. https://chromiumcodereview.appspot.com/11358190/diff/5002/remoting/host/webso... remoting/host/websocket_connection.h:94: std::string current_packet_; nit: Add a comment to this field, esp given the similarity of name to |current_message_| https://chromiumcodereview.appspot.com/11358190/diff/5002/remoting/host/webso... File remoting/host/websocket_listener.cc (right): https://chromiumcodereview.appspot.com/11358190/diff/5002/remoting/host/webso... remoting/host/websocket_listener.cc:22: const int kTcpListenBacklog = 2; nit: The conventional value for this is 5. Does net/ really not define a default? https://chromiumcodereview.appspot.com/11358190/diff/5002/remoting/host/webso... remoting/host/websocket_listener.cc:36: new_connection_callback_ = callback; nit: Blank line after this. https://chromiumcodereview.appspot.com/11358190/diff/5002/remoting/host/webso... File remoting/host/websocket_listener.h (right): https://chromiumcodereview.appspot.com/11358190/diff/5002/remoting/host/webso... remoting/host/websocket_listener.h:32: // Start listening on the specified port. Returns false in case of a failure. Why not return a net/ error, or provide a result code in the NewConnectionCallback and make this asynchronous? https://chromiumcodereview.appspot.com/11358190/diff/5002/remoting/host/webso... remoting/host/websocket_listener.h:32: // Start listening on the specified port. Returns false in case of a failure. nit: Clarify the tear-down semantics; is |callback| guaranteed never to be called after WebsocketListener is destroyed? https://chromiumcodereview.appspot.com/11358190/diff/5002/remoting/host/webso... remoting/host/websocket_listener.h:32: // Start listening on the specified port. Returns false in case of a failure. nit: port -> address, Start -> Starts https://chromiumcodereview.appspot.com/11358190/diff/5002/remoting/host/webso... remoting/host/websocket_listener.h:33: bool Listen(const net::IPEndPoint& address, nit: If you make |address| an in-out parameter, it can be used both to specify the requested port, and the one that actually got allocated, if the caller passes in zero? https://chromiumcodereview.appspot.com/11358190/diff/5002/remoting/host/webso... remoting/host/websocket_listener.h:40: void OnConnected(WebsocketConnection* connection_ptr, bool handshake_result); Each of these methods need a (short) comment explaining its purpose.
https://codereview.chromium.org/11358190/diff/5002/remoting/host/websocket_co... File remoting/host/websocket_connection.h (right): https://codereview.chromium.org/11358190/diff/5002/remoting/host/websocket_co... remoting/host/websocket_connection.h:33: // |connected_callback| is called when handshake finishes or connection fails. On 2012/11/15 02:28:37, Wez wrote: > Please reword this comment. :) Just removed it. No, I wasn't drunk when writing it, in case you were wondering :-) https://codereview.chromium.org/11358190/diff/5002/remoting/host/websocket_co... remoting/host/websocket_connection.h:37: // Initialize WebSocket connection for the specified |socket|. On 2012/11/15 02:28:37, Wez wrote: > nit: You're calling it WebSocket here, but the classes are WebsocketFoo; should > the classes be WebSocketFoo? WebSocket is the name of the protocol. For type names I think it's better to use WebsocketFoo because WebSocket is a single word. It's the same reason why HttpFoo is better than HTTPFoo. Style guide says "Type names should start with a capital letter and have a capital letter for each new word." and gives example of "UrlTable". I.e. it is not very prescriptive and can be interpreted either way. Let me know if you still disagree, I will rename the types then. https://codereview.chromium.org/11358190/diff/5002/remoting/host/websocket_co... remoting/host/websocket_connection.h:45: const std::string& origin() { return origin_; } On 2012/11/15 02:28:37, Wez wrote: > nit: Add a short comment to explain these getters. Done. https://codereview.chromium.org/11358190/diff/5002/remoting/host/websocket_co... remoting/host/websocket_connection.h:50: void SendText(const std::string& text); On 2012/11/15 02:28:37, Wez wrote: > If this implementation only supports text frames, add a comment mentioning that > above the class? Done. According to RFC the only difference between Text and Data messages is that Text messages are required to be valid UTF-8 strings. Otherwise they are the same. This implementation supports receiving both text and data messages, but handles them the same way - just passes them to OnWebsocketMessage(). As far as I know Chrome (as well as other browsers) currently supports only text message. https://codereview.chromium.org/11358190/diff/5002/remoting/host/websocket_co... remoting/host/websocket_connection.h:51: void Close(); On 2012/11/15 02:28:37, Wez wrote: > nit: Add short comments to explain these methods. Done. https://codereview.chromium.org/11358190/diff/5002/remoting/host/websocket_co... remoting/host/websocket_connection.h:79: void UnmaskPayload(const char* mask, char* payload, int payload_length); On 2012/11/15 02:28:37, Wez wrote: > Again, short comments for each method, please. Done. https://codereview.chromium.org/11358190/diff/5002/remoting/host/websocket_co... remoting/host/websocket_connection.h:94: std::string current_packet_; On 2012/11/15 02:28:37, Wez wrote: > nit: Add a comment to this field, esp given the similarity of name to > |current_message_| Done. Also renamed it to |received_data_| https://codereview.chromium.org/11358190/diff/5002/remoting/host/websocket_li... File remoting/host/websocket_listener.cc (right): https://codereview.chromium.org/11358190/diff/5002/remoting/host/websocket_li... remoting/host/websocket_listener.cc:22: const int kTcpListenBacklog = 2; On 2012/11/15 02:28:37, Wez wrote: > nit: The conventional value for this is 5. Does net/ really not define a > default? Done. https://codereview.chromium.org/11358190/diff/5002/remoting/host/websocket_li... remoting/host/websocket_listener.cc:36: new_connection_callback_ = callback; On 2012/11/15 02:28:37, Wez wrote: > nit: Blank line after this. Done. https://codereview.chromium.org/11358190/diff/5002/remoting/host/websocket_li... File remoting/host/websocket_listener.h (right): https://codereview.chromium.org/11358190/diff/5002/remoting/host/websocket_li... remoting/host/websocket_listener.h:32: // Start listening on the specified port. Returns false in case of a failure. On 2012/11/15 02:28:37, Wez wrote: > nit: port -> address, Start -> Starts Done. https://codereview.chromium.org/11358190/diff/5002/remoting/host/websocket_li... remoting/host/websocket_listener.h:32: // Start listening on the specified port. Returns false in case of a failure. On 2012/11/15 02:28:37, Wez wrote: > nit: Clarify the tear-down semantics; is |callback| guaranteed never to be > called after WebsocketListener is destroyed? Done. https://codereview.chromium.org/11358190/diff/5002/remoting/host/websocket_li... remoting/host/websocket_listener.h:32: // Start listening on the specified port. Returns false in case of a failure. On 2012/11/15 02:28:37, Wez wrote: > Why not return a net/ error, or provide a result code in the > NewConnectionCallback and make this asynchronous? Done. https://codereview.chromium.org/11358190/diff/5002/remoting/host/websocket_li... remoting/host/websocket_listener.h:33: bool Listen(const net::IPEndPoint& address, On 2012/11/15 02:28:37, Wez wrote: > nit: If you make |address| an in-out parameter, it can be used both to specify > the requested port, and the one that actually got allocated, if the caller > passes in zero? I don't think that using in-out parameter here is the right thing to do. We can add GetLocalAddress() to match net::ServerSocket interface, when it becomes necessary. https://codereview.chromium.org/11358190/diff/5002/remoting/host/websocket_li... remoting/host/websocket_listener.h:40: void OnConnected(WebsocketConnection* connection_ptr, bool handshake_result); On 2012/11/15 02:28:37, Wez wrote: > Each of these methods need a (short) comment explaining its purpose. Done.
ping
https://chromiumcodereview.appspot.com/11358190/diff/5002/remoting/host/webso... File remoting/host/websocket_connection.h (right): https://chromiumcodereview.appspot.com/11358190/diff/5002/remoting/host/webso... remoting/host/websocket_connection.h:33: // |connected_callback| is called when handshake finishes or connection fails. On 2012/11/15 21:01:03, sergeyu wrote: > On 2012/11/15 02:28:37, Wez wrote: > > Please reword this comment. :) > > Just removed it. No, I wasn't drunk when writing it, in case you were wondering > :-) :D https://chromiumcodereview.appspot.com/11358190/diff/5002/remoting/host/webso... remoting/host/websocket_connection.h:37: // Initialize WebSocket connection for the specified |socket|. On 2012/11/15 21:01:03, sergeyu wrote: > On 2012/11/15 02:28:37, Wez wrote: > > nit: You're calling it WebSocket here, but the classes are WebsocketFoo; > should > > the classes be WebSocketFoo? > > WebSocket is the name of the protocol. For type names I think it's better to use > WebsocketFoo because WebSocket is a single word. It's the same reason why > HttpFoo is better than HTTPFoo. > Style guide says "Type names should start with a capital letter and have a > capital letter for each new word." and gives example of "UrlTable". I.e. it is > not very prescriptive and can be interpreted either way. Let me know if you > still disagree, I will rename the types then. I think in this case WebSocket fits the style guide better, since "web" and "socket" are words; it's different from HTTP or URL which are abbreviations. Not worth holding up this CL further for, though! https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... File remoting/host/websocket_connection.cc (right): https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_connection.cc:204: int rsv_bits = received_data_.data()[0] & 0x70; nit: Add a comment summarizing what these bits are that aren't supported? https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_connection.cc:220: int length_field_size = 1; Please add a comment summarizing this length processing; that WebSockets has 8(ish)-bit fragment lengths with two special values to introduce 16-bit and 64-bit frame lengths. :) https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_connection.cc:224: // Haven't received the whole frame yet. nit: "Haven't received the whole frame header yet"? https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_connection.cc:242: if (maximum_message_size_ > 0 && length > maximum_message_size_) { nit: Add a comment explaining why we need this check in addition to the message size check below. https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_connection.cc:254: if (mask_bit) { nit: Add a comment to the effect of "un-mask the message fragment" https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_connection.cc:268: current_message_.size() > maximum_message_size_) { It's too late to check this here; the total size of the current message + this fragment needs checking before you cause the extra space to be allocated. https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_connection.cc:329: // Post a task to process the data we have left in the buffer if any. nit: "... left in the buffer, if any." https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... File remoting/host/websocket_connection.h (right): https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_connection.h:42: // |connected_callback| specified in Start() is called with true. nit: "Valid only after |connected_callback_| has been called with |connected| true." or "Valid only once Start() has successfully completed." https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_connection.h:49: // called with connected=true. nit: If you rephrase the comment on start to mention specifically that |connected|=true iff Start() "completes successfully" then I think you can just refer to Start completing successfully here, without the clarification. https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_connection.h:55: // to 0 (default), which indicates that message size is unlimited. nit: "Sets the maximum incoming message size to allow. The connection will be closed if a message exceeding this size is received. |size| may be zero to allow any message size". What's the default size? https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_connection.h:61: // Closes the connection sending Close frame to the client if necessary. nit: "Closes the connection and sends a Close frame ..." https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_connection.h:81: // Closes the socket after protocol error and calls nit: Suggest "Closes the socket in response to a protocol error, and notifies Delegate::OnWebsocketClosed()." https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_connection.h:92: // Parse incoming data in |received_data_| and dispatches delegate calls when nit: Parse -> Parses https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_connection.h:101: const char* payload, int payload_length); nit: Why not just pass payload as std::string or base::StringPiece? https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... File remoting/host/websocket_connection_unittest.cc (right): https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_connection_unittest.cc:102: endpoint_, base::Bind(&WebsocketConnectionTest::OnNewConnection, nit: Indentation. https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_connection_unittest.cc:205: ASSERT_NO_FATAL_FAILURE(ConnectSocket()); Why are these ASSERTs rather than EXPECTs? https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_connection_unittest.cc:215: TEST_F(WebsocketConnectionTest, DisconnectAndConnect) { nit: ConnectAndReconnect? https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_connection_unittest.cc:224: EXPECT_TRUE(connection_.get() == NULL); Why is this EXPECT, not ASSERT as the rest of these tests use? https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_connection_unittest.cc:231: TEST_F(WebsocketConnectionTest, CloseFrame) { nit: CloseFrameIsSent https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_connection_unittest.cc:244: TEST_F(WebsocketConnectionTest, NonWebsocketHeader) { nit: ConnectFailsOnNonWebSocketHeader? https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... File remoting/host/websocket_listener.h (right): https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_listener.h:34: // for each incoming until the listener is destroyed. nit: "... each incoming connection until ..." https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_listener.h:39: // Calls ServerSocket::Accept() to accept new connections. nit: "... to accept the next connection." https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_listener.h:42: // Callback for ServerSocket::Accept(). nit: "Completion callback..." https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_listener.h:45: // Handles Accept() result from DoAccept() or OnAccepted(). nit: "... Accept() completion result from ..." https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_listener.h:51: scoped_ptr<net::TCPServerSocket> tcp_socket_; nit: Add a comment "TCP socket on which to receive connections." https://chromiumcodereview.appspot.com/11358190/diff/16001/remoting/host/webs... remoting/host/websocket_listener.h:59: // are owned by the listener until we receive headers and call the callback. We should limit the number of pending connections, e.g. by closing the oldest when a new connection is received. We should really also maintain a timeout for each pending connection's handshake process.
https://codereview.chromium.org/11358190/diff/5002/remoting/host/websocket_co... File remoting/host/websocket_connection.h (right): https://codereview.chromium.org/11358190/diff/5002/remoting/host/websocket_co... remoting/host/websocket_connection.h:37: // Initialize WebSocket connection for the specified |socket|. On 2012/11/20 05:44:09, Wez wrote: > On 2012/11/15 21:01:03, sergeyu wrote: > > On 2012/11/15 02:28:37, Wez wrote: > > > nit: You're calling it WebSocket here, but the classes are WebsocketFoo; > > should > > > the classes be WebSocketFoo? > > > > WebSocket is the name of the protocol. For type names I think it's better to > use > > WebsocketFoo because WebSocket is a single word. It's the same reason why > > HttpFoo is better than HTTPFoo. > > Style guide says "Type names should start with a capital letter and have a > > capital letter for each new word." and gives example of "UrlTable". I.e. it is > > not very prescriptive and can be interpreted either way. Let me know if you > > still disagree, I will rename the types then. > > I think in this case WebSocket fits the style guide better, since "web" and > "socket" are words; it's different from HTTP or URL which are abbreviations. > Not worth holding up this CL further for, though! Ok. Renamed everything to WebSocket. https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... File remoting/host/websocket_connection.cc (right): https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... remoting/host/websocket_connection.cc:204: int rsv_bits = received_data_.data()[0] & 0x70; On 2012/11/20 05:44:09, Wez wrote: > nit: Add a comment summarizing what these bits are that aren't supported? Done. https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... remoting/host/websocket_connection.cc:220: int length_field_size = 1; On 2012/11/20 05:44:09, Wez wrote: > Please add a comment summarizing this length processing; that WebSockets has > 8(ish)-bit fragment lengths with two special values to introduce 16-bit and > 64-bit frame lengths. :) Done. https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... remoting/host/websocket_connection.cc:224: // Haven't received the whole frame yet. On 2012/11/20 05:44:09, Wez wrote: > nit: "Haven't received the whole frame header yet"? Done. https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... remoting/host/websocket_connection.cc:242: if (maximum_message_size_ > 0 && length > maximum_message_size_) { On 2012/11/20 05:44:09, Wez wrote: > nit: Add a comment explaining why we need this check in addition to the message > size check below. Done. https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... remoting/host/websocket_connection.cc:254: if (mask_bit) { On 2012/11/20 05:44:09, Wez wrote: > nit: Add a comment to the effect of "un-mask the message fragment" Done. https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... remoting/host/websocket_connection.cc:268: current_message_.size() > maximum_message_size_) { On 2012/11/20 05:44:09, Wez wrote: > It's too late to check this here; the total size of the current message + this > fragment needs checking before you cause the extra space to be allocated. Done, but it doesn't really matter much. https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... remoting/host/websocket_connection.cc:329: // Post a task to process the data we have left in the buffer if any. On 2012/11/20 05:44:09, Wez wrote: > nit: "... left in the buffer, if any." Done. https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... File remoting/host/websocket_connection.h (right): https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... remoting/host/websocket_connection.h:42: // |connected_callback| specified in Start() is called with true. On 2012/11/20 05:44:09, Wez wrote: > nit: "Valid only after |connected_callback_| has been called with |connected| > true." or "Valid only once Start() has successfully completed." Done. https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... remoting/host/websocket_connection.h:49: // called with connected=true. On 2012/11/20 05:44:09, Wez wrote: > nit: If you rephrase the comment on start to mention specifically that > |connected|=true iff Start() "completes successfully" then I think you can just > refer to Start completing successfully here, without the clarification. Done. https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... remoting/host/websocket_connection.h:55: // to 0 (default), which indicates that message size is unlimited. On 2012/11/20 05:44:09, Wez wrote: > nit: "Sets the maximum incoming message size to allow. The connection will be > closed if a message exceeding this size is received. |size| may be zero to allow > any message size". > > What's the default size? Done. https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... remoting/host/websocket_connection.h:61: // Closes the connection sending Close frame to the client if necessary. On 2012/11/20 05:44:09, Wez wrote: > nit: "Closes the connection and sends a Close frame ..." Done. https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... remoting/host/websocket_connection.h:81: // Closes the socket after protocol error and calls On 2012/11/20 05:44:09, Wez wrote: > nit: Suggest "Closes the socket in response to a protocol error, and notifies > Delegate::OnWebsocketClosed()." Done. https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... remoting/host/websocket_connection.h:92: // Parse incoming data in |received_data_| and dispatches delegate calls when On 2012/11/20 05:44:09, Wez wrote: > nit: Parse -> Parses Done. https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... remoting/host/websocket_connection.h:92: // Parse incoming data in |received_data_| and dispatches delegate calls when On 2012/11/20 05:44:09, Wez wrote: > nit: Parse -> Parses Done. https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... remoting/host/websocket_connection.h:101: const char* payload, int payload_length); On 2012/11/20 05:44:09, Wez wrote: > nit: Why not just pass payload as std::string or base::StringPiece? Done. https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... File remoting/host/websocket_connection_unittest.cc (right): https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... remoting/host/websocket_connection_unittest.cc:102: endpoint_, base::Bind(&WebsocketConnectionTest::OnNewConnection, On 2012/11/20 05:44:09, Wez wrote: > nit: Indentation. Done. https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... remoting/host/websocket_connection_unittest.cc:205: ASSERT_NO_FATAL_FAILURE(ConnectSocket()); On 2012/11/20 05:44:09, Wez wrote: > Why are these ASSERTs rather than EXPECTs? Because we don't want to call ConnectSocket() if Initialize() failed. It may crash the test if Initialize() wasn't successful. https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... remoting/host/websocket_connection_unittest.cc:215: TEST_F(WebsocketConnectionTest, DisconnectAndConnect) { On 2012/11/20 05:44:09, Wez wrote: > nit: ConnectAndReconnect? Done. https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... remoting/host/websocket_connection_unittest.cc:224: EXPECT_TRUE(connection_.get() == NULL); On 2012/11/20 05:44:09, Wez wrote: > Why is this EXPECT, not ASSERT as the rest of these tests use? ASSERT is preferred when failed condition may crash the test. In this case the test will not crash if connection_ is NULL. It's consistent with other tests in this file: EXPECT for ==NULL and ASSERT for !=NULL. It's not super important though. https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... remoting/host/websocket_connection_unittest.cc:231: TEST_F(WebsocketConnectionTest, CloseFrame) { On 2012/11/20 05:44:09, Wez wrote: > nit: CloseFrameIsSent Done. https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... remoting/host/websocket_connection_unittest.cc:244: TEST_F(WebsocketConnectionTest, NonWebsocketHeader) { On 2012/11/20 05:44:09, Wez wrote: > nit: ConnectFailsOnNonWebSocketHeader? Done.
Uploaded new patchset. On 2012/11/21 01:40:23, sergeyu wrote: > https://codereview.chromium.org/11358190/diff/5002/remoting/host/websocket_co... > File remoting/host/websocket_connection.h (right): > > https://codereview.chromium.org/11358190/diff/5002/remoting/host/websocket_co... > remoting/host/websocket_connection.h:37: // Initialize WebSocket connection for > the specified |socket|. > On 2012/11/20 05:44:09, Wez wrote: > > On 2012/11/15 21:01:03, sergeyu wrote: > > > On 2012/11/15 02:28:37, Wez wrote: > > > > nit: You're calling it WebSocket here, but the classes are WebsocketFoo; > > > should > > > > the classes be WebSocketFoo? > > > > > > WebSocket is the name of the protocol. For type names I think it's better to > > use > > > WebsocketFoo because WebSocket is a single word. It's the same reason why > > > HttpFoo is better than HTTPFoo. > > > Style guide says "Type names should start with a capital letter and have a > > > capital letter for each new word." and gives example of "UrlTable". I.e. it > is > > > not very prescriptive and can be interpreted either way. Let me know if you > > > still disagree, I will rename the types then. > > > > I think in this case WebSocket fits the style guide better, since "web" and > > "socket" are words; it's different from HTTP or URL which are abbreviations. > > Not worth holding up this CL further for, though! > > Ok. Renamed everything to WebSocket. > > https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... > File remoting/host/websocket_connection.cc (right): > > https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... > remoting/host/websocket_connection.cc:204: int rsv_bits = > received_data_.data()[0] & 0x70; > On 2012/11/20 05:44:09, Wez wrote: > > nit: Add a comment summarizing what these bits are that aren't supported? > > Done. > > https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... > remoting/host/websocket_connection.cc:220: int length_field_size = 1; > On 2012/11/20 05:44:09, Wez wrote: > > Please add a comment summarizing this length processing; that WebSockets has > > 8(ish)-bit fragment lengths with two special values to introduce 16-bit and > > 64-bit frame lengths. :) > > Done. > > https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... > remoting/host/websocket_connection.cc:224: // Haven't received the whole frame > yet. > On 2012/11/20 05:44:09, Wez wrote: > > nit: "Haven't received the whole frame header yet"? > > Done. > > https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... > remoting/host/websocket_connection.cc:242: if (maximum_message_size_ > 0 && > length > maximum_message_size_) { > On 2012/11/20 05:44:09, Wez wrote: > > nit: Add a comment explaining why we need this check in addition to the > message > > size check below. > > Done. > > https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... > remoting/host/websocket_connection.cc:254: if (mask_bit) { > On 2012/11/20 05:44:09, Wez wrote: > > nit: Add a comment to the effect of "un-mask the message fragment" > > Done. > > https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... > remoting/host/websocket_connection.cc:268: current_message_.size() > > maximum_message_size_) { > On 2012/11/20 05:44:09, Wez wrote: > > It's too late to check this here; the total size of the current message + this > > fragment needs checking before you cause the extra space to be allocated. > > Done, but it doesn't really matter much. > > https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... > remoting/host/websocket_connection.cc:329: // Post a task to process the data we > have left in the buffer if any. > On 2012/11/20 05:44:09, Wez wrote: > > nit: "... left in the buffer, if any." > > Done. > > https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... > File remoting/host/websocket_connection.h (right): > > https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... > remoting/host/websocket_connection.h:42: // |connected_callback| specified in > Start() is called with true. > On 2012/11/20 05:44:09, Wez wrote: > > nit: "Valid only after |connected_callback_| has been called with |connected| > > true." or "Valid only once Start() has successfully completed." > > Done. > > https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... > remoting/host/websocket_connection.h:49: // called with connected=true. > On 2012/11/20 05:44:09, Wez wrote: > > nit: If you rephrase the comment on start to mention specifically that > > |connected|=true iff Start() "completes successfully" then I think you can > just > > refer to Start completing successfully here, without the clarification. > > Done. > > https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... > remoting/host/websocket_connection.h:55: // to 0 (default), which indicates that > message size is unlimited. > On 2012/11/20 05:44:09, Wez wrote: > > nit: "Sets the maximum incoming message size to allow. The connection will be > > closed if a message exceeding this size is received. |size| may be zero to > allow > > any message size". > > > > What's the default size? > > Done. > > https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... > remoting/host/websocket_connection.h:61: // Closes the connection sending Close > frame to the client if necessary. > On 2012/11/20 05:44:09, Wez wrote: > > nit: "Closes the connection and sends a Close frame ..." > > Done. > > https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... > remoting/host/websocket_connection.h:81: // Closes the socket after protocol > error and calls > On 2012/11/20 05:44:09, Wez wrote: > > nit: Suggest "Closes the socket in response to a protocol error, and notifies > > Delegate::OnWebsocketClosed()." > > Done. > > https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... > remoting/host/websocket_connection.h:92: // Parse incoming data in > |received_data_| and dispatches delegate calls when > On 2012/11/20 05:44:09, Wez wrote: > > nit: Parse -> Parses > > Done. > > https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... > remoting/host/websocket_connection.h:92: // Parse incoming data in > |received_data_| and dispatches delegate calls when > On 2012/11/20 05:44:09, Wez wrote: > > nit: Parse -> Parses > > Done. > > https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... > remoting/host/websocket_connection.h:101: const char* payload, int > payload_length); > On 2012/11/20 05:44:09, Wez wrote: > > nit: Why not just pass payload as std::string or base::StringPiece? > > Done. > > https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... > File remoting/host/websocket_connection_unittest.cc (right): > > https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... > remoting/host/websocket_connection_unittest.cc:102: endpoint_, > base::Bind(&WebsocketConnectionTest::OnNewConnection, > On 2012/11/20 05:44:09, Wez wrote: > > nit: Indentation. > > Done. > > https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... > remoting/host/websocket_connection_unittest.cc:205: > ASSERT_NO_FATAL_FAILURE(ConnectSocket()); > On 2012/11/20 05:44:09, Wez wrote: > > Why are these ASSERTs rather than EXPECTs? > > Because we don't want to call ConnectSocket() if Initialize() failed. It may > crash the test if Initialize() wasn't successful. > > https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... > remoting/host/websocket_connection_unittest.cc:215: > TEST_F(WebsocketConnectionTest, DisconnectAndConnect) { > On 2012/11/20 05:44:09, Wez wrote: > > nit: ConnectAndReconnect? > > Done. > > https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... > remoting/host/websocket_connection_unittest.cc:224: > EXPECT_TRUE(connection_.get() == NULL); > On 2012/11/20 05:44:09, Wez wrote: > > Why is this EXPECT, not ASSERT as the rest of these tests use? > > ASSERT is preferred when failed condition may crash the test. In this case the > test will not crash if connection_ is NULL. It's consistent with other tests in > this file: EXPECT for ==NULL and ASSERT for !=NULL. It's not super important > though. > > https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... > remoting/host/websocket_connection_unittest.cc:231: > TEST_F(WebsocketConnectionTest, CloseFrame) { > On 2012/11/20 05:44:09, Wez wrote: > > nit: CloseFrameIsSent > > Done. > > https://codereview.chromium.org/11358190/diff/16001/remoting/host/websocket_c... > remoting/host/websocket_connection_unittest.cc:244: > TEST_F(WebsocketConnectionTest, NonWebsocketHeader) { > On 2012/11/20 05:44:09, Wez wrote: > > nit: ConnectFailsOnNonWebSocketHeader? > > Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/11358190/21001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/11358190/12005
Sorry for I got bad news for ya. Compile failed with a clobber build on win. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/11358190/15011
Sorry for I got bad news for ya. Compile failed with a clobber build on win_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/11358190/12008
Change committed as 169218 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
