Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12764006:
WebSocketChannel implementation (Closed)
|
Created:
7 years, 9 months ago by Adam Rice Modified:
7 years, 5 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, tyoshino (SeeGerritForStatus) Base URL:
http://git.chromium.org/chromium/src.git@web_socket_dispatcher Visibility:
Public. |
DescriptionWebSocketChannel implements the transport-independent parts of the WebSocket protocol. It also provides the interface to the content library.
The unit tests are still incomplete at the moment.
BUG=230756
TEST=net_unittests --gtest_filter='WebSocketChannel*'
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212019
Patch Set 1 #Patch Set 2 : First real implementation of WebSocketChannel #Patch Set 3 : Add unit tests and fix bugs. #
Total comments: 41
Patch Set 4 : Changes in response to review comments. #
Total comments: 16
Patch Set 5 : Fix potential NULL-pointer dereference in ProcessFrameChunks. #
Total comments: 8
Patch Set 6 : Rename OnSendFrame to OnDataFrame #Patch Set 7 : Small fixes. #
Total comments: 10
Patch Set 8 : Simplify ProcessFrameChunk #Patch Set 9 : Tiny comment fixes. #Patch Set 10 : Rename (reason, reason_text) to (code, reason) #Patch Set 11 : Compile fixes for Windows. #Patch Set 12 : Make ConnectDelegate constructor explicit #
Total comments: 26
Patch Set 13 : Fixes to comments and variable names. #
Total comments: 14
Patch Set 14 : Comment and variable name fixes to tests. #Patch Set 15 : Fix "OverFlow" -> "Overflow" #
Total comments: 10
Patch Set 16 : Change close status code to uint16 #
Total comments: 46
Patch Set 17 : Code refactoring and cleanup. #
Total comments: 21
Patch Set 18 : Rework failure and closing cases. #
Total comments: 3
Patch Set 19 : Comment fixes and clarifications #
Total comments: 8
Patch Set 20 : Comment clarifications and formatting tweak #
Total comments: 60
Patch Set 21 : Changes from szym review #
Total comments: 55
Patch Set 22 : Miscellaneous small fixes to unit tests. #Patch Set 23 : One character punctuation fix. #Patch Set 24 : Explain why operator<< needs to be in ::net #
Total comments: 4
Messages
Total messages: 49 (0 generated)
Sorry if this is a repeat--it looks like I didn't send it last time.
https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:28: const size_t kWebSocketErrorNetworkByteLength = 2; It's not always error. And the length of the field is independent from what the byte ordering of the field is. Though it's also used at L31 to define NetworkErrorCode, this constant is mainly used in parsing code to identify close code field. I'd recommend that you name this as kWebSocketCloseCodeLength. https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:298: break; let's call ReadFrames() and return. https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:308: break; let's return https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:321: break; let's return https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:334: frame_header.swap(current_frame_header_); Write the reason of swap. https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:366: scoped_refptr<IOBufferWithSize> old_body_; not member variable. remove last _ https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:402: event_interface_->OnSendFrame(final, opcode, data); why this is On"Send"? https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:425: unsigned short reason; initialize to some default value. https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:504: data.begin()); how about using net::WriteBigEndian? https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:520: if (size < kWebSocketErrorNetworkByteLength) { it works but i'd code this like if (size < kWebSocket...) { if (size != 0) { // error return; } // no code return; } or if (size < kWebSocket...) { if (size == 0) { // no code return; } // error return; } or if (size == 0) { // no code return; } else if (size < kWebSocket...) { // error return; } https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:533: base::NetToHost16(network_code.code_as_network_short); how about using net::ReadBigEndian?
https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:40: const IOBufferWithSize* part2) you don't need a new class. you can just have a utility method that creates a new IOBufferWithSize instance with concatenated contents, I think. https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_m... File net/websockets/websocket_mux.h (right): https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_m... net/websockets/websocket_mux.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2013
https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:145: DCHECK_LE(data.size(), static_cast<size_t>(INT_MAX)); What is this DCHECK_LE? https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:237: if (current_send_quota_ < send_quota_low_water_mark_) { I think this clause should be executed only if send_next_ is null. https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:417: VLOG(1) << "Got Pong of size " << data_buffer->size(); Can you write a TODO comment that describes how to handle a PONG message in the future? https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:526: *reason = 1005; Explaining what 1005 error is in a comment would be helpful. https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... File net/websockets/websocket_channel_test.cc (right): https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel_test.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. I think websocket_channel_unittest.cc would be a better name.
There are also some new unit tests and changes for compatibility with the changes to https://codereview.chromium.org/17610006/ in the latest patch. Sorry about the churn. https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:28: const size_t kWebSocketErrorNetworkByteLength = 2; On 2013/06/26 09:27:06, tyoshino wrote: > It's not always error. And the length of the field is independent from what the > byte ordering of the field is. > > Though it's also used at L31 to define NetworkErrorCode, this constant is mainly > used in parsing code to identify close code field. I'd recommend that you name > this as kWebSocketCloseCodeLength. Done. https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:40: const IOBufferWithSize* part2) On 2013/06/26 09:40:17, tyoshino wrote: > you don't need a new class. you can just have a utility method that creates a > new IOBufferWithSize instance with concatenated contents, I think. I must have been confused when I wrote this. Changed to a utility function. https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:145: DCHECK_LE(data.size(), static_cast<size_t>(INT_MAX)); On 2013/06/26 11:33:29, yhirano wrote: > What is this DCHECK_LE? It will cause a debug build to abort if the frame is larger than INT_MAX, which would definitely mean there was a bug somewhere. Actually, the largest single frame we can send without overflowing IOBufferWithSize is probably INT-MAX-14, but it depends on the protocol in use, so the final enforcement will have to happen in WebSocket*Stream::WriteFrames(). https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:237: if (current_send_quota_ < send_quota_low_water_mark_) { On 2013/06/26 11:33:29, yhirano wrote: > I think this clause should be executed only if send_next_ is null. You're probably right. I have moved it into the if statement below. I am concerned that this may lead to lockstep behaviour, ie. if the renderer is able to send out frames faster than the browser can issue writes, then the renderer will never get a quota refresh until its quota reaches zero and the browser catches up. This might cause poor parallelism causing poor throughput. But it is probably better to wait until we can do real experiments instead of speculating about it now. https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:298: break; On 2013/06/26 09:27:06, tyoshino wrote: > let's call ReadFrames() and return. Done. https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:308: break; On 2013/06/26 09:27:06, tyoshino wrote: > let's return Done. https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:321: break; On 2013/06/26 09:27:06, tyoshino wrote: > let's return Done. https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:334: frame_header.swap(current_frame_header_); On 2013/06/26 09:27:06, tyoshino wrote: > Write the reason of swap. I added a comment. I hope it helps. https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:366: scoped_refptr<IOBufferWithSize> old_body_; On 2013/06/26 09:27:06, tyoshino wrote: > not member variable. remove last _ I have no idea why I put that _ there. Removed. https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:402: event_interface_->OnSendFrame(final, opcode, data); On 2013/06/26 09:27:06, tyoshino wrote: > why this is On"Send"? Having received the message, we will send the IPC WebSocketMsg_SendFrame to pass it to the renderer. Maybe OnDataFrame would be a better name? https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:417: VLOG(1) << "Got Pong of size " << data_buffer->size(); On 2013/06/26 11:33:29, yhirano wrote: > Can you write a TODO comment that describes how to handle a PONG message in the > future? We don't need to anything with pong messages. https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:425: unsigned short reason; On 2013/06/26 09:27:06, tyoshino wrote: > initialize to some default value. Done. https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:504: data.begin()); On 2013/06/26 09:27:06, tyoshino wrote: > how about using net::WriteBigEndian? Done. https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:520: if (size < kWebSocketErrorNetworkByteLength) { On 2013/06/26 09:27:06, tyoshino wrote: > it works but i'd code this like > > if (size < kWebSocket...) { > if (size != 0) { > // error > return; > } > // no code > return; > } > > or > > if (size < kWebSocket...) { > if (size == 0) { > // no code > return; > } > // error > return; > } > > or > > if (size == 0) { > // no code > return; > } else if (size < kWebSocket...) { > // error > return; > } Done. https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:526: *reason = 1005; On 2013/06/26 11:33:29, yhirano wrote: > Explaining what 1005 error is in a comment would be helpful. I'm not sure why I wrote 1005 instead of just using the value from the WebSocketError enum. Perhaps I wrote this code first? I changed it to use the enum value, which should be clear enough. https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:533: base::NetToHost16(network_code.code_as_network_short); On 2013/06/26 09:27:06, tyoshino wrote: > how about using net::ReadBigEndian? Done. https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... File net/websockets/websocket_channel_test.cc (right): https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel_test.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2013/06/26 11:33:29, yhirano wrote: > I think websocket_channel_unittest.cc would be a better name. The style guide says "_unittest and _regtest are deprecated." I wasn't sure whether to follow that rule or be consistent with the other files under net/websockets. It does appear that newer tests under net/ are moving to the _test.cc standard.
https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:145: DCHECK_LE(data.size(), static_cast<size_t>(INT_MAX)); On 2013/06/27 17:29:49, Adam Rice wrote: > On 2013/06/26 11:33:29, yhirano wrote: > > What is this DCHECK_LE? > > It will cause a debug build to abort if the frame is larger than INT_MAX, which > would definitely mean there was a bug somewhere. > > Actually, the largest single frame we can send without overflowing > IOBufferWithSize is probably INT-MAX-14, but it depends on the protocol in use, > so the final enforcement will have to happen in WebSocket*Stream::WriteFrames(). How about NOTREACHED? https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... File net/websockets/websocket_channel_test.cc (right): https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel_test.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2013/06/27 17:29:49, Adam Rice wrote: > On 2013/06/26 11:33:29, yhirano wrote: > > I think websocket_channel_unittest.cc would be a better name. > > The style guide says "_unittest and _regtest are deprecated." > > I wasn't sure whether to follow that rule or be consistent with the other files > under net/websockets. It does appear that newer tests under net/ are moving to > the _test.cc standard. I see, thank you! https://codereview.chromium.org/12764006/diff/15001/net/websockets/websocket_... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/15001/net/websockets/websocket_... net/websockets/websocket_channel.cc:53: SendBuffer() : frames(), total_bytes(0) {} nit: You don't need frames(). https://codereview.chromium.org/12764006/diff/15001/net/websockets/websocket_... net/websockets/websocket_channel.cc:92: incomplete_control_frame_body_(), nit: You don't need call a constructor if it doesn't take arguments. https://codereview.chromium.org/12764006/diff/15001/net/websockets/websocket_... net/websockets/websocket_channel.cc:317: scoped_ptr<WebSocketFrameChunk> chunk(read_frame_chunks_[i]); What happens when ProcessFrameChunk fails (i.e. FailChannel is called) and there are more chunks to be processed? Assume the channel is brand-new and read_frame_chunks_ is [chunk1(masked, with header), chunk2 (without header)]. ProcessFrameChunk(chunk1) will fail and ProcessFrameChunk(chunk2) will be called after that. Is it OK? At least, current_frame_header_ will be null after ProcessFrameChunk(chunk1) and it will cause SEGV in ProcessFrameChunk(chunk2), I think. https://codereview.chromium.org/12764006/diff/15001/net/websockets/websocket_... net/websockets/websocket_channel.cc:374: } optional: DCHECK(frame_header) here?
https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:402: event_interface_->OnSendFrame(final, opcode, data); On 2013/06/27 17:29:49, Adam Rice wrote: > On 2013/06/26 09:27:06, tyoshino wrote: > > why this is On"Send"? > > Having received the message, we will send the IPC WebSocketMsg_SendFrame to pass > it to the renderer. Maybe OnDataFrame would be a better name? Yes. It's so common convention that OnSomething() naming means a handler for Something event. OnDataFrame sounds good. https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... File net/websockets/websocket_channel_test.cc (right): https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel_test.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2013/06/27 17:29:49, Adam Rice wrote: > On 2013/06/26 11:33:29, yhirano wrote: > > I think websocket_channel_unittest.cc would be a better name. > > The style guide says "_unittest and _regtest are deprecated." > > I wasn't sure whether to follow that rule or be consistent with the other files > under net/websockets. It does appear that newer tests under net/ are moving to > the _test.cc standard. I don't have strong opinion. Both make sense. Shall we go with _test.cc? If there's strong policy under net/, you'll get some comment when you get review on gyp file by a net/ OWNER. https://codereview.chromium.org/12764006/diff/15001/net/websockets/websocket_... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/15001/net/websockets/websocket_... net/websockets/websocket_channel.cc:165: if (data.size() > INT_MAX) { > -> >= ? what is this check for? https://codereview.chromium.org/12764006/diff/15001/net/websockets/websocket_... net/websockets/websocket_channel.cc:182: typedef std::vector<char>::size_type size_type; don't worry about size_type being different from size_t. please just use size_t. https://codereview.chromium.org/12764006/diff/15001/net/websockets/websocket_... File net/websockets/websocket_channel.h (right): https://codereview.chromium.org/12764006/diff/15001/net/websockets/websocket_... net/websockets/websocket_channel.h:178: // The current amount of quota that the renderer has available for sending is this really "renderer"? not the corresponding WebSocket instance or just this (logical) channel?
https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:145: DCHECK_LE(data.size(), static_cast<size_t>(INT_MAX)); On 2013/06/28 06:49:17, yhirano wrote: > On 2013/06/27 17:29:49, Adam Rice wrote: > > On 2013/06/26 11:33:29, yhirano wrote: > > > What is this DCHECK_LE? > > > > It will cause a debug build to abort if the frame is larger than INT_MAX, > which > > would definitely mean there was a bug somewhere. > > > > Actually, the largest single frame we can send without overflowing > > IOBufferWithSize is probably INT-MAX-14, but it depends on the protocol in > use, > > so the final enforcement will have to happen in > WebSocket*Stream::WriteFrames(). > > How about NOTREACHED? That is much better, thank you. https://codereview.chromium.org/12764006/diff/15001/net/websockets/websocket_... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/15001/net/websockets/websocket_... net/websockets/websocket_channel.cc:53: SendBuffer() : frames(), total_bytes(0) {} On 2013/06/28 06:49:17, yhirano wrote: > nit: You don't need frames(). I have a habit of initialising all the member varables in the constructor, since for built-in types it makes a difference. For example I have seen cases where there is a scoped_ptr<C> member then later someone changes it to be a C* but forgets to update the constructor and the code becomes unstable. However, this habit seems to worry code reviewers a lot so I think I will stop doing it. https://codereview.chromium.org/12764006/diff/15001/net/websockets/websocket_... net/websockets/websocket_channel.cc:92: incomplete_control_frame_body_(), On 2013/06/28 06:49:17, yhirano wrote: > nit: You don't need call a constructor if it doesn't take arguments. I removed the redundant initialisers. See my comment above. https://codereview.chromium.org/12764006/diff/15001/net/websockets/websocket_... net/websockets/websocket_channel.cc:317: scoped_ptr<WebSocketFrameChunk> chunk(read_frame_chunks_[i]); On 2013/06/28 06:49:17, yhirano wrote: > What happens when ProcessFrameChunk fails (i.e. FailChannel is called) and there > are more chunks to be processed? > > Assume the channel is brand-new and read_frame_chunks_ is [chunk1(masked, with > header), chunk2 (without header)]. > ProcessFrameChunk(chunk1) will fail and ProcessFrameChunk(chunk2) will be called > after that. Is it OK? > At least, current_frame_header_ will be null after ProcessFrameChunk(chunk1) and > it will cause SEGV in ProcessFrameChunk(chunk2), I think. It is important that is doesn't make any difference whether two chunks arrive in one read or in two reads. So ProcessFrameChunk() needs to be able to handle frames in every possible case anyway. I tried to make ProcessFrameChunk() robust in all cases but I missed the one that you mentioned. I have added a check for it now, and I will add a test in a separate CL. I looked for more cases that ProcessFrameChunk() isn't handling but I wasn't able to find any. Please let me know if you spot one. https://codereview.chromium.org/12764006/diff/15001/net/websockets/websocket_... net/websockets/websocket_channel.cc:374: } On 2013/06/28 06:49:17, yhirano wrote: > optional: DCHECK(frame_header) here? I made it return instead, since this can legitimately happen if the previous frame was invalid.
https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/5001/net/websockets/websocket_c... net/websockets/websocket_channel.cc:402: event_interface_->OnSendFrame(final, opcode, data); On 2013/06/28 07:55:27, tyoshino wrote: > On 2013/06/27 17:29:49, Adam Rice wrote: > > On 2013/06/26 09:27:06, tyoshino wrote: > > > why this is On"Send"? > > > > Having received the message, we will send the IPC WebSocketMsg_SendFrame to > pass > > it to the renderer. Maybe OnDataFrame would be a better name? > > Yes. It's so common convention that OnSomething() naming means a handler for > Something event. OnDataFrame sounds good. Done. https://codereview.chromium.org/12764006/diff/15001/net/websockets/websocket_... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/15001/net/websockets/websocket_... net/websockets/websocket_channel.cc:165: if (data.size() > INT_MAX) { On 2013/06/28 07:55:27, tyoshino wrote: > > -> >= ? > > what is this check for? Mostly just paranoia. The networking code uses int for buffer sizes in lots of places and I wanted to avoid any risk of overflow right from the start. https://codereview.chromium.org/12764006/diff/15001/net/websockets/websocket_... net/websockets/websocket_channel.cc:182: typedef std::vector<char>::size_type size_type; On 2013/06/28 07:55:27, tyoshino wrote: > don't worry about size_type being different from size_t. please just use size_t. Done. https://codereview.chromium.org/12764006/diff/15001/net/websockets/websocket_... File net/websockets/websocket_channel.h (right): https://codereview.chromium.org/12764006/diff/15001/net/websockets/websocket_... net/websockets/websocket_channel.h:178: // The current amount of quota that the renderer has available for sending On 2013/06/28 07:55:27, tyoshino wrote: > is this really "renderer"? not the corresponding WebSocket instance or just this > (logical) channel? It is the logical channel as you thought. I have changed the comment a bit. PTAL.
https://codereview.chromium.org/12764006/diff/25002/net/websockets/websocket_... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/25002/net/websockets/websocket_... net/websockets/websocket_channel.cc:157: void WebSocketChannel::SendFrame(bool fin, Shorter function is good. But for this case, precondition checkings should be in the same function for which it is, I think. Also, we should give more descriptive name for SendFrame and Send. Please just merge Send int SendFrame. https://codereview.chromium.org/12764006/diff/25002/net/websockets/websocket_... net/websockets/websocket_channel.cc:174: } how about handling other unexpected states, too? i don't see reason to return for the three states above but leave others go in release build. https://codereview.chromium.org/12764006/diff/25002/net/websockets/websocket_... net/websockets/websocket_channel.cc:178: if (data.size() > static_cast<size_type>(current_send_quota_)) { just size_t. please add a comment in .h that SendFrame may fail for send quota violation so the caller need to check remaining send quota before calling this. is this intended design? https://codereview.chromium.org/12764006/diff/25002/net/websockets/websocket_... net/websockets/websocket_channel.cc:356: frame_header.swap(current_frame_header_); thanks for adding comment and being careful about efficiency. but it's hard to read. l356 and l360 make me get thoughtful. it seems some code can be simplified by just using const WebSocketFrameHeader* instead of swap. could you please try?
https://codereview.chromium.org/12764006/diff/15001/net/websockets/websocket_... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/15001/net/websockets/websocket_... net/websockets/websocket_channel.cc:165: if (data.size() > INT_MAX) { On 2013/06/28 08:28:06, Adam Rice wrote: > On 2013/06/28 07:55:27, tyoshino wrote: > > > -> >= ? > > > > what is this check for? > > Mostly just paranoia. The networking code uses int for buffer sizes in lots of > places and I wanted to avoid any risk of overflow right from the start. ok. it's fine for now. please write a comment that this method accepts only data not bigger than INT_MAX and some TODO to support big message by doing fragmentation at higher layer.
lgtm https://codereview.chromium.org/12764006/diff/36001/net/websockets/websocket_... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/36001/net/websockets/websocket_... net/websockets/websocket_channel.cc:475: unsigned short reason = kWebSocketNormalClosure; nit: We decided that we would use code and reason rather than reason and reason_text. https://codereview.chromium.org/12730003/diff/15001/content/common/websocket_... https://codereview.chromium.org/12764006/diff/36001/net/websockets/websocket_... net/websockets/websocket_channel.cc:526: if (state_ == SEND_CLOSED || state_ == CLOSED) { Ditto https://codereview.chromium.org/12764006/diff/36001/net/websockets/websocket_... net/websockets/websocket_channel.cc:575: const char* data = const_cast<IOBufferWithSize&>(buffer).data(); Ditto https://codereview.chromium.org/12764006/diff/36001/net/websockets/websocket_... File net/websockets/websocket_channel.h (right): https://codereview.chromium.org/12764006/diff/36001/net/websockets/websocket_... net/websockets/websocket_channel.h:50: void SendDropChannel(unsigned short reason, const std::string& reason_text); nit: We decided that we would use code and reason rather than reason and reason_text. https://codereview.chromium.org/12730003/diff/15001/content/common/websocket_... https://codereview.chromium.org/12764006/diff/36001/net/websockets/websocket_... net/websockets/websocket_channel.h:139: void ParseClose(const IOBufferWithSize& buffer, Ditto
https://codereview.chromium.org/12764006/diff/25002/net/websockets/websocket_... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/25002/net/websockets/websocket_... net/websockets/websocket_channel.cc:157: void WebSocketChannel::SendFrame(bool fin, On 2013/06/28 08:28:39, tyoshino wrote: > Shorter function is good. But for this case, precondition checkings should be in > the same function for which it is, I think. Also, we should give more > descriptive name for SendFrame and Send. Please just merge Send int SendFrame. Send() was originally also used by SendClose() but I modified SendClose() not to use it any more and removed Send(). https://codereview.chromium.org/12764006/diff/25002/net/websockets/websocket_... net/websockets/websocket_channel.cc:174: } On 2013/06/28 08:28:39, tyoshino wrote: > how about handling other unexpected states, too? i don't see reason to return > for the three states above but leave others go in release build. Done. https://codereview.chromium.org/12764006/diff/25002/net/websockets/websocket_... net/websockets/websocket_channel.cc:178: if (data.size() > static_cast<size_type>(current_send_quota_)) { On 2013/06/28 08:28:39, tyoshino wrote: > just size_t. > > please add a comment in .h that SendFrame may fail for send quota violation so > the caller need to check remaining send quota before calling this. is this > intended design? Yes. The renderer must obey our quota restrictions to avoid holding lots of browser memory. https://codereview.chromium.org/12764006/diff/25002/net/websockets/websocket_... net/websockets/websocket_channel.cc:356: frame_header.swap(current_frame_header_); On 2013/06/28 08:28:39, tyoshino wrote: > thanks for adding comment and being careful about efficiency. but it's hard to > read. l356 and l360 make me get thoughtful. > > it seems some code can be simplified by just using const WebSocketFrameHeader* > instead of swap. could you please try? I removed the frame_header variable completely and I think it is clearer now. PTAL.
https://codereview.chromium.org/12764006/diff/36001/net/websockets/websocket_... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/36001/net/websockets/websocket_... net/websockets/websocket_channel.cc:475: unsigned short reason = kWebSocketNormalClosure; On 2013/06/28 10:52:33, yhirano wrote: > nit: We decided that we would use code and reason rather than reason and > reason_text. > https://codereview.chromium.org/12730003/diff/15001/content/common/websocket_... You are right. I had completely forgotten. Sorry. I have fixed it everywhere now. https://codereview.chromium.org/12764006/diff/36001/net/websockets/websocket_... net/websockets/websocket_channel.cc:526: if (state_ == SEND_CLOSED || state_ == CLOSED) { On 2013/06/28 10:52:33, yhirano wrote: > Ditto Done. https://codereview.chromium.org/12764006/diff/36001/net/websockets/websocket_... net/websockets/websocket_channel.cc:575: const char* data = const_cast<IOBufferWithSize&>(buffer).data(); On 2013/06/28 10:52:33, yhirano wrote: > Ditto Done. https://codereview.chromium.org/12764006/diff/36001/net/websockets/websocket_... File net/websockets/websocket_channel.h (right): https://codereview.chromium.org/12764006/diff/36001/net/websockets/websocket_... net/websockets/websocket_channel.h:50: void SendDropChannel(unsigned short reason, const std::string& reason_text); On 2013/06/28 10:52:33, yhirano wrote: > nit: We decided that we would use code and reason rather than reason and > reason_text. > https://codereview.chromium.org/12730003/diff/15001/content/common/websocket_... Done. https://codereview.chromium.org/12764006/diff/36001/net/websockets/websocket_... net/websockets/websocket_channel.h:139: void ParseClose(const IOBufferWithSize& buffer, On 2013/06/28 10:52:33, yhirano wrote: > Ditto Done.
https://codereview.chromium.org/12764006/diff/15001/net/websockets/websocket_... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/15001/net/websockets/websocket_... net/websockets/websocket_channel.cc:165: if (data.size() > INT_MAX) { On 2013/06/28 08:30:51, tyoshino wrote: > On 2013/06/28 08:28:06, Adam Rice wrote: > > On 2013/06/28 07:55:27, tyoshino wrote: > > > > -> >= ? > > > > > > what is this check for? > > > > Mostly just paranoia. The networking code uses int for buffer sizes in lots of > > places and I wanted to avoid any risk of overflow right from the start. > > ok. it's fine for now. please write a comment that this method accepts only data > not bigger than INT_MAX and some TODO to support big message by doing > fragmentation at higher layer. I updated the comment.
https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.cc:282: // This use if base::Unretained is safe because we own the WebSocketStream, if -> of? https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... File net/websockets/websocket_channel.h (right): https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.h:100: // Call WebSocketStream::WriteFrames() with the appropriate arguments Calls http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Comments https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.h:106: // Call WebSocketStream::ReadFrames() with the appropriate arguments ditto https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.h:112: // Process a single chunk that has been read from the stream. ditto https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.h:130: // Send a close frame to Start the WebSocket Closing Handshake, or to respond ditto https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.h:134: // Parse a Close frame. If no status code is supplied, then |code| is set to ditto https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.h:156: scoped_ptr<SendBuffer> currently_sending_; can you name this using some noun? for example - pending_send_data_ - data_being_sent_ https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.h:159: scoped_ptr<SendBuffer> send_next_; how about putting a blank line here to group members for send op and receive op? https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.h:159: scoped_ptr<SendBuffer> send_next_; ditto (naming)
https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... File net/websockets/websocket_channel.h (right): https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.h:92: class ConnectDelegate; any reason not to make WebSocketChannel itself derive from WebSocketStream::ConnectDelegate? if so, please write that here. https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.h:162: // Frame header for the current frame. Only non-NULL if we have had to current frame -> the frame currently received https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.h:180: int current_send_quota_; how about putting a blank line here since the members below are not operation specific.
https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.cc:134: event_interface_->OnAddChannelResponse(false, stream_->GetSubProtocol()); what action are the implementors of this interface allowed to take? they never call back to this instance from this (OnAddChannelResponse()) method? basically i think it's better that we finish update of variables before calling this. i.e. state_ = CONNECTED, etc. but it depends on semantics of this interface you give.
https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.cc:134: event_interface_->OnAddChannelResponse(false, stream_->GetSubProtocol()); On 2013/07/01 05:28:34, tyoshino wrote: > what action are the implementors of this interface allowed to take? they never > call back to this instance from this (OnAddChannelResponse()) method? > > basically i think it's better that we finish update of variables before calling > this. i.e. state_ = CONNECTED, etc. but it depends on semantics of this > interface you give. OnAddChannelResponse(true, ...) ie. "Connection Failed" is permitted to delete the WebSocketChannel object. OnDropChannel() should probably also permit deleting the WebSocketChannel object, except that with the current implementation that will break RFC6455 semantics, because it will cause the browser to close the connection without waiting for a server response. I haven't decided what to do about that yet. For testing purposes it may be useful to call WebSocketChannel methods re-entrantly, so I have reordered the code in this and a few other places to make that safer. https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.cc:282: // This use if base::Unretained is safe because we own the WebSocketStream, On 2013/07/01 05:05:18, tyoshino wrote: > if -> of? Sorry, yes. Fixed. https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... File net/websockets/websocket_channel.h (right): https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.h:92: class ConnectDelegate; On 2013/07/01 05:15:29, tyoshino wrote: > any reason not to make WebSocketChannel itself derive from > WebSocketStream::ConnectDelegate? if so, please write that here. Done. https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.h:100: // Call WebSocketStream::WriteFrames() with the appropriate arguments On 2013/07/01 05:05:18, tyoshino wrote: > Calls > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Comments Done. https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.h:106: // Call WebSocketStream::ReadFrames() with the appropriate arguments On 2013/07/01 05:05:18, tyoshino wrote: > ditto Done. https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.h:112: // Process a single chunk that has been read from the stream. On 2013/07/01 05:05:18, tyoshino wrote: > ditto Done. https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.h:130: // Send a close frame to Start the WebSocket Closing Handshake, or to respond On 2013/07/01 05:05:18, tyoshino wrote: > ditto Done. https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.h:134: // Parse a Close frame. If no status code is supplied, then |code| is set to On 2013/07/01 05:05:18, tyoshino wrote: > ditto Done. https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.h:156: scoped_ptr<SendBuffer> currently_sending_; On 2013/07/01 05:05:18, tyoshino wrote: > can you name this using some noun? > for example > - pending_send_data_ > - data_being_sent_ Done. https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.h:159: scoped_ptr<SendBuffer> send_next_; On 2013/07/01 05:05:18, tyoshino wrote: > how about putting a blank line here to group members for send op and receive op? Done. https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.h:159: scoped_ptr<SendBuffer> send_next_; On 2013/07/01 05:05:18, tyoshino wrote: > ditto (naming) Done. https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.h:162: // Frame header for the current frame. Only non-NULL if we have had to On 2013/07/01 05:15:29, tyoshino wrote: > current frame -> the frame currently received Done. https://codereview.chromium.org/12764006/diff/55001/net/websockets/websocket_... net/websockets/websocket_channel.h:180: int current_send_quota_; On 2013/07/01 05:15:29, tyoshino wrote: > how about putting a blank line here since the members below are not operation > specific. Done.
https://codereview.chromium.org/12764006/diff/61001/net/websockets/websocket_... File net/websockets/websocket_channel_test.cc (right): https://codereview.chromium.org/12764006/diff/61001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:75: // This fake EventInterface is for tests which need an WebSocketEventInterface a WebSocket...? Not sure but [w] is not vowel? https://codereview.chromium.org/12764006/diff/61001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:89: // This fake WebSocketStream is for tests that require a WebSocketStream but are are are -> are https://codereview.chromium.org/12764006/diff/61001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:94: // Construct with empty protocol and extensions. Constructs https://codereview.chromium.org/12764006/diff/61001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:186: const InitFrameChunk (&chunks)[N]) { chunks -> source_chunks https://codereview.chromium.org/12764006/diff/61001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:187: ScopedVector<WebSocketFrameChunk> vector; vector -> result_chunks https://codereview.chromium.org/12764006/diff/61001/net/websockets/websocket_... File net/websockets/websocket_event_interface.h (right): https://codereview.chromium.org/12764006/diff/61001/net/websockets/websocket_... net/websockets/websocket_event_interface.h:29: // Provide more send quota to the renderer process. As these methods are named like OnBlahBlah, it looks like that they're disconnected from actual implementation details, but showing only how to receive events from the caller (WebSocketChannel) and what kind of events will be delivered. So, the comments also should explain for what situation this method will be called. Current comment looks good for implementation. Like L33-L35, some implementation specific comments are sometimes useful. So, You may keep this but start with more general comment and maybe put one blank line between them. https://codereview.chromium.org/12764006/diff/61001/net/websockets/websocket_... File net/websockets/websocket_mux.h (right): https://codereview.chromium.org/12764006/diff/61001/net/websockets/websocket_... net/websockets/websocket_mux.h:22: kWebSocketMuxErrorNewChannelSlotOverFlow = 2008, Overflow? it's ok but found Overflow at L31
https://codereview.chromium.org/12764006/diff/61001/net/websockets/websocket_... File net/websockets/websocket_channel_test.cc (right): https://codereview.chromium.org/12764006/diff/61001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:75: // This fake EventInterface is for tests which need an WebSocketEventInterface On 2013/07/01 08:53:32, tyoshino wrote: > a WebSocket...? Not sure but [w] is not vowel? I wrote "an EventInterface" then changed it to "WebSocketEventInterface" but didn't fix "an". Fixed. https://codereview.chromium.org/12764006/diff/61001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:89: // This fake WebSocketStream is for tests that require a WebSocketStream but are On 2013/07/01 08:53:32, tyoshino wrote: > are are -> are Well spotted! Fixed. https://codereview.chromium.org/12764006/diff/61001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:94: // Construct with empty protocol and extensions. On 2013/07/01 08:53:32, tyoshino wrote: > Constructs Done. https://codereview.chromium.org/12764006/diff/61001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:186: const InitFrameChunk (&chunks)[N]) { On 2013/07/01 08:53:32, tyoshino wrote: > chunks -> source_chunks Done. https://codereview.chromium.org/12764006/diff/61001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:187: ScopedVector<WebSocketFrameChunk> vector; On 2013/07/01 08:53:32, tyoshino wrote: > vector -> result_chunks Done. https://codereview.chromium.org/12764006/diff/61001/net/websockets/websocket_... File net/websockets/websocket_event_interface.h (right): https://codereview.chromium.org/12764006/diff/61001/net/websockets/websocket_... net/websockets/websocket_event_interface.h:29: // Provide more send quota to the renderer process. On 2013/07/01 08:53:32, tyoshino wrote: > As these methods are named like OnBlahBlah, it looks like that they're > disconnected from actual implementation details, but showing only how to receive > events from the caller (WebSocketChannel) and what kind of events will be > delivered. > > So, the comments also should explain for what situation this method will be > called. > > Current comment looks good for implementation. Like L33-L35, some implementation > specific comments are sometimes useful. So, You may keep this but start with > more general comment and maybe put one blank line between them. Updated. PTAL. https://codereview.chromium.org/12764006/diff/61001/net/websockets/websocket_... File net/websockets/websocket_mux.h (right): https://codereview.chromium.org/12764006/diff/61001/net/websockets/websocket_... net/websockets/websocket_mux.h:22: kWebSocketMuxErrorNewChannelSlotOverFlow = 2008, On 2013/07/01 08:53:32, tyoshino wrote: > Overflow? it's ok but found Overflow at L31 I think OverFlow is wrong. I have fixed it.
https://codereview.chromium.org/12764006/diff/62008/net/websockets/websocket_... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/62008/net/websockets/websocket_... net/websockets/websocket_channel.cc:17: #include "net/ssl/ssl_config_service.h" where is this used? https://codereview.chromium.org/12764006/diff/62008/net/websockets/websocket_... net/websockets/websocket_channel.cc:72: // being called. double space https://codereview.chromium.org/12764006/diff/62008/net/websockets/websocket_... net/websockets/websocket_channel.cc:507: DCHECK_EQ(CONNECTED, state_); Do the same as L171? https://codereview.chromium.org/12764006/diff/62008/net/websockets/websocket_... net/websockets/websocket_channel.cc:535: void WebSocketChannel::SendClose(unsigned short code, let's use uint16 https://codereview.chromium.org/12764006/diff/62008/net/websockets/websocket_... File net/websockets/websocket_channel_test.cc (right): https://codereview.chromium.org/12764006/diff/62008/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:62: using ::testing::SetArgPointee; not used
https://codereview.chromium.org/12764006/diff/62008/net/websockets/websocket_... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/62008/net/websockets/websocket_... net/websockets/websocket_channel.cc:17: #include "net/ssl/ssl_config_service.h" On 2013/07/02 03:26:42, tyoshino wrote: > where is this used? Sorry, I am used to depending on iwyu. I have taken out the unneeded include statements. https://codereview.chromium.org/12764006/diff/62008/net/websockets/websocket_... net/websockets/websocket_channel.cc:72: // being called. On 2013/07/02 03:26:42, tyoshino wrote: > double space Done. https://codereview.chromium.org/12764006/diff/62008/net/websockets/websocket_... net/websockets/websocket_channel.cc:507: DCHECK_EQ(CONNECTED, state_); On 2013/07/02 03:26:42, tyoshino wrote: > Do the same as L171? Done. https://codereview.chromium.org/12764006/diff/62008/net/websockets/websocket_... net/websockets/websocket_channel.cc:535: void WebSocketChannel::SendClose(unsigned short code, On 2013/07/02 03:26:42, tyoshino wrote: > let's use uint16 Done. https://codereview.chromium.org/12764006/diff/62008/net/websockets/websocket_... File net/websockets/websocket_channel_test.cc (right): https://codereview.chromium.org/12764006/diff/62008/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:62: using ::testing::SetArgPointee; On 2013/07/02 03:26:42, tyoshino wrote: > not used Removed.
https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:144: void WebSocketChannel::OnConnectFailure(uint16 web_socket_error) { websocket_error https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:235: OnWriteDone(result); let's use loop. recursive call to WriteFrames from OnWriteDone L247 can be source of stack overflow attack. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:288: } ditto (loop) https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:310: case ERR_CONNECTION_CLOSED: { stream_->Close() is unnecessary for this case? https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:340: if (chunk->header) { Check if current_frame_header_ is NULL. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:353: DCHECK(state_ != CONNECTED) << "Unexpected header-less frame received " why checking state_? https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:360: data_buffer.swap(chunk->data); let's save chunk->final_chunk and call chunk.reset() here. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:378: // TODO(ricea): This algorithm is O(N^2). Use a fixed 127-byte byffer buffer you could do similar to SocketStream::pending_write_bufs_ in the future. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:391: L392 to L485 can be factored out into a separate method. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:400: if (state_ == RECV_CLOSED) { l400, l425, l440 can be merged and moved out to L391 https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:511: SendClose(code, reason); shouldn't SendDropChannel just close the stream? SendClose is for core WebSocket protocol's closure https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:533: } set state_ to CLOSED and maybe close stream? https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:569: they_should_both_be_two_bytes); sorry, is this normalization specified in the spec? https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:577: *code = kWebSocketErrorProtocolError; this is confusing. please pass the received code as is or it looks that the peer told us that we made mistake. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:584: swap(*reason, text); is this swap more efficient than substitution? https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... File net/websockets/websocket_channel.h (right): https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.h:75: // We have a simple linear progression of states from CONSTRUCTED to CLOSED, FRESHLY_ https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.h:135: // state. The suppled code and reason are sent back to the renderer; the supplied https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.h:137: // CONNECTED and |expose| is SendInternalError). Resets current_frame_header_ SEND_INTERNAL_ERROR
https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:144: void WebSocketChannel::OnConnectFailure(uint16 web_socket_error) { On 2013/07/02 08:23:01, tyoshino wrote: > websocket_error Done. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:235: OnWriteDone(result); On 2013/07/02 08:23:01, tyoshino wrote: > let's use loop. recursive call to WriteFrames from OnWriteDone L247 can be > source of stack overflow attack. Done. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:288: } On 2013/07/02 08:23:01, tyoshino wrote: > ditto (loop) Done. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:310: case ERR_CONNECTION_CLOSED: { On 2013/07/02 08:23:01, tyoshino wrote: > stream_->Close() is unnecessary for this case? It is probably correct to call Close() here. Which makes this case the same as the default case, so I have combined them. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:340: if (chunk->header) { On 2013/07/02 08:23:01, tyoshino wrote: > Check if current_frame_header_ is NULL. Done. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:353: DCHECK(state_ != CONNECTED) << "Unexpected header-less frame received " On 2013/07/02 08:23:01, tyoshino wrote: > why checking state_? If the header was invalid, then we will have called FailChannel() which sets current_frame_header_ to NULL and state_ to SEND_CLOSED or CLOSED. On the other hand, if the state_ is CONNECTED, then we know that FailChannel() was not called and so it must be a bug on our side. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:360: data_buffer.swap(chunk->data); On 2013/07/02 08:23:01, tyoshino wrote: > let's save chunk->final_chunk and call chunk.reset() here. Done. I thought about splitting the method here, but I don't think it will actually help readability. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:378: // TODO(ricea): This algorithm is O(N^2). Use a fixed 127-byte byffer On 2013/07/02 08:23:01, tyoshino wrote: > buffer > > you could do similar to SocketStream::pending_write_bufs_ in the future. Done. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:391: On 2013/07/02 08:23:01, tyoshino wrote: > L392 to L485 can be factored out into a separate method. Done. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:400: if (state_ == RECV_CLOSED) { On 2013/07/02 08:23:01, tyoshino wrote: > l400, l425, l440 can be merged and moved out to L391 Well-spotted. I did not notice that. Done. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:511: SendClose(code, reason); On 2013/07/02 08:23:01, tyoshino wrote: > shouldn't SendDropChannel just close the stream? SendClose is for core WebSocket > protocol's closure I'm not sure I get your point. SendDropChannel() is the public interface. It is what will be called if someone does ws.close() from Javascript. So http://dev.w3.org/html5/websockets/#dom-websocket-close applies, ie. we should start the closing handshake by sending a Close frame. SendClose() is the internal method that actually sends a Close frame. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:533: } On 2013/07/02 08:23:01, tyoshino wrote: > set state_ to CLOSED and maybe close stream? I don't think I should be calling OnDropChannel here. Looking at the DOM WebSocket spec, it says "When the WebSocket closing handshake is started, the user agent must queue a task to change the readyState attribute's value to CLOSING (2).". Which means I need to add an extra message, distinct from OnDropChannel, to trigger that. I think I need to split the Fail cases into those that should close the connection immediately from the client side (for example, errors during the closing handshake, or timeouts), and those that attempt to perform The WebSocket Closing Handshake. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:569: they_should_both_be_two_bytes); On 2013/07/02 08:23:01, tyoshino wrote: > sorry, is this normalization specified in the spec? It's a bit ambiguous, to be honest. The clearest language is in section 11.7, where it says "The status code is an integer number between 1000 and 4999 (inclusive)." https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:577: *code = kWebSocketErrorProtocolError; On 2013/07/02 08:23:01, tyoshino wrote: > this is confusing. please pass the received code as is or it looks that the peer > told us that we made mistake. I agree that it is confusing, but I think it is the least worst option. Previously I was using 1008 ("an endpoint is terminating the connection because it has received a message that violates its policy"), but I think that was worse. I don't want to pass through garbage status codes like 65535, because then every other bit of code that handles status codes would have to be able to be able to correctly handle them. It is safer to have the validation code in one place. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:584: swap(*reason, text); On 2013/07/02 08:23:01, tyoshino wrote: > is this swap more efficient than substitution? I'm not sure what you mean by a substitution. It is more efficient than a copy when the string is heap allocated, and equivalent otherwise. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... File net/websockets/websocket_channel.h (right): https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.h:75: // We have a simple linear progression of states from CONSTRUCTED to CLOSED, On 2013/07/02 08:23:01, tyoshino wrote: > FRESHLY_ Done. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.h:135: // state. The suppled code and reason are sent back to the renderer; the On 2013/07/02 08:23:01, tyoshino wrote: > supplied Done. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.h:137: // CONNECTED and |expose| is SendInternalError). Resets current_frame_header_ On 2013/07/02 08:23:01, tyoshino wrote: > SEND_INTERNAL_ERROR Done.
https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:353: DCHECK(state_ != CONNECTED) << "Unexpected header-less frame received " On 2013/07/02 13:49:29, Adam Rice wrote: > On 2013/07/02 08:23:01, tyoshino wrote: > > why checking state_? > > If the header was invalid, then we will have called FailChannel() which sets > current_frame_header_ to NULL and state_ to SEND_CLOSED or CLOSED. > > On the other hand, if the state_ is CONNECTED, then we know that FailChannel() > was not called and so it must be a bug on our side. I see. Thanks for adding the comment. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:360: data_buffer.swap(chunk->data); On 2013/07/02 13:49:29, Adam Rice wrote: > On 2013/07/02 08:23:01, tyoshino wrote: > > let's save chunk->final_chunk and call chunk.reset() here. > > Done. > > I thought about splitting the method here, but I don't think it will actually > help readability. ok https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:511: SendClose(code, reason); On 2013/07/02 13:49:29, Adam Rice wrote: > On 2013/07/02 08:23:01, tyoshino wrote: > > shouldn't SendDropChannel just close the stream? SendClose is for core > WebSocket > > protocol's closure > > I'm not sure I get your point. SendDropChannel() is the public interface. It is > what will be called if someone does ws.close() from Javascript. So > http://dev.w3.org/html5/websockets/#dom-websocket-close applies, ie. we should > start the closing handshake by sending a Close frame. SendClose() is the > internal method that actually sends a Close frame. I see. So, for ws.close(), this code looks good. But the comment in .h and the method name say this sends DropChannel. As we don't have mux yet, it should temporarily mean "just close" for now. Can we give it different name? E.g. StartClosingHandshake. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:533: } On 2013/07/02 13:49:29, Adam Rice wrote: > On 2013/07/02 08:23:01, tyoshino wrote: > > set state_ to CLOSED and maybe close stream? > > I don't think I should be calling OnDropChannel here. > > Looking at the DOM WebSocket spec, it says "When the WebSocket closing handshake > is started, the user agent must queue a task to change the readyState > attribute's value to CLOSING (2).". Which means I need to add an extra message, > distinct from OnDropChannel, to trigger that. > See also http://tools.ietf.org/html/rfc6455#section-7.1.7 Because of the reason similar to what you say by L528, we don't have to wait for response like normal closing > I think I need to split the Fail cases into those that should close the > connection immediately from the client side (for example, errors during the Right. > closing handshake, or timeouts), and those that attempt to perform The WebSocket > Closing Handshake. Oh. Which use case? https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:569: they_should_both_be_two_bytes); On 2013/07/02 13:49:29, Adam Rice wrote: > On 2013/07/02 08:23:01, tyoshino wrote: > > sorry, is this normalization specified in the spec? > > It's a bit ambiguous, to be honest. The clearest language is in section 11.7, > where it says "The status code is an integer number between 1000 and 4999 > (inclusive)." That makes some sense, but not to confuse users, we need some variable by which we can tell the user of this class that some normalization happened. Currently, Chrome and Firefox Aurora both pass received code as-is to the app. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:577: *code = kWebSocketErrorProtocolError; On 2013/07/02 13:49:29, Adam Rice wrote: > On 2013/07/02 08:23:01, tyoshino wrote: > > this is confusing. please pass the received code as is or it looks that the > peer > > told us that we made mistake. > > I agree that it is confusing, but I think it is the least worst option. > > Previously I was using 1008 ("an endpoint is terminating the connection because > it has received a message that violates its policy"), but I think that was > worse. > > I don't want to pass through garbage status codes like 65535, because then every > other bit of code that handles status codes would have to be able to be able to > correctly handle them. It is safer to have the validation code in one place. I understand, but we must use some code that can tell us that the peer endpoint is to be blamed, but we don't have such a code. If you really want to do some check, that shouldn't normalization but validation and I'd like you to report 1006 and wasClean=false to the application layer. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:584: swap(*reason, text); On 2013/07/02 13:49:29, Adam Rice wrote: > On 2013/07/02 08:23:01, tyoshino wrote: > > is this swap more efficient than substitution? > > I'm not sure what you mean by a substitution. It is more efficient than a copy > when the string is heap allocated, and equivalent otherwise. Thanks. Never mind. https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... net/websockets/websocket_channel.cc:359: // if this happens when state_ is CONNECTED, it is a definitely a bug. remove first a?
https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:511: SendClose(code, reason); On 2013/07/02 16:34:19, tyoshino wrote: > On 2013/07/02 13:49:29, Adam Rice wrote: > > On 2013/07/02 08:23:01, tyoshino wrote: > > > shouldn't SendDropChannel just close the stream? SendClose is for core > > WebSocket > > > protocol's closure > > > > I'm not sure I get your point. SendDropChannel() is the public interface. It > is > > what will be called if someone does ws.close() from Javascript. So > > http://dev.w3.org/html5/websockets/#dom-websocket-close applies, ie. we should > > start the closing handshake by sending a Close frame. SendClose() is the > > internal method that actually sends a Close frame. > > I see. So, for ws.close(), this code looks good. But the comment in .h and the > method name say this sends DropChannel. As we don't have mux yet, it should > temporarily mean "just close" for now. > > Can we give it different name? E.g. StartClosingHandshake. Done. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:533: } On 2013/07/02 16:34:19, tyoshino wrote: > On 2013/07/02 13:49:29, Adam Rice wrote: > > On 2013/07/02 08:23:01, tyoshino wrote: > > > set state_ to CLOSED and maybe close stream? > > > > I don't think I should be calling OnDropChannel here. > > > > Looking at the DOM WebSocket spec, it says "When the WebSocket closing > handshake > > is started, the user agent must queue a task to change the readyState > > attribute's value to CLOSING (2).". Which means I need to add an extra > message, > > distinct from OnDropChannel, to trigger that. > > > > See also http://tools.ietf.org/html/rfc6455#section-7.1.7 > Because of the reason similar to what you say by L528, we don't have to wait for > response like normal closing > > > I think I need to split the Fail cases into those that should close the > > connection immediately from the client side (for example, errors during the > > Right. > > > closing handshake, or timeouts), and those that attempt to perform The > WebSocket > > Closing Handshake. > > Oh. Which use case? Oh. I think I misinterpreted the RFC. I was thinking that when the client has to _Fail the WebSocket Connection_, then it would normally _Start the WebSocket Closing Handshake_ and wait for the server to send a Close frame back and close the connection. But now I've read it twice more, I think the client should send a Close frame (if it thinks the socket is still alive), and then immediately close the TCP/IP connection. I'm not sure how that interacts with multiplexing but I will worry about that later. So the two Fail cases are: 1) Send Close frame and disconnect, and 2) Just disconnect. There is no case where we wait for a server response after Failing. https://codereview.chromium.org/12764006/diff/79001/net/websockets/websocket_... net/websockets/websocket_channel.cc:577: *code = kWebSocketErrorProtocolError; On 2013/07/02 16:34:19, tyoshino wrote: > On 2013/07/02 13:49:29, Adam Rice wrote: > > On 2013/07/02 08:23:01, tyoshino wrote: > > > this is confusing. please pass the received code as is or it looks that the > > peer > > > told us that we made mistake. > > > > I agree that it is confusing, but I think it is the least worst option. > > > > Previously I was using 1008 ("an endpoint is terminating the connection > because > > it has received a message that violates its policy"), but I think that was > > worse. > > > > I don't want to pass through garbage status codes like 65535, because then > every > > other bit of code that handles status codes would have to be able to be able > to > > correctly handle them. It is safer to have the validation code in one place. > > I understand, but we must use some code that can tell us that the peer endpoint > is to be blamed, but we don't have such a code. If you really want to do some > check, that shouldn't normalization but validation and I'd like you to report > 1006 and wasClean=false to the application layer. Okay, that makes sense. However, if all other browsers pass through the code without validation then I would need a concrete security issue to justify being different. https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... net/websockets/websocket_channel.cc:359: // if this happens when state_ is CONNECTED, it is a definitely a bug. On 2013/07/02 16:34:19, tyoshino wrote: > remove first a? Sorry. Done.
https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... File net/websockets/websocket_channel_test.cc (right): https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:202: result_chunk->data = new IOBufferWithSize(strlen(source_chunks[i].data)); why not using payload_length? if you want to use null terminated string, please add a comment about that to L177. https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:233: // and PrepareReadFramesError(). If |async| is true, then ReadFrames() will true -> ASYNC https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:236: // |async| is false, the response will be returned synchronously. |error| is false -> SYNC https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:319: // must run the MessageLoop to receive the callback. comment that ReadFrames never completes unless some WriteFrames occurs after ReadFrames call. https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:323: : stored_frame_chunks_(), read_callback_(), read_frame_chunks_(NULL) {} remove unnecessary initializers? https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:356: void MoveFrameChunks(ScopedVector<WebSocketFrameChunk>* to) { MoveStoredFrameChunks comment that masked is unset. https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:457: virtual scoped_ptr<WebSocketEventInterface> EventInterface() { CreateEventInterface? WebSocketChannelEventInterfaceTest returns non null value only once. Is this intentional? https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:462: // work (bug?). This function works around that and simplifies initialising does PassAs<WebSocketStream> work? https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:518: scoped_ptr<MockWebSocketStream> mock_; mock_stream_
https://codereview.chromium.org/12764006/diff/94001/net/websockets/websocket_... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/94001/net/websockets/websocket_... net/websockets/websocket_channel.cc:417: if (state_ == CLOSED) { o, not state_ == RECV_CLOSED || state_ == CLOSED?
https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... File net/websockets/websocket_channel_test.cc (right): https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:202: result_chunk->data = new IOBufferWithSize(strlen(source_chunks[i].data)); On 2013/07/03 07:27:00, tyoshino wrote: > why not using payload_length? if you want to use null terminated string, please > add a comment about that to L177. I added a comment and tried to clarify the code. I also added support for the NULL data support which the code claimed to implement but never actually did. https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:233: // and PrepareReadFramesError(). If |async| is true, then ReadFrames() will On 2013/07/03 07:27:00, tyoshino wrote: > true -> ASYNC Sorry. As you probably guessed, I changed the type from a bool when I realised I couldn't understand the tests. https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:236: // |async| is false, the response will be returned synchronously. |error| is On 2013/07/03 07:27:00, tyoshino wrote: > false -> SYNC Done. https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:319: // must run the MessageLoop to receive the callback. On 2013/07/03 07:27:00, tyoshino wrote: > comment that ReadFrames never completes unless some WriteFrames occurs after > ReadFrames call. Done. https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:323: : stored_frame_chunks_(), read_callback_(), read_frame_chunks_(NULL) {} On 2013/07/03 07:27:00, tyoshino wrote: > remove unnecessary initializers? Done. https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:356: void MoveFrameChunks(ScopedVector<WebSocketFrameChunk>* to) { On 2013/07/03 07:27:00, tyoshino wrote: > MoveStoredFrameChunks > > comment that masked is unset. Done. https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:457: virtual scoped_ptr<WebSocketEventInterface> EventInterface() { On 2013/07/03 07:27:00, tyoshino wrote: > CreateEventInterface? > > WebSocketChannelEventInterfaceTest returns non null value only once. Is this > intentional? Not exactly intentional, but none of the test cases need more than one mock. I have documented the behaviour. https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:462: // work (bug?). This function works around that and simplifies initialising On 2013/07/03 07:27:00, tyoshino wrote: > does PassAs<WebSocketStream> work? It does. Thank you. But I found that the set_stream() method makes my tests look more elegant, so I kept it. https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:518: scoped_ptr<MockWebSocketStream> mock_; On 2013/07/03 07:27:00, tyoshino wrote: > mock_stream_ Done. https://codereview.chromium.org/12764006/diff/94001/net/websockets/websocket_... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/94001/net/websockets/websocket_... net/websockets/websocket_channel.cc:417: if (state_ == CLOSED) { On 2013/07/03 07:45:44, tyoshino wrote: > o, not state_ == RECV_CLOSED || state_ == CLOSED? state_ cannot be RECV_CLOSED unless OnReadDone() is called recursively from within SendClose(), something I'd prefer not to support. I have added a DCHECK to discourage it.
LGTM https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... File net/websockets/websocket_channel_test.cc (right): https://codereview.chromium.org/12764006/diff/87001/net/websockets/websocket_... net/websockets/websocket_channel_test.cc:202: result_chunk->data = new IOBufferWithSize(strlen(source_chunks[i].data)); On 2013/07/03 10:25:26, Adam Rice wrote: > On 2013/07/03 07:27:00, tyoshino wrote: > > why not using payload_length? if you want to use null terminated string, > please > > add a comment about that to L177. > > I added a comment and tried to clarify the code. I also added support for the > NULL data support which the code claimed to implement but never actually did. Oh, I didn't understand correctly. Thanks https://codereview.chromium.org/12764006/diff/94001/net/websockets/websocket_... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/94001/net/websockets/websocket_... net/websockets/websocket_channel.cc:417: if (state_ == CLOSED) { On 2013/07/03 10:25:26, Adam Rice wrote: > On 2013/07/03 07:45:44, tyoshino wrote: > > o, not state_ == RECV_CLOSED || state_ == CLOSED? > > state_ cannot be RECV_CLOSED unless OnReadDone() is called recursively from > within SendClose(), something I'd prefer not to support. I have added a DCHECK > to discourage it. Got it. Thanks https://codereview.chromium.org/12764006/diff/101001/net/websockets/websocket... File net/websockets/websocket_channel_test.cc (right): https://codereview.chromium.org/12764006/diff/101001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:499: stream_ = stream.template PassAs<WebSocketStream>(); ".template " is required since you templatized this method? https://codereview.chromium.org/12764006/diff/101001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:746: // chunk. This should be delivered as 5 frames. frames -> chunks? https://codereview.chromium.org/12764006/diff/101001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:803: // chunk. This should be delivered as 5 frames. This comment is copied but not edited correctly for this test case?
https://codereview.chromium.org/12764006/diff/101001/net/websockets/websocket... File net/websockets/websocket_channel_test.cc (right): https://codereview.chromium.org/12764006/diff/101001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:499: stream_ = stream.template PassAs<WebSocketStream>(); On 2013/07/04 12:54:26, tyoshino wrote: > ".template " is required since you templatized this method? Yes. "PassAs" could theoretically be a class member for some type T and so the standard requires us to explicitly state that it is a templated method. I have added a comment attempting to explain it. https://codereview.chromium.org/12764006/diff/101001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:746: // chunk. This should be delivered as 5 frames. On 2013/07/04 12:54:26, tyoshino wrote: > frames -> chunks? "frames" is correct here. I have added some extra explanation. https://codereview.chromium.org/12764006/diff/101001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:803: // chunk. This should be delivered as 5 frames. On 2013/07/04 12:54:26, tyoshino wrote: > This comment is copied but not edited correctly for this test case? Sorry. Fixed.
+szym for net review.
Still LGTM patch set 20 https://codereview.chromium.org/12764006/diff/101001/net/websockets/websocket... File net/websockets/websocket_channel_test.cc (right): https://codereview.chromium.org/12764006/diff/101001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:746: // chunk. This should be delivered as 5 frames. On 2013/07/05 06:35:27, Adam Rice wrote: > On 2013/07/04 12:54:26, tyoshino wrote: > > frames -> chunks? > > "frames" is correct here. I have added some extra explanation. OK. Thanks https://codereview.chromium.org/12764006/diff/101001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:803: // chunk. This should be delivered as 5 frames. On 2013/07/05 06:35:27, Adam Rice wrote: > On 2013/07/04 12:54:26, tyoshino wrote: > > This comment is copied but not edited correctly for this test case? > > Sorry. Fixed. Looks good.
Overall looks good (haven't looked at the test yet), but there's a serious risk of use after free, because failures are communicated to OnDropChannel which is allowed to destroy this WebSocketChannel. The easiest solution is to forbid WebSocketEventInterface from destroying the WebSocketChannel from within any callbacks. (It's ok if the destruction is deferred via PostTask.) https://codereview.chromium.org/12764006/diff/110001/net/websockets/README File net/websockets/README (right): https://codereview.chromium.org/12764006/diff/110001/net/websockets/README#ne... net/websockets/README:46: websocket_stream.h Mind adding websocket_stream.cc and websocket_stream_base.h here? My concern is that the list appears complete, but isn't. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:47: ScopedVector<WebSocketFrameChunk> frames; nit: add blank line between methods and fields. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:48: size_t total_bytes; To ensure consistency add an inline method which appends a chunk and increments |total_bytes|. Also suggest hiding total_bytes behind an inline getter. That said, |total_bytes| does not seem to be used, and therefore SendBuffer could just as well be typedef ScopedVector<WebSocketFrameChunk>, in which case using ScopedVector::swap might make more sense than passing it via scoped_ptr. Ignore the point if you have crisp plans for using this variable in the future. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:118: new WebSocketChannel::ConnectDelegate(this)); Remove WebSocketChannel:: https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:152: void WebSocketChannel::SendFrame(bool fin, Keep the order of methods in implementation the same as in the header. (Google code style) https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:218: } nit: {} unnecessary when both condition and body are single line https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:245: DCHECK(state_ != FRESHLY_CONSTRUCTED && state_ != CONNECTING); ditto: split into two DCHECK_NEs https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:303: DCHECK(state_ != FRESHLY_CONSTRUCTED && state_ != CONNECTING); ditto here https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:314: ProcessFrameChunk(chunk.Pass()); BUG: This could be use after free. ProcessFrameChunk... HandleFrame... FailChannel... OnDropChannel, which could delete this. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:375: if (is_final_chunk) { Reorganize this so that the early exit case goes first, that is: if (!is_final_chunk) { if (incomplete...) { } else { } return; // Handle when complete. } if (incomplete...) { concatenate... incomplete_... = NULL; // Frame now complete. } HandleFrame... https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:408: current_frame_header_.reset(); BUG: This could be use after free. HandleFrame... FailChannel... OnDropChannel, which could delete this. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:556: DCHECK(state_ != FRESHLY_CONSTRUCTED && state_ != CONNECTING); I suggest splitting && DCHECK into two so that failures are more informative. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:583: uint64 payload_length = kWebSocketCloseCodeLength + reason.length(); why is this uint64 rather than size_t? https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:591: state_ = state_ == CONNECTED ? SEND_CLOSED : CLOSED; parens around the condition would help readability https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:628: swap(*reason, text); just call std::swap https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... File net/websockets/websocket_channel.h (right): https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.h:27: // interface to the content layer. Add link to the RFC: http://tools.ietf.org/html/rfc6455 so that concepts such as "Close frame" and "Closing Handshake" have reference. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.h:44: // larger than 32KB to avoid blocking the IO thread. This method has a hard Why would this block the IO thread? Do you mean the time spent by the CPU executing the syscalls, or do you imply this call would actually idly block until a network event occurs? https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.h:52: // Sends "quota" units of flow control to the remote side. If the underlying nit: Use pipes, i.e.: Sends |quota| units. Are those units bytes? If so, use "bytes" instead of "units" (or "quota amount") everywhere. If not, explain the relationship somewhere. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.h:69: // default. This is used for testing. Change "a different factory" to "specified factory", and "used for testing" to "exposed for testing." The goal is to remove doubt that this is *only* for testing. I'd also suggest making this private and exposing it only as SendAddChannelRequestForTesting, so that the presubmit check can verify that this is called from tests only. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.h:74: base::Callback<scoped_ptr<WebSocketStreamRequest>( Add a typedef for this Callback type, even if it's used for testing only. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.h:135: // is being called directly from within ReadFrames(). |result| is a net error Since this is also called from a loop within ReadFrames (just a WriteFrames), I suggest keeping the comment parallel to the one above OnWriteDone, or referring this mechanic here. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.h:147: const bool is_final_chunk, remove consts from variables passed by value https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.h:152: // when the current write finishes. What does |fin| do? https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.h:163: // stream_, and sets state_ to CLOSED. Suggest adding a comment that OnDropChannel is allowed to destroy this WebSocketChannel. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.h:167: // to a close frame from the server. Be consistent: "a Close frame". https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.h:182: // The event_interface_ object to call back into. "event_interface_" is unnecessary in the comment. Suggest: // The object receiving events. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.h:220: int current_send_quota_; Why not use an unsigned type for the quotas so that you don't need checked casts? If it's actually possible for it to go negative, then you should not use checked casts without first checking that case. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... File net/websockets/websocket_mux.h (right): https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_mux.h:14: kWebSocketMuxErrorPhysicalConnectionFailed = 2000, Hmm, Chromium code style suggests using MACRO-style enums for consistency, so I'm assuming you have a good reason for all the kWebSocket* enums.
https://codereview.chromium.org/12764006/diff/110001/net/websockets/README File net/websockets/README (right): https://codereview.chromium.org/12764006/diff/110001/net/websockets/README#ne... net/websockets/README:46: websocket_stream.h On 2013/07/09 15:59:51, szym wrote: > Mind adding websocket_stream.cc and websocket_stream_base.h here? > > My concern is that the list appears complete, but isn't. Done. I added a PRESUBMIT check ( https://codereview.chromium.org/18112015/ ) to help us keep this README up-to-date for however long we need it. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:47: ScopedVector<WebSocketFrameChunk> frames; On 2013/07/09 15:59:51, szym wrote: > nit: add blank line between methods and fields. Done. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:48: size_t total_bytes; On 2013/07/09 15:59:51, szym wrote: > To ensure consistency add an inline method which appends a chunk and increments > |total_bytes|. Also suggest hiding total_bytes behind an inline getter. > > That said, |total_bytes| does not seem to be used, and therefore SendBuffer > could just as well be typedef ScopedVector<WebSocketFrameChunk>, in which case > using ScopedVector::swap might make more sense than passing it via scoped_ptr. > Ignore the point if you have crisp plans for using this variable in the future. I changed it into a class. This does make the calling code more concise. I defined AddFrame() out-of-line so the compiler can make the choice of whether to inline it or not. I plan to use total_bytes to measure throughput so that we can adjust quota appropriately. I'm not sure whether I would consider these plans "fresh" as I have quite a lot to do before I can look at optimisations. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:118: new WebSocketChannel::ConnectDelegate(this)); On 2013/07/09 15:59:51, szym wrote: > Remove WebSocketChannel:: Done. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:152: void WebSocketChannel::SendFrame(bool fin, On 2013/07/09 15:59:51, szym wrote: > Keep the order of methods in implementation the same as in the header. (Google > code style) Done. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:218: } On 2013/07/09 15:59:51, szym wrote: > nit: {} unnecessary when both condition and body are single line Done. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:245: DCHECK(state_ != FRESHLY_CONSTRUCTED && state_ != CONNECTING); On 2013/07/09 15:59:51, szym wrote: > ditto: split into two DCHECK_NEs Done. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:303: DCHECK(state_ != FRESHLY_CONSTRUCTED && state_ != CONNECTING); On 2013/07/09 15:59:51, szym wrote: > ditto here Done. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:314: ProcessFrameChunk(chunk.Pass()); On 2013/07/09 15:59:51, szym wrote: > BUG: This could be use after free. > > ProcessFrameChunk... HandleFrame... FailChannel... OnDropChannel, which could > delete this. Thank you for noticing. I was pretty sure I had got it right. Since I'm not confident about this any more, I changed the documentation for OnDropChannel to say it can't delete the object (re-entrantly). Unfortunately, asynchronous deletion is not a cure-all either, as it carries the risk of phantom events from undead WebSocketChannel objects. For OnAddChannelResponse(), I've decided to continue to allow re-entrant delete, but I will add a test to make sure it actually works (in my separate "rest of the unit tests" CL). https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:375: if (is_final_chunk) { On 2013/07/09 15:59:51, szym wrote: > Reorganize this so that the early exit case goes first, that is: > > if (!is_final_chunk) { > if (incomplete...) { > > } else { > > } > return; // Handle when complete. > } > > if (incomplete...) { > concatenate... > incomplete_... = NULL; // Frame now complete. > } > > HandleFrame... Done. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:408: current_frame_header_.reset(); On 2013/07/09 15:59:51, szym wrote: > BUG: This could be use after free. > > HandleFrame... FailChannel... OnDropChannel, which could delete this. Ack. Fixed by disallowing delete from OnDropChannel. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:556: DCHECK(state_ != FRESHLY_CONSTRUCTED && state_ != CONNECTING); On 2013/07/09 15:59:51, szym wrote: > I suggest splitting && DCHECK into two so that failures are more informative. Done. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:583: uint64 payload_length = kWebSocketCloseCodeLength + reason.length(); On 2013/07/09 15:59:51, szym wrote: > why is this uint64 rather than size_t? I have no idea. It's been like that since the first version I checked into git. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:591: state_ = state_ == CONNECTED ? SEND_CLOSED : CLOSED; On 2013/07/09 15:59:51, szym wrote: > parens around the condition would help readability Done. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.cc:628: swap(*reason, text); On 2013/07/09 15:59:51, szym wrote: > just call std::swap I'm not completely sure that the specialisation of swap for std::string is guaranteed to be in the std namespace. So I used the member function instead. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... File net/websockets/websocket_channel.h (right): https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.h:27: // interface to the content layer. On 2013/07/09 15:59:51, szym wrote: > Add link to the RFC: http://tools.ietf.org/html/rfc6455 > so that concepts such as "Close frame" and "Closing Handshake" have reference. Done. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.h:44: // larger than 32KB to avoid blocking the IO thread. This method has a hard On 2013/07/09 15:59:51, szym wrote: > Why would this block the IO thread? Do you mean the time spent by the CPU > executing the syscalls, or do you imply this call would actually idly block > until a network event occurs? I am mostly talking about the memory copy overhead. I'm not sure whether I should also be concerned about blocking the IPC channel. I have changed the wording of the comment to try to make it clear I am not talking about blocking IO or similar. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.h:52: // Sends "quota" units of flow control to the remote side. If the underlying On 2013/07/09 15:59:51, szym wrote: > nit: Use pipes, i.e.: > Sends |quota| units. > > Are those units bytes? If so, use "bytes" instead of "units" (or "quota amount") > everywhere. If not, explain the relationship somewhere. Currently they are implemented as bytes. When we have multiplexing implemented we might have a situation where the method to count quota depends on the type of multiplexing in use (ie. WebSocket multiplexing or SPDY/HTTP2.0). In which case we will have to tell the rendering process in the OnAddChannelResponse() message what kind of quota counting to use for the channel. I have changed the description to "quota units" everywhere and noted that "quota units" currently means bytes in a couple of places. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.h:69: // default. This is used for testing. On 2013/07/09 15:59:51, szym wrote: > Change "a different factory" to "specified factory", and > "used for testing" to "exposed for testing." > The goal is to remove doubt that this is *only* for testing. > > I'd also suggest making this private and exposing it only as > SendAddChannelRequestForTesting, so that the presubmit check can verify that > this is called from tests only. That is better, thank you. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.h:74: base::Callback<scoped_ptr<WebSocketStreamRequest>( On 2013/07/09 15:59:51, szym wrote: > Add a typedef for this Callback type, even if it's used for testing only. Done. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.h:135: // is being called directly from within ReadFrames(). |result| is a net error On 2013/07/09 15:59:51, szym wrote: > Since this is also called from a loop within ReadFrames (just a WriteFrames), I > suggest keeping the comment parallel to the one above OnWriteDone, or referring > this mechanic here. Done. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.h:147: const bool is_final_chunk, On 2013/07/09 15:59:51, szym wrote: > remove consts from variables passed by value Done. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.h:152: // when the current write finishes. On 2013/07/09 15:59:51, szym wrote: > What does |fin| do? I added some explanation to SendFrame(), and the referred back to it from here. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.h:163: // stream_, and sets state_ to CLOSED. On 2013/07/09 15:59:51, szym wrote: > Suggest adding a comment that OnDropChannel is allowed to destroy this > WebSocketChannel. I did the opposite and took away OnDropChannel's destroying privileges, although I could still go the other way. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.h:167: // to a close frame from the server. On 2013/07/09 15:59:51, szym wrote: > Be consistent: "a Close frame". Done. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.h:182: // The event_interface_ object to call back into. On 2013/07/09 15:59:51, szym wrote: > "event_interface_" is unnecessary in the comment. Suggest: > // The object receiving events. Good point. Done. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel.h:220: int current_send_quota_; On 2013/07/09 15:59:51, szym wrote: > Why not use an unsigned type for the quotas so that you don't need checked > casts? > > If it's actually possible for it to go negative, then you should not use checked > casts without first checking that case. The style guide seems to be pulling me in both directions here. In favour of int: 1. "Do not use unsigned types to mean "this value should never be < 0"." from http://www.chromium.org/developers/coding-style . 2. "You should not use the unsigned integer types such as uint32_t, unless there is a valid reason such as representing a bit pattern rather than a number" http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Intege... 3. IOBufferWithSize uses int as its size type. In favour of size_t: 1. "Use unsigned integer types (preferably size_t) for ... object counts... . The signed types are incorrect and unsafe for these purposes." from http://www.chromium.org/developers/coding-style 2. std::vector<char> uses size_t as its size type. https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... File net/websockets/websocket_mux.h (right): https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_mux.h:14: kWebSocketMuxErrorPhysicalConnectionFailed = 2000, On 2013/07/09 15:59:51, szym wrote: > Hmm, Chromium code style suggests using MACRO-style enums for consistency, so > I'm assuming you have a good reason for all the kWebSocket* enums. I failed to notice that the Chromium style guide overrides the Google style guide. I do have a good reason not to change them to MACRO-style right at the moment, which is that I have a bunch of pending CLs in this area at the moment and it will be much less work to change them once the code is all landed.
LGTM, nice tests https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... File net/websockets/websocket_channel_test.cc (right): https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:31: // these tests. Static to reduce the risk of linker clashes. Unnamed namespaces are superior to static (according to C++ standard). https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel.cc:55: ScopedVector<WebSocketFrameChunk>* GetFrames() { return &frames_; } nit: frames() would be preferred in chromium style. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel.cc:358: scoped_ptr<WebSocketFrameChunk> chunk) { Reading through tests, I was wondering if you should be checking chunk->header->payload_length somewhere in this function. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... File net/websockets/websocket_channel_test.cc (right): https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:202: result_header->final = source_header.final == FINAL_FRAME; nit: parens would help https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:204: result_header->masked = source_header.masked == MASKED; ditto https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:209: result_chunk->final_chunk = source_chunks[i].final_chunk == FINAL_CHUNK; ditto https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:264: const CompletionCallback& callback) OVERRIDE { Maybe add some check that this is not called while ReadFrames is pending. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:267: } nit: no need for {} https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:272: base::Unretained(this), To avoid confusing use-after-free I suggest adding a CHECK in the dtor that |index_| is at the end of |responses_|. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:325: // A FakeWebSocketStream which echoes any frames written back. It unset the "It unset" => "Clears" https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:374: // Copy the chunks stored in stored_frame_chunks_ to |to|, while unsetting the suggest "unsetting" -> "clearing" https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:377: bool MoveFrameChunks(ScopedVector<WebSocketFrameChunk>* to) { Suggest |out| rather than |to|. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:379: to->assign(stored_frame_chunks_.begin(), stored_frame_chunks_.end()); out->swap(stored_frame_chunks_); // alternatively *out = stored_frame_chunks_.Pass(); https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:384: const scoped_ptr<WebSocketFrameHeader>& header = (*it)->header; Suggest simple pointer instead of const scoped_ptr&. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:385: if (header->masked) { unnecessary condition https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:390: } nit: unnecessary {} https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:459: // Base class for WebSocketChannelTests. Suggest: // Base class for all test fixtures. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:463: : data_(), channel_(), stream_(new FakeWebSocketStream) {} no need for default initializers https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:478: // well. This method is virtual so that WebSocketChannelStreamTest can also To avoid "comment drift", just "subclasses" instead of "WebSocketChannelStreamTest". "Virtual to allow override." -- would probably suffice. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:499: // parsed as a template method. remark: I learn something new about C++ every day. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:516: ConnectData data_; suggest connect_data_, but I don't really see the benefit of having it in a separate struct. I'd just put all of the fields directly in the fixture, i.e., data_.url => url_ data_.factory => factory_ https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:536: // case, but once should be enough. Just a remark: In test environment, it's typically OK to pass ownership of some mock to an object under test, while retaining a plain pointer to it. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:541: scoped_ptr<MockWebSocketEventInterface> event_interface_; wondering if this should be a StrictMock. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:575: // false means success. Would changing OnAddChannelResponse to (bool success, ...) make it easier to read without comments? https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:670: OnDropChannel(kWebSocketErrorAbnormalClosure, "Abnormal Closure")); If the error message strings matter, should they be defined as constants somewhere? https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:889: // Connection reset. Uninformative comment. I wonder if there is any technical difference between this case and the one right above. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:1158: Pointee(Field(&WebSocketFrameHeader::masked, true)))))), remark: feels like Lisp, http://xkcd.com/297/ I don't know googlemock well enough to propose a remedy :-/
https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... File net/websockets/websocket_channel_test.cc (right): https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:31: // these tests. Static to reduce the risk of linker clashes. On 2013/07/12 17:44:51, szym wrote: > Unnamed namespaces are superior to static (according to C++ standard). The trouble is that unless the operators are in the ::net namespace, GoogleMock can't find them and you get output like this: Actual: 0x2b235439fec0, which points to { 0x2b2354378e80 }, whose element #0 doesn't match, which points to 24-byte object <30-8E 37-54 23-2B 00-00 01-AB AB-AB AB-AB AB-AB 80-99 39-54 23-2B 00-00>, whose given field is 8-byte object <30-8E 37-54 23-2B 00-00>, which points to 24-byte object <01-00 00-00 01-00 00-00 01-AB AB-AB AB-AB AB-AB 0D-00 00-00 00-00 00-00>, whose given field is true Instead of output like this: Actual: 0x131eb2e1dec0, which points to { 0x131eb2de5e80 }, whose element #0 doesn't match, which points to {{FINAL_FRAME, 1, MASKED, 13}, FINAL_CHUNK, "NEEDS MASKING"}, whose given field is 8-byte object <30-5E DE-B2 1E-13 00-00>, which points to {FINAL_FRAME, 1, MASKED, 13}, whose given field is true Being in an anonymous namespace inside net doesn't work for Koenig lookup purposes. Having said that, making them static wasn't really achieving a lot so I removed the static qualifier. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel.cc:55: ScopedVector<WebSocketFrameChunk>* GetFrames() { return &frames_; } On 2013/07/12 17:44:51, szym wrote: > nit: frames() would be preferred in chromium style. I didn't think frames() would be appropriate, since we are returning a pointer to the member and not the value or reference. Protocol buffers use mutable_frames() for a situation like this, do you think that would be acceptable? https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel.cc:358: scoped_ptr<WebSocketFrameChunk> chunk) { On 2013/07/12 17:44:51, szym wrote: > Reading through tests, I was wondering if you should be checking > chunk->header->payload_length somewhere in this function. The lower protocol layers will fail to parse the frames if the length is wrong, so I'm not too worried, but I agree it is useful to have a sanity check, so I have added one. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... File net/websockets/websocket_channel_test.cc (right): https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:202: result_header->final = source_header.final == FINAL_FRAME; On 2013/07/12 17:44:51, szym wrote: > nit: parens would help I was getting kind of an anti-params vibe from the style guide, so I didn't put any in. Thank you for giving me an excuse to add them. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:204: result_header->masked = source_header.masked == MASKED; On 2013/07/12 17:44:51, szym wrote: > ditto Done. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:209: result_chunk->final_chunk = source_chunks[i].final_chunk == FINAL_CHUNK; On 2013/07/12 17:44:51, szym wrote: > ditto Done. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:264: const CompletionCallback& callback) OVERRIDE { On 2013/07/12 17:44:51, szym wrote: > Maybe add some check that this is not called while ReadFrames is pending. Done. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:267: } On 2013/07/12 17:44:51, szym wrote: > nit: no need for {} Done. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:272: base::Unretained(this), On 2013/07/12 17:44:51, szym wrote: > To avoid confusing use-after-free I suggest adding a CHECK in the dtor that > |index_| is at the end of |responses_|. Done. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:325: // A FakeWebSocketStream which echoes any frames written back. It unset the On 2013/07/12 17:44:51, szym wrote: > "It unset" => "Clears" Done. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:374: // Copy the chunks stored in stored_frame_chunks_ to |to|, while unsetting the On 2013/07/12 17:44:51, szym wrote: > suggest "unsetting" -> "clearing" Done. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:377: bool MoveFrameChunks(ScopedVector<WebSocketFrameChunk>* to) { On 2013/07/12 17:44:51, szym wrote: > Suggest |out| rather than |to|. Done. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:379: to->assign(stored_frame_chunks_.begin(), stored_frame_chunks_.end()); On 2013/07/12 17:44:51, szym wrote: > out->swap(stored_frame_chunks_); > // alternatively > *out = stored_frame_chunks_.Pass(); > Oh yeah. I don't know why I did it the hard way. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:384: const scoped_ptr<WebSocketFrameHeader>& header = (*it)->header; On 2013/07/12 17:44:51, szym wrote: > Suggest simple pointer instead of const scoped_ptr&. Done. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:385: if (header->masked) { On 2013/07/12 17:44:51, szym wrote: > unnecessary condition Done. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:390: } On 2013/07/12 17:44:51, szym wrote: > nit: unnecessary {} Done. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:459: // Base class for WebSocketChannelTests. On 2013/07/12 17:44:51, szym wrote: > Suggest: // Base class for all test fixtures. Done. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:463: : data_(), channel_(), stream_(new FakeWebSocketStream) {} On 2013/07/12 17:44:51, szym wrote: > no need for default initializers Done. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:478: // well. This method is virtual so that WebSocketChannelStreamTest can also On 2013/07/12 17:44:51, szym wrote: > To avoid "comment drift", just "subclasses" instead of > "WebSocketChannelStreamTest". > "Virtual to allow override." -- would probably suffice. "Virtual to allow override" is uncomfortably close to a tautology, so I settled on your first comment. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:516: ConnectData data_; On 2013/07/12 17:44:51, szym wrote: > suggest connect_data_, but I don't really see the benefit of having it in a > separate struct. I'd just put all of the fields directly in the fixture, i.e., > data_.url => url_ > data_.factory => factory_ Basically it's a struct so that I don't have to type as many underscores. I realise that that is highly illogical. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:536: // case, but once should be enough. On 2013/07/12 17:44:51, szym wrote: > Just a remark: In test environment, it's typically OK to pass ownership of some > mock to an object under test, while retaining a plain pointer to it. The amount of time I want to spend debugging use-after-free bugs in unit tests is extremely small. I'm willing to accept a reasonable degree of clumsiness to avoid it. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:541: scoped_ptr<MockWebSocketEventInterface> event_interface_; On 2013/07/12 17:44:51, szym wrote: > wondering if this should be a StrictMock. Yes, I had been wondering the same thing. I have made it so. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:575: // false means success. On 2013/07/12 17:44:51, szym wrote: > Would changing OnAddChannelResponse to (bool success, ...) make it easier to > read without comments? I've been bitten by this a few times, but I am trying to to stay close to the draft mux spec. If I changed it, it would be to an enum. I am starting to view function calls with mysterious boolean parameters in the same light as function calls with unexplained magic constants. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:670: OnDropChannel(kWebSocketErrorAbnormalClosure, "Abnormal Closure")); On 2013/07/12 17:44:51, szym wrote: > If the error message strings matter, should they be defined as constants > somewhere? No. Or rather, yes, if they did matter. But this one is an implementation detail, and I shouldn't have been matching it in the first place. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:889: // Connection reset. On 2013/07/12 17:44:51, szym wrote: > Uninformative comment. I wonder if there is any technical difference between > this case and the one right above. The point is actually to verify that the behaviour is the same. As the implementation currently stands, this is trivially true, but an older version of WebSocketChannel actually treated the cases differently. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:1158: Pointee(Field(&WebSocketFrameHeader::masked, true)))))), On 2013/07/12 17:44:51, szym wrote: > remark: feels like Lisp, http://xkcd.com/297/ > > I don't know googlemock well enough to propose a remedy :-/ Yeah. And this is one of the simpler cases. In the "rest of the unit tests" CL I added a custom matcher so this becomes WriteFrames(EqualsChunks(expected), _)) which I think is easier to read.
https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... File net/websockets/websocket_channel_test.cc (right): https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:31: // these tests. Static to reduce the risk of linker clashes. On 2013/07/12 20:46:55, Adam Rice wrote: > On 2013/07/12 17:44:51, szym wrote: > > Unnamed namespaces are superior to static (according to C++ standard). > > The trouble is that unless the operators are in the ::net namespace, GoogleMock > can't find them and you get output like this: > > Actual: 0x2b235439fec0, which points to { 0x2b2354378e80 }, whose > element #0 doesn't match, which points to 24-byte object <30-8E 37-54 23-2B > 00-00 01-AB AB-AB AB-AB AB-AB 80-99 39-54 23-2B 00-00>, whose given field is > 8-byte object <30-8E 37-54 23-2B 00-00>, which points to 24-byte object <01-00 > 00-00 01-00 00-00 01-AB AB-AB AB-AB AB-AB 0D-00 00-00 00-00 00-00>, whose given > field is true > > Instead of output like this: > > Actual: 0x131eb2e1dec0, which points to { 0x131eb2de5e80 }, whose > element #0 doesn't match, which points to {{FINAL_FRAME, 1, MASKED, 13}, > FINAL_CHUNK, "NEEDS MASKING"}, whose given field is 8-byte object <30-5E DE-B2 > 1E-13 00-00>, which points to {FINAL_FRAME, 1, MASKED, 13}, whose given field is > true > > Being in an anonymous namespace inside net doesn't work for Koenig lookup > purposes. > > Having said that, making them static wasn't really achieving a lot so I removed > the static qualifier. Actually, in this case, static does make sense (although risk of clash is low). I suggest a comment clarifying that this cannot be in anonymous namespace to work with GoogleMock. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... File net/websockets/websocket_channel.cc (right): https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel.cc:55: ScopedVector<WebSocketFrameChunk>* GetFrames() { return &frames_; } On 2013/07/12 20:46:55, Adam Rice wrote: > On 2013/07/12 17:44:51, szym wrote: > > nit: frames() would be preferred in chromium style. > > I didn't think frames() would be appropriate, since we are returning a pointer > to the member and not the value or reference. > > Protocol buffers use mutable_frames() for a situation like this, do you think > that would be acceptable? "You may use lowercase letters for other very short inlined function." |frames| fits because it's a trivial accessor to the |frames_| field. IMO, it does not matter if it's const& or *, but if you prefer mutable_frames, I'm fine with that too. https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... File net/websockets/websocket_channel_test.cc (right): https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:202: result_header->final = source_header.final == FINAL_FRAME; On 2013/07/12 20:46:55, Adam Rice wrote: > On 2013/07/12 17:44:51, szym wrote: > > nit: parens would help > > I was getting kind of an anti-params vibe from the style guide, so I didn't put > any in. Thank you for giving me an excuse to add them. Maybe it's my eyes, but while I have no problem with result = a < b; I do need to stare a bit at: result = a == b; https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:575: // false means success. On 2013/07/12 20:46:55, Adam Rice wrote: > On 2013/07/12 17:44:51, szym wrote: > > Would changing OnAddChannelResponse to (bool success, ...) make it easier to > > read without comments? > > I've been bitten by this a few times, but I am trying to to stay close to the > draft mux spec. If I changed it, it would be to an enum. I am starting to view > function calls with mysterious boolean parameters in the same light as function > calls with unexplained magic constants. Agreed, also see Function(string, string, string, string) :-/ https://codereview.chromium.org/12764006/diff/121001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:889: // Connection reset. On 2013/07/12 20:46:55, Adam Rice wrote: > On 2013/07/12 17:44:51, szym wrote: > > Uninformative comment. I wonder if there is any technical difference between > > this case and the one right above. > > The point is actually to verify that the behaviour is the same. As the > implementation currently stands, this is trivially true, but an older version of > WebSocketChannel actually treated the cases differently. ok
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ricea@chromium.org/12764006/139001
https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... File net/websockets/websocket_channel_test.cc (right): https://codereview.chromium.org/12764006/diff/110001/net/websockets/websocket... net/websockets/websocket_channel_test.cc:31: // these tests. Static to reduce the risk of linker clashes. On 2013/07/12 21:03:21, szym wrote: > On 2013/07/12 20:46:55, Adam Rice wrote: > > On 2013/07/12 17:44:51, szym wrote: > > > Unnamed namespaces are superior to static (according to C++ standard). > > > > The trouble is that unless the operators are in the ::net namespace, > GoogleMock > > can't find them and you get output like this: > > > > Actual: 0x2b235439fec0, which points to { 0x2b2354378e80 }, whose > > element #0 doesn't match, which points to 24-byte object <30-8E 37-54 23-2B > > 00-00 01-AB AB-AB AB-AB AB-AB 80-99 39-54 23-2B 00-00>, whose given field is > > 8-byte object <30-8E 37-54 23-2B 00-00>, which points to 24-byte object <01-00 > > 00-00 01-00 00-00 01-AB AB-AB AB-AB AB-AB 0D-00 00-00 00-00 00-00>, whose > given > > field is true > > > > Instead of output like this: > > > > Actual: 0x131eb2e1dec0, which points to { 0x131eb2de5e80 }, whose > > element #0 doesn't match, which points to {{FINAL_FRAME, 1, MASKED, 13}, > > FINAL_CHUNK, "NEEDS MASKING"}, whose given field is 8-byte object <30-5E DE-B2 > > 1E-13 00-00>, which points to {FINAL_FRAME, 1, MASKED, 13}, whose given field > is > > true > > > > Being in an anonymous namespace inside net doesn't work for Koenig lookup > > purposes. > > > > Having said that, making them static wasn't really achieving a lot so I > removed > > the static qualifier. > > Actually, in this case, static does make sense (although risk of clash is low). > I suggest a comment clarifying that this cannot be in anonymous namespace to > work with GoogleMock. Added a clarifying comment.
Retried try job too often on linux_aura for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ricea@chromium.org/12764006/139001
Retried try job too often on linux_aura for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ricea@chromium.org/12764006/139001
Message was sent while issue was closed.
Change committed as 212019
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/12764006/diff/139001/net/websockets/we... File net/websockets/websocket_channel.h (right): https://chromiumcodereview.appspot.com/12764006/diff/139001/net/websockets/we... net/websockets/websocket_channel.h:15: #include "googleurl/src/gurl.h" how you were able to land this? Didn't you get a PRESUBMIT warning? you should have used url/gurl.h instead!
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/12764006/diff/139001/net/websockets/we... File net/websockets/websocket_channel.h (right): https://chromiumcodereview.appspot.com/12764006/diff/139001/net/websockets/we... net/websockets/websocket_channel.h:15: #include "googleurl/src/gurl.h" On 2013/07/19 23:11:00, tfarina wrote: > how you were able to land this? Didn't you get a PRESUBMIT warning? you should > have used url/gurl.h instead! I'm fixing this here https://codereview.chromium.org/19492015
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/12764006/diff/139001/net/websockets/we... File net/websockets/websocket_channel.h (right): https://chromiumcodereview.appspot.com/12764006/diff/139001/net/websockets/we... net/websockets/websocket_channel.h:15: #include "googleurl/src/gurl.h" On 2013/07/19 23:11:00, tfarina wrote: > how you were able to land this? Didn't you get a PRESUBMIT warning? you should > have used url/gurl.h instead! I was under the impression that I had seen a PRESUBMIT warning, and fixed it. Apparently not. https://chromiumcodereview.appspot.com/12764006/diff/139001/net/websockets/we... net/websockets/websocket_channel.h:15: #include "googleurl/src/gurl.h" On 2013/07/19 23:26:07, tfarina wrote: > On 2013/07/19 23:11:00, tfarina wrote: > > how you were able to land this? Didn't you get a PRESUBMIT warning? you should > > have used url/gurl.h instead! > > I'm fixing this here https://codereview.chromium.org/19492015 Thank you for your fix. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
