Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10808073:
Add WebSocketStream interface. (Closed)
|
Created:
8 years, 5 months ago by Yuta Kitamura Modified:
8 years, 4 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd WebSocketStream interface.
BUG=116624
TEST=none
R=mmenke@chromium.org,willchan@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150713
Patch Set 1 #
Total comments: 17
Patch Set 2 : Address comments. #
Total comments: 4
Patch Set 3 : Address Matt's comments. #
Messages
Total messages: 11 (0 generated)
http://codereview.chromium.org/10808073/diff/1/net/websockets/websocket_stream.h File net/websockets/websocket_stream.h (right): http://codereview.chromium.org/10808073/diff/1/net/websockets/websocket_strea... net/websockets/websocket_stream.h:74: const CompletionCallback& callback) = 0; How is this supposed to work when it sends only part of the WebSocket frame data? How do you send the rest? http://codereview.chromium.org/10808073/diff/1/net/websockets/websocket_strea... net/websockets/websocket_stream.h:79: virtual int Disconnect() = 0; Should this be Close()? Also, what about the multiplexing extension? Wouldn't that result in having two WebSocketStreams with the same underlying connection? http://codereview.chromium.org/10808073/diff/1/net/websockets/websocket_strea... net/websockets/websocket_stream.h:79: virtual int Disconnect() = 0; Why do we need a return value?
https://chromiumcodereview.appspot.com/10808073/diff/1/net/websockets/websock... File net/websockets/websocket_stream.h (right): https://chromiumcodereview.appspot.com/10808073/diff/1/net/websockets/websock... net/websockets/websocket_stream.h:31: virtual ~WebSocketStream() {} Document that ~WebSocketStream will Close() the stream. https://chromiumcodereview.appspot.com/10808073/diff/1/net/websockets/websock... net/websockets/websocket_stream.h:35: // ERR_IO_PENDING, |callback| is called when the operation is finished. This In all these APIs, document that !callback.is_null(). https://chromiumcodereview.appspot.com/10808073/diff/1/net/websockets/websock... net/websockets/websocket_stream.h:41: virtual int InitializeStream(const HttpRequestInfo* request_info, const HttpRequestInfo&? I don't think it should be possible to be NULL, right? https://chromiumcodereview.appspot.com/10808073/diff/1/net/websockets/websock... net/websockets/websocket_stream.h:55: virtual int ReadHandshakeResponse(const CompletionCallback& callback) = 0; Why is this split out from SendHandshakeRequest()? https://chromiumcodereview.appspot.com/10808073/diff/1/net/websockets/websock... net/websockets/websocket_stream.h:60: // |frame_chunks| must be valid until the operation completes. or canceled via Close() https://chromiumcodereview.appspot.com/10808073/diff/1/net/websockets/websock... net/websockets/websocket_stream.h:79: virtual int Disconnect() = 0; On 2012/07/23 17:46:05, Matt Menke wrote: > Should this be Close()? Also, what about the multiplexing extension? Wouldn't > that result in having two WebSocketStreams with the same underlying connection? Yeah, this should be called closing the WebSocketStream. It shouldn't reference the underlying connection at all. You also need to document that all IO operations are canceled at this point, so |frame_chunks| can be freed.
PTAL https://chromiumcodereview.appspot.com/10808073/diff/1/net/websockets/websock... File net/websockets/websocket_stream.h (right): https://chromiumcodereview.appspot.com/10808073/diff/1/net/websockets/websock... net/websockets/websocket_stream.h:31: virtual ~WebSocketStream() {} On 2012/07/23 21:28:52, willchan wrote: > Document that ~WebSocketStream will Close() the stream. Done. https://chromiumcodereview.appspot.com/10808073/diff/1/net/websockets/websock... net/websockets/websocket_stream.h:35: // ERR_IO_PENDING, |callback| is called when the operation is finished. This On 2012/07/23 21:28:52, willchan wrote: > In all these APIs, document that !callback.is_null(). Done. Requirement for asynchronous operations is moved to the top of the class. https://chromiumcodereview.appspot.com/10808073/diff/1/net/websockets/websock... net/websockets/websocket_stream.h:41: virtual int InitializeStream(const HttpRequestInfo* request_info, On 2012/07/23 21:28:52, willchan wrote: > const HttpRequestInfo&? I don't think it should be possible to be NULL, right? Correct. Fixed. (I just copied this from HttpStream, and am not sure if there's some history here...) https://chromiumcodereview.appspot.com/10808073/diff/1/net/websockets/websock... net/websockets/websocket_stream.h:55: virtual int ReadHandshakeResponse(const CompletionCallback& callback) = 0; On 2012/07/23 21:28:52, willchan wrote: > Why is this split out from SendHandshakeRequest()? Good point, there is not much reason except that HttpStream has both SendRequest() and ReadResponseHeaders(). Having both Send and Read functions will probably make the implementation a little bit easier. Otherwise we need a state machine in Stream implementation. Also, users might want to distinguish whether a failure comes from write or read. https://chromiumcodereview.appspot.com/10808073/diff/1/net/websockets/websock... net/websockets/websocket_stream.h:60: // |frame_chunks| must be valid until the operation completes. On 2012/07/23 21:28:52, willchan wrote: > or canceled via Close() Done. https://chromiumcodereview.appspot.com/10808073/diff/1/net/websockets/websock... net/websockets/websocket_stream.h:74: const CompletionCallback& callback) = 0; On 2012/07/23 17:46:05, Matt Menke wrote: > How is this supposed to work when it sends only part of the WebSocket frame > data? How do you send the rest? Users are expected to send one or more WebSocketFrameChunks with |final_chunk| set to false, followed by a WebSocketFrameChunk with |final_chunk| set to true. These chunks collectively represent a WebSocket frame. I clarified this point in the updated patch. https://chromiumcodereview.appspot.com/10808073/diff/1/net/websockets/websock... net/websockets/websocket_stream.h:79: virtual int Disconnect() = 0; On 2012/07/23 21:28:52, willchan wrote: > On 2012/07/23 17:46:05, Matt Menke wrote: > > Should this be Close()? Also, what about the multiplexing extension? > Wouldn't > > that result in having two WebSocketStreams with the same underlying > connection? > > Yeah, this should be called closing the WebSocketStream. It shouldn't reference > the underlying connection at all. > > You also need to document that all IO operations are canceled at this point, so > |frame_chunks| can be freed. OK, fixed.
LGTM, though you should also wait to see if Will has anything to add. https://chromiumcodereview.appspot.com/10808073/diff/1/net/websockets/websock... File net/websockets/websocket_stream.h (right): https://chromiumcodereview.appspot.com/10808073/diff/1/net/websockets/websock... net/websockets/websocket_stream.h:74: const CompletionCallback& callback) = 0; On 2012/08/06 11:34:29, Yuta Kitamura wrote: > On 2012/07/23 17:46:05, Matt Menke wrote: > > How is this supposed to work when it sends only part of the WebSocket frame > > data? How do you send the rest? > > Users are expected to send one or more WebSocketFrameChunks > with |final_chunk| set to false, followed by > a WebSocketFrameChunk with |final_chunk| set to true. These > chunks collectively represent a WebSocket frame. > > I clarified this point in the updated patch. Thanks. You're right - the old comment had confused me. https://chromiumcodereview.appspot.com/10808073/diff/6001/net/websockets/webs... File net/websockets/websocket_stream.h (right): https://chromiumcodereview.appspot.com/10808073/diff/6001/net/websockets/webs... net/websockets/websocket_stream.h:53: // ReadHandshakeResponse() is called. Suggest documenting the lifetime of |response_info|. Looking at HttpStream, the expected lifetime isn't terribly clear there, either. It doesn't look to be written to after the headers are read, however, the HttpStream and the parser both continue to keep a copy of the pointer until destruction. Suggest just saying it must remain valid until the stream is destroyed. https://chromiumcodereview.appspot.com/10808073/diff/6001/net/websockets/webs... net/websockets/websocket_stream.h:83: // should not be called while previous call of WriteFrames() is still pending. nit: "while the previous call"
Thanks. Will, do you have anything to add? https://chromiumcodereview.appspot.com/10808073/diff/6001/net/websockets/webs... File net/websockets/websocket_stream.h (right): https://chromiumcodereview.appspot.com/10808073/diff/6001/net/websockets/webs... net/websockets/websocket_stream.h:53: // ReadHandshakeResponse() is called. On 2012/08/06 16:15:37, Matt Menke wrote: > Suggest documenting the lifetime of |response_info|. Looking at HttpStream, the > expected lifetime isn't terribly clear there, either. It doesn't look to be > written to after the headers are read, however, the HttpStream and the parser > both continue to keep a copy of the pointer until destruction. > > Suggest just saying it must remain valid until the stream is destroyed. Sure, done. https://chromiumcodereview.appspot.com/10808073/diff/6001/net/websockets/webs... net/websockets/websocket_stream.h:83: // should not be called while previous call of WriteFrames() is still pending. On 2012/08/06 16:15:37, Matt Menke wrote: > nit: "while the previous call" Done.
Will: ping?
On 2012/08/08 12:23:41, Yuta Kitamura wrote: > Will: ping? Turns out will is on vacation - I hadn't known, and am not sure when he'll be getting back. Think you're safe landing it and revising it later if he has any issues to raise, if this is blocking you.
On 2012/08/08 16:54:34, Matt Menke wrote: > Turns out will is on vacation - I hadn't known, and am not sure when he'll be > getting back. Think you're safe landing it and revising it later if he has any > issues to raise, if this is blocking you. Got it, landing this patch now. I will fix any issue raised later.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yutak@chromium.org/10808073/11001
Change committed as 150713 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
