Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18792002:
WebSocketBasicStream framing logic (Closed)
|
Created:
7 years, 5 months ago by Adam Rice Modified:
7 years, 3 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, yhirano Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionWebSocketBasicStream is the basic implementation of the WebSocket protocol over
TCP/IP with no extensions in use.
This CL implements the logic to read and write frames from the stream that is used after connection.
BUG=257680
TEST=net_unittests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220573
Patch Set 1 #Patch Set 2 : Compile fixes. #
Total comments: 2
Patch Set 3 : Remove handshake functionality and add ReadFrames tests #Patch Set 4 : Add more unit tests and fix bugs. #Patch Set 5 : Comment fixes, plus fixed an error case. #Patch Set 6 : Comment fixes. #
Total comments: 15
Patch Set 7 : Fixes from tyoshino code review. #
Total comments: 8
Patch Set 8 : Add includes and rename "data" variable #
Total comments: 44
Patch Set 9 : Refactor ReadFrames() plus other fixes from szym #Patch Set 10 : Amend comment to http_read_buffer_ to explain the type. #
Total comments: 4
Patch Set 11 : Add comments on purpose of *CloseWithErr tests #Patch Set 12 : Remove "We" from comment. #
Messages
Total messages: 18 (0 generated)
https://codereview.chromium.org/18792002/diff/2001/net/websockets/websocket_b... File net/websockets/websocket_basic_stream.cc (right): https://codereview.chromium.org/18792002/diff/2001/net/websockets/websocket_b... net/websockets/websocket_basic_stream.cc:49: // TODO(ricea): Define all these constants in one central place yes. you can split this into a cl adding a file containing WebSocket protocol constants. it's nice if you also replace string literal occurrence of protocol constants in Chromium code base with them. https://codereview.chromium.org/18792002/diff/2001/net/websockets/websocket_b... net/websockets/websocket_basic_stream.cc:453: int WebSocketBasicStream::ProcessHandshake() { consider splitting handshake processing code or starting with incomplete code e.g. just framing or just handshake leaving the other unimplemented. it would be easier to review. it's totally fine since they're not live yet.
PTAL
https://codereview.chromium.org/18792002/diff/24001/net/websockets/websocket_... File net/websockets/websocket_basic_stream.cc (right): https://codereview.chromium.org/18792002/diff/24001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:23: "implemented in another CL."; \ ok but you can just put "NOTREACHED();" and put "// TODO(ricea): Implement handshake-related functionality" on the first occurrence. Also, it'd a bit odd that "another CL" is used in code, not CL description. how about changing "in another CL" to "later"? https://codereview.chromium.org/18792002/diff/24001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:142: char* data = total->data(); how about naming this "dest"? https://codereview.chromium.org/18792002/diff/24001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:154: remaining_size -= result; put one blank line to separate header writing code and body writing code? https://codereview.chromium.org/18792002/diff/24001/net/websockets/websocket_... File net/websockets/websocket_basic_stream.h (right): https://codereview.chromium.org/18792002/diff/24001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:11: #include "base/memory/ref_counted.h" base/memory/scoped_ptr.h https://codereview.chromium.org/18792002/diff/24001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:13: #include "base/strings/string_piece.h" not used? https://codereview.chromium.org/18792002/diff/24001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:62: // "Sec-Websocket-Key" header; this should not be included in |headers|. Websocket -> WebSocket https://codereview.chromium.org/18792002/diff/24001/net/websockets/websocket_... File net/websockets/websocket_basic_stream_test.cc (right): https://codereview.chromium.org/18792002/diff/24001/net/websockets/websocket_... net/websockets/websocket_basic_stream_test.cc:338: TEST_F(WebSocketBasicStreamSocketTest, ASyncCloseWithErr) { ASync -> Async
https://codereview.chromium.org/18792002/diff/24001/net/websockets/websocket_... File net/websockets/websocket_basic_stream.cc (right): https://codereview.chromium.org/18792002/diff/24001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:23: "implemented in another CL."; \ On 2013/08/22 07:56:10, tyoshino wrote: > ok but you can just put "NOTREACHED();" and put "// TODO(ricea): Implement > handshake-related functionality" on the first occurrence. > > Also, it'd a bit odd that "another CL" is used in code, not CL description. how > about changing "in another CL" to "later"? Done. https://codereview.chromium.org/18792002/diff/24001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:142: char* data = total->data(); On 2013/08/22 07:56:10, tyoshino wrote: > how about naming this "dest"? Done. https://codereview.chromium.org/18792002/diff/24001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:154: remaining_size -= result; On 2013/08/22 07:56:10, tyoshino wrote: > put one blank line to separate header writing code and body writing code? Done. https://codereview.chromium.org/18792002/diff/24001/net/websockets/websocket_... File net/websockets/websocket_basic_stream.h (right): https://codereview.chromium.org/18792002/diff/24001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:11: #include "base/memory/ref_counted.h" On 2013/08/22 07:56:10, tyoshino wrote: > base/memory/scoped_ptr.h Done. https://codereview.chromium.org/18792002/diff/24001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:13: #include "base/strings/string_piece.h" On 2013/08/22 07:56:10, tyoshino wrote: > not used? Yes. Removed. https://codereview.chromium.org/18792002/diff/24001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:62: // "Sec-Websocket-Key" header; this should not be included in |headers|. On 2013/08/22 07:56:10, tyoshino wrote: > Websocket -> WebSocket Done. https://codereview.chromium.org/18792002/diff/24001/net/websockets/websocket_... File net/websockets/websocket_basic_stream_test.cc (right): https://codereview.chromium.org/18792002/diff/24001/net/websockets/websocket_... net/websockets/websocket_basic_stream_test.cc:338: TEST_F(WebSocketBasicStreamSocketTest, ASyncCloseWithErr) { On 2013/08/22 07:56:10, tyoshino wrote: > ASync -> Async Done (and the other places).
lgtm https://codereview.chromium.org/18792002/diff/24001/net/websockets/websocket_... File net/websockets/websocket_basic_stream.cc (right): https://codereview.chromium.org/18792002/diff/24001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:142: char* data = total->data(); On 2013/08/22 08:13:04, Adam Rice wrote: > On 2013/08/22 07:56:10, tyoshino wrote: > > how about naming this "dest"? > > Done. sorry. i meant s/data/dest/ https://codereview.chromium.org/18792002/diff/28001/net/websockets/websocket_... File net/websockets/websocket_basic_stream.cc (right): https://codereview.chromium.org/18792002/diff/28001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:13: #include "base/bind.h" add base/logging.h https://codereview.chromium.org/18792002/diff/28001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:13: #include "base/bind.h" add net/base/net_errors.h https://codereview.chromium.org/18792002/diff/28001/net/websockets/websocket_... File net/websockets/websocket_basic_stream_test.cc (right): https://codereview.chromium.org/18792002/diff/28001/net/websockets/websocket_... net/websockets/websocket_basic_stream_test.cc:11: #include "base/basictypes.h" add base/port.h https://codereview.chromium.org/18792002/diff/28001/net/websockets/websocket_... net/websockets/websocket_basic_stream_test.cc:19: please add - tests for empty frame cases - frame writing test with non-nul masking key - receiving a big frame whose size exceeds kReadAtTime later. write TODO please
https://codereview.chromium.org/18792002/diff/28001/net/websockets/websocket_... File net/websockets/websocket_basic_stream.cc (right): https://codereview.chromium.org/18792002/diff/28001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:13: #include "base/bind.h" On 2013/08/22 21:08:43, tyoshino wrote: > add > base/logging.h Done. https://codereview.chromium.org/18792002/diff/28001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:13: #include "base/bind.h" On 2013/08/22 21:08:43, tyoshino wrote: > add > net/base/net_errors.h Done. https://codereview.chromium.org/18792002/diff/28001/net/websockets/websocket_... File net/websockets/websocket_basic_stream_test.cc (right): https://codereview.chromium.org/18792002/diff/28001/net/websockets/websocket_... net/websockets/websocket_basic_stream_test.cc:11: #include "base/basictypes.h" On 2013/08/22 21:08:43, tyoshino wrote: > add > base/port.h Done. https://codereview.chromium.org/18792002/diff/28001/net/websockets/websocket_... net/websockets/websocket_basic_stream_test.cc:19: On 2013/08/22 21:08:43, tyoshino wrote: > please add > - tests for empty frame cases > - frame writing test with non-nul masking key > - receiving a big frame whose size exceeds kReadAtTime > later. > > write TODO please Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ricea@chromium.org/18792002/35001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
+szym for net review.
Haven't read the tests yet. https://codereview.chromium.org/18792002/diff/35001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/18792002/diff/35001/net/net.gyp#newcode1858 net/net.gyp:1858: 'websockets/websocket_basic_stream_test.cc', The file name should end with _unittest.cc If possible, please change the name of websocket_channel_test.cc below to end with _unittest.cc as well. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... File net/websockets/websocket_basic_stream.cc (right): https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:29: const int kReadAtATime = 32 * 1024; suggest: kReadBufferSize This is merely the size of the buffer passed to Socket::Read. Why would you change this value dynamically? https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:37: generate_websocket_masking_key_(&GenerateWebSocketMaskingKey) {} suggest: DCHECK(connection_->is_initialized()) https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:40: connection_->socket()->Disconnect(); Suggest calling Close() instead. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:63: // delete us before deleting frame_chunks. Do not refer to WebSocketChannel. This use is safe, because of the semantics of ReadFrames. "|frame_chunks| remains owned by the caller and must be valid until the operation completes..." So this comment is unnecessary, but if you want to be explicit, just say "The semantics of ReadFrames() guarantee |frame_chunks| will live until callback or Close()." https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:84: void WebSocketBasicStream::ReadDone( Chromium code style: Function declaration order should match function definition order. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:88: if (result > 0) { The logic here and in ReadFrames is very similar but rather difficult to follow. I suggest extracting it to one location in a design pattern similar to "int DoLoop(int result)". Here, I suggest: // Returns ERR_IO_PENDING if needs more data. int HandleReadResult(int result, * frame_chunks) { DCHECK_NE(ERR_IO_PENDING, result); DCHECK(frame_chunks->empty()); if (result < 0) return result; if (result == 0) return ERR_CONNECTION_CLOSED; if (!parser_.Decode(read_buffer_, result, frame_chunks)) return WebSocketErrorToNetError(parser_.websocket_error()) if (!frame_chunks->empty()) return OK; return ERR_IO_PENDING; } int ReadFrames(* frame_chunks, callback) { DCHECK(frame_chunks->empty()); // Run until socket stops giving us data or we get some chunks. while(true) { int result = socket->Read(..., Bind(OnReadComplete, ...)); if (result == ERR_IO_PENDING) return result; result = HandleReadResult(result, frame_chunks); if (result != ERR_IO_PENDING) return result; } } void OnReadComplete(int result, * frame_chunks, callback) { int result = HandleReadResult(result, frame_chunks); if (result == ERR_IO_PENDING) result = ReadFrames(frame_chunks, callback); if (result != ERR_IO_PENDING) callback.Run(result); } https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:122: DCHECK(chunk->header && chunk->final_chunk) Suggest breaking DCHECK(..&&..) into two DCHECKs https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:130: CHECK_GE(std::numeric_limits<int>::max() - total_size, chunk_size) Define a constant const int kMaximumTotalSize = ... and use it instead of std::numeric_limits here https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:191: if (result > 0) { Suggest unrolling the if{} folds in this function: if (result < 0) { callback.Run(result); return; } buffer->DidConsume(result); ... https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:193: if (buffer->BytesRemaining() > 0) { You don't need this check. Just call WriteEverything(). https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:203: callback.Run(result); BUG: It's possible to end up in here with result == 0 and return OK. This does not match the spec in websocket_stream.h Although I'm not sure Socket::Write could return 0. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... File net/websockets/websocket_basic_stream.h (right): https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:31: // The type of a pointer to a function that generates a WebSocketMaskingKey. Spurious comment. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:43: // specifications of the behaviour. Remove all comments on the OVERRIDEN methods if they implement the spec in the base class. A comment is necessary if the behavior differs in any way from the spec. If the general spec needs to be updated, do it in the base class. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:88: // |key_generator_function|. Spurious comment. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:97: // The semantics of WriteFrames() are such that it only returns OK or calls Do not elaborate on the implementation in the header comment. "Returns OK or calls |callback| when the |buffer| is fully drained or something has failed." https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:105: void WriteDone(const scoped_refptr<DrainableIOBuffer>& buffer, Suggest: OnWriteComplete or OnWriteDone https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:111: void ReadDone(ScopedVector<WebSocketFrameChunk>* frame_chunks, Suggest: OnReadComplete or OnReadDone https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:121: // from being returned to the pool. connection_ is used only for its socket_. The socket is always Disconnected after passed to this WebSocketBasicStream, so instead of passing ClientSocketHandle you should pass the StreamSocket. Then you can use it as: ClientSocketHandle handle; new WebSocketBasicStream(handle.PassSocket()); https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:127: scoped_refptr<GrowableIOBuffer> http_read_buffer_; This appears to be used only in tests. Does it have a future non-test use case?
https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... File net/websockets/websocket_basic_stream.h (right): https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:121: // from being returned to the pool. On 2013/08/27 22:54:04, szym wrote: > connection_ is used only for its socket_. The socket is always Disconnected > after passed to this WebSocketBasicStream, so instead of passing > ClientSocketHandle you should pass the StreamSocket. > > Then you can use it as: > > ClientSocketHandle handle; > new WebSocketBasicStream(handle.PassSocket()); > Disregard this comment. akalin@ explained to me in https://codereview.chromium.org/18792002/ that that'd be illegal and using CSH is preferred.
https://codereview.chromium.org/18792002/diff/35001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/18792002/diff/35001/net/net.gyp#newcode1858 net/net.gyp:1858: 'websockets/websocket_basic_stream_test.cc', On 2013/08/27 22:54:04, szym wrote: > The file name should end with _unittest.cc > > If possible, please change the name of websocket_channel_test.cc below to end > with _unittest.cc as well. *_unittest.cc is deprecated: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=File_N... It appears that newer code in net/ is using _test.cc more. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... File net/websockets/websocket_basic_stream.cc (right): https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:29: const int kReadAtATime = 32 * 1024; On 2013/08/27 22:54:04, szym wrote: > suggest: kReadBufferSize > > This is merely the size of the buffer passed to Socket::Read. Why would you > change this value dynamically? Some applications of WebSockets may require a large number of low-bandwidth sockets, so it would be good to limit the memory used by each socket. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:37: generate_websocket_masking_key_(&GenerateWebSocketMaskingKey) {} On 2013/08/27 22:54:04, szym wrote: > suggest: DCHECK(connection_->is_initialized()) Done. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:63: // delete us before deleting frame_chunks. On 2013/08/27 22:54:04, szym wrote: > Do not refer to WebSocketChannel. This use is safe, because of the semantics of > ReadFrames. "|frame_chunks| remains owned by the caller and must be valid until > the operation completes..." So this comment is unnecessary, but if you want to > be explicit, just say "The semantics of ReadFrames() guarantee |frame_chunks| > will live until callback or Close()." I plan to rename base::Unretained to base::CriticalSecurityVulnerability in the future, so I am putting these comments in place as insurance. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:84: void WebSocketBasicStream::ReadDone( On 2013/08/27 22:54:04, szym wrote: > Chromium code style: Function declaration order should match function definition > order. Done. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:88: if (result > 0) { On 2013/08/27 22:54:04, szym wrote: > The logic here and in ReadFrames is very similar but rather difficult to follow. > I suggest extracting it to one location in a design pattern similar to "int > DoLoop(int result)". Here, I suggest: > > // Returns ERR_IO_PENDING if needs more data. > int HandleReadResult(int result, * frame_chunks) { > DCHECK_NE(ERR_IO_PENDING, result); > DCHECK(frame_chunks->empty()); > if (result < 0) > return result; > if (result == 0) > return ERR_CONNECTION_CLOSED; > if (!parser_.Decode(read_buffer_, result, frame_chunks)) > return WebSocketErrorToNetError(parser_.websocket_error()) > if (!frame_chunks->empty()) > return OK; > return ERR_IO_PENDING; > } > > int ReadFrames(* frame_chunks, callback) { > DCHECK(frame_chunks->empty()); > // Run until socket stops giving us data or we get some chunks. > while(true) { > int result = socket->Read(..., Bind(OnReadComplete, ...)); > if (result == ERR_IO_PENDING) > return result; > result = HandleReadResult(result, frame_chunks); > if (result != ERR_IO_PENDING) > return result; > } > } > > void OnReadComplete(int result, * frame_chunks, callback) { > int result = HandleReadResult(result, frame_chunks); > if (result == ERR_IO_PENDING) > result = ReadFrames(frame_chunks, callback); > if (result != ERR_IO_PENDING) > callback.Run(result); > } Done. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:122: DCHECK(chunk->header && chunk->final_chunk) On 2013/08/27 22:54:04, szym wrote: > Suggest breaking DCHECK(..&&..) into two DCHECKs Done. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:130: CHECK_GE(std::numeric_limits<int>::max() - total_size, chunk_size) On 2013/08/27 22:54:04, szym wrote: > Define a constant > const int kMaximumTotalSize = ... > and use it instead of std::numeric_limits here Done. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:191: if (result > 0) { On 2013/08/27 22:54:04, szym wrote: > Suggest unrolling the if{} folds in this function: > > if (result < 0) { > callback.Run(result); > return; > } > > buffer->DidConsume(result); > ... Done. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:193: if (buffer->BytesRemaining() > 0) { On 2013/08/27 22:54:04, szym wrote: > You don't need this check. Just call WriteEverything(). Done. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.cc:203: callback.Run(result); On 2013/08/27 22:54:04, szym wrote: > BUG: It's possible to end up in here with result == 0 and return OK. This does > not match the spec in websocket_stream.h > > Although I'm not sure Socket::Write could return 0. I've put in a DCHECK for result != 0 for the time being. socket.h has the unhelpful comment "The return value when the connection is closed is undefined, and may be OS dependent." It is possible that a Socket implementer could take that as license to use 0 for "Connection Closed", but I hope no-one would do that. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... File net/websockets/websocket_basic_stream.h (right): https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:31: // The type of a pointer to a function that generates a WebSocketMaskingKey. On 2013/08/27 22:54:04, szym wrote: > Spurious comment. Removed. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:43: // specifications of the behaviour. On 2013/08/27 22:54:04, szym wrote: > Remove all comments on the OVERRIDEN methods if they implement the spec in the > base class. A comment is necessary if the behavior differs in any way from the > spec. If the general spec needs to be updated, do it in the base class. Thanks. That helps. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:88: // |key_generator_function|. On 2013/08/27 22:54:04, szym wrote: > Spurious comment. Removed. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:97: // The semantics of WriteFrames() are such that it only returns OK or calls On 2013/08/27 22:54:04, szym wrote: > Do not elaborate on the implementation in the header comment. > > "Returns OK or calls |callback| when the |buffer| is fully drained or something > has failed." Done. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:105: void WriteDone(const scoped_refptr<DrainableIOBuffer>& buffer, On 2013/08/27 22:54:04, szym wrote: > Suggest: OnWriteComplete or OnWriteDone Done. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:111: void ReadDone(ScopedVector<WebSocketFrameChunk>* frame_chunks, On 2013/08/27 22:54:04, szym wrote: > Suggest: OnReadComplete or OnReadDone Done. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:121: // from being returned to the pool. On 2013/08/27 23:51:30, szym wrote: > On 2013/08/27 22:54:04, szym wrote: > > connection_ is used only for its socket_. The socket is always Disconnected > > after passed to this WebSocketBasicStream, so instead of passing > > ClientSocketHandle you should pass the StreamSocket. > > > > Then you can use it as: > > > > ClientSocketHandle handle; > > new WebSocketBasicStream(handle.PassSocket()); > > > > Disregard this comment. akalin@ explained to me in > https://codereview.chromium.org/18792002/ that that'd be illegal and using CSH > is preferred. Done. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:127: scoped_refptr<GrowableIOBuffer> http_read_buffer_; On 2013/08/27 22:54:04, szym wrote: > This appears to be used only in tests. Does it have a future non-test use case? Yes. I left it in this CL because the first call to ReadFrames() after the handshake is complete may need to consume this data.
https://codereview.chromium.org/18792002/diff/35001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/18792002/diff/35001/net/net.gyp#newcode1858 net/net.gyp:1858: 'websockets/websocket_basic_stream_test.cc', On 2013/08/28 12:01:11, Adam Rice wrote: > On 2013/08/27 22:54:04, szym wrote: > > The file name should end with _unittest.cc > > > > If possible, please change the name of websocket_channel_test.cc below to end > > with _unittest.cc as well. > > *_unittest.cc is deprecated: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=File_N... > > It appears that newer code in net/ is using _test.cc more. I suggest using _unittest.cc for consistency. The code in net/ that uses _test.cc is primarily quic-related which has been ported out of Google internal. If you intend to switch to _test.cc I suggest renaming the other 8 files in websockets/*_unittest.cc as well. https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... File net/websockets/websocket_basic_stream.h (right): https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:127: scoped_refptr<GrowableIOBuffer> http_read_buffer_; On 2013/08/28 12:01:11, Adam Rice wrote: > On 2013/08/27 22:54:04, szym wrote: > > This appears to be used only in tests. Does it have a future non-test use > case? > > Yes. I left it in this CL because the first call to ReadFrames() after the > handshake is complete may need to consume this data. I understand that ReadFrames would use it if set, but in what non-test use case is this field ever non-NULL? Why does it need to be a GrowableIOBuffer?
https://codereview.chromium.org/18792002/diff/35001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/18792002/diff/35001/net/net.gyp#newcode1858 net/net.gyp:1858: 'websockets/websocket_basic_stream_test.cc', On 2013/08/28 21:17:54, szym wrote: > On 2013/08/28 12:01:11, Adam Rice wrote: > > On 2013/08/27 22:54:04, szym wrote: > > > The file name should end with _unittest.cc > > > > > > If possible, please change the name of websocket_channel_test.cc below to > end > > > with _unittest.cc as well. > > > > *_unittest.cc is deprecated: > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=File_N... > > > > It appears that newer code in net/ is using _test.cc more. > > I suggest using _unittest.cc for consistency. The code in net/ that uses > _test.cc is primarily quic-related which has been ported out of Google internal. > > If you intend to switch to _test.cc I suggest renaming the other 8 files in > websockets/*_unittest.cc as well. I discussed it with the team, and we decided to do the rename: https://codereview.chromium.org/23614009/ https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... File net/websockets/websocket_basic_stream.h (right): https://codereview.chromium.org/18792002/diff/35001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:127: scoped_refptr<GrowableIOBuffer> http_read_buffer_; On 2013/08/28 21:17:54, szym wrote: > On 2013/08/28 12:01:11, Adam Rice wrote: > > On 2013/08/27 22:54:04, szym wrote: > > > This appears to be used only in tests. Does it have a future non-test use > > case? > > > > Yes. I left it in this CL because the first call to ReadFrames() after the > > handshake is complete may need to consume this data. > > I understand that ReadFrames would use it if set, but in what non-test use case > is this field ever non-NULL? Why does it need to be a GrowableIOBuffer? I added a comment indicating why it is a GrowableIOBuffer. The code to initialise this field is not in this CL, but I am afraid to remove the test. If I forget to put the test back again, everything will appear to work until someone suffers data loss. Thank you for your patience.
lgtm https://codereview.chromium.org/18792002/diff/58001/net/websockets/websocket_... File net/websockets/websocket_basic_stream.h (right): https://codereview.chromium.org/18792002/diff/58001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:41: virtual int ReadFrames(ScopedVector<WebSocketFrameChunk>* frame_chunks, nit: // WebSocketStream implementation: https://codereview.chromium.org/18792002/diff/58001/net/websockets/websocket_... File net/websockets/websocket_basic_stream_test.cc (right): https://codereview.chromium.org/18792002/diff/58001/net/websockets/websocket_... net/websockets/websocket_basic_stream_test.cc:338: // The result should be the same if the socket returns ERR_CONNECTION_CLOSED I think Socket implementations only return this during connection handshake, but maybe I'm missing some cases. What should ReadFrames return if the Socket fails with some other error?
https://codereview.chromium.org/18792002/diff/58001/net/websockets/websocket_... File net/websockets/websocket_basic_stream.h (right): https://codereview.chromium.org/18792002/diff/58001/net/websockets/websocket_... net/websockets/websocket_basic_stream.h:41: virtual int ReadFrames(ScopedVector<WebSocketFrameChunk>* frame_chunks, On 2013/08/29 16:42:01, szym wrote: > nit: // WebSocketStream implementation: Done. https://codereview.chromium.org/18792002/diff/58001/net/websockets/websocket_... File net/websockets/websocket_basic_stream_test.cc (right): https://codereview.chromium.org/18792002/diff/58001/net/websockets/websocket_... net/websockets/websocket_basic_stream_test.cc:338: // The result should be the same if the socket returns ERR_CONNECTION_CLOSED On 2013/08/29 16:42:01, szym wrote: > I think Socket implementations only return this during connection handshake, but > maybe I'm missing some cases. What should ReadFrames return if the Socket fails > with some other error? I added a comment clarifying that we are not actually expecting to see this error in practice. I also added two tests verifying that other Socket errors are passed through unchanged.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ricea@chromium.org/18792002/68001
Message was sent while issue was closed.
Change committed as 220573 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
