Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17610006:
Add WebSocketStream factory function (Closed)
|
Created:
7 years, 6 months ago by Adam Rice Modified:
7 years, 5 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@web_socket_net Visibility:
Public. |
DescriptionChange the interface of WebSocketStream to suit WebSocketChannel, in particular adding a factory function.
Provide an implementation of WebSocketStream::CreateAndConnectStream
which calls NOTIMPLEMENTED();
BUG=253812
TEST=net_unittests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209675
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fixes for yhirano review. #
Total comments: 2
Patch Set 3 : Spelling fixes. #
Total comments: 6
Patch Set 4 : Rename virtual getter methods with Get prefix. #Patch Set 5 : Fix clang compile errors. #
Total comments: 18
Patch Set 6 : Add WebSocketStreamRequest declaration. #
Total comments: 14
Patch Set 7 : Changes inspired by mmenke's review #
Total comments: 4
Patch Set 8 : Nit fixes to WebSocketStream. #Patch Set 9 : Remove unnecessary call to base class constructor. #Patch Set 10 : Add missing ConnectDelegate destructor #
Total comments: 1
Patch Set 11 : Rebase against ToT #Patch Set 12 : Fix bad net.gyp diff. #
Messages
Total messages: 22 (0 generated)
I think the issue title should be a bit more specific... https://codereview.chromium.org/17610006/diff/1/net/websockets/websocket_stre... File net/websockets/websocket_stream.h (right): https://codereview.chromium.org/17610006/diff/1/net/websockets/websocket_stre... net/websockets/websocket_stream.h:97: // This function should not be called while the previous call of ReadFrames() Can this function be called before ReadHandshakeResponse? https://codereview.chromium.org/17610006/diff/1/net/websockets/websocket_stre... net/websockets/websocket_stream.h:103: // The callback will not be called until the entire frame header is How about "This function will not complete until the entire frame header is available."? https://codereview.chromium.org/17610006/diff/1/net/websockets/websocket_stre... net/websockets/websocket_stream.h:130: // This function should not be called while previous call of WriteFrames() is Can this function be called before ReadHandshakeResponse? https://codereview.chromium.org/17610006/diff/1/net/websockets/websocket_stre... net/websockets/websocket_stream.h:143: // TODO(ricea): Is this actually needed? Isn't it? I think it is necessary.
I have tried to make the CL title clearer. https://codereview.chromium.org/17610006/diff/1/net/websockets/websocket_stre... File net/websockets/websocket_stream.h (right): https://codereview.chromium.org/17610006/diff/1/net/websockets/websocket_stre... net/websockets/websocket_stream.h:97: // This function should not be called while the previous call of ReadFrames() On 2013/06/25 07:51:40, yhirano wrote: > Can this function be called before ReadHandshakeResponse? No, I have moved ReadHandshakeResponse() and SendHandshakeResponse() down below to try to make it clear that they are not used after the session is established. https://codereview.chromium.org/17610006/diff/1/net/websockets/websocket_stre... net/websockets/websocket_stream.h:103: // The callback will not be called until the entire frame header is On 2013/06/25 07:51:40, yhirano wrote: > How about "This function will not complete until the entire frame header is > available."? I have tried to make the comment clearer. Please take another look. https://codereview.chromium.org/17610006/diff/1/net/websockets/websocket_stre... net/websockets/websocket_stream.h:130: // This function should not be called while previous call of WriteFrames() is On 2013/06/25 07:51:40, yhirano wrote: > Can this function be called before ReadHandshakeResponse? No, but WebSocketFactory will not attempt to call it so it should not be a problem. https://codereview.chromium.org/17610006/diff/1/net/websockets/websocket_stre... net/websockets/websocket_stream.h:143: // TODO(ricea): Is this actually needed? On 2013/06/25 07:51:40, yhirano wrote: > Isn't it? I think it is necessary. Yes, I wrote that comment before implementing WebSocketChannel. WebSocketChannel uses Close() when there is an IO error. In normal operation the channel is always closed from the server side, so this method is only used on error.
lgtm https://codereview.chromium.org/17610006/diff/5001/net/websockets/websocket_s... File net/websockets/websocket_stream.h (right): https://codereview.chromium.org/17610006/diff/5001/net/websockets/websocket_s... net/websockets/websocket_stream.h:90: // Every WebSocketFrameChunk in the vector except the firat and last is first?
+tyoshino for OWNER review. https://codereview.chromium.org/17610006/diff/5001/net/websockets/websocket_s... File net/websockets/websocket_stream.h (right): https://codereview.chromium.org/17610006/diff/5001/net/websockets/websocket_s... net/websockets/websocket_stream.h:90: // Every WebSocketFrameChunk in the vector except the firat and last is On 2013/06/26 01:27:51, yhirano wrote: > first? Oops. Fixed.
LGTM https://codereview.chromium.org/17610006/diff/10001/net/websockets/websocket_... File net/websockets/websocket_stream.cc (right): https://codereview.chromium.org/17610006/diff/10001/net/websockets/websocket_... net/websockets/websocket_stream.cc:12: : WebSocketStreamBase() {} 4 sp indent https://codereview.chromium.org/17610006/diff/10001/net/websockets/websocket_... File net/websockets/websocket_stream.h (right): https://codereview.chromium.org/17610006/diff/10001/net/websockets/websocket_... net/websockets/websocket_stream.h:131: virtual std::string Protocol() = 0; GetProtocol? https://codereview.chromium.org/17610006/diff/10001/net/websockets/websocket_... net/websockets/websocket_stream.h:136: virtual std::string Extensions() = 0; GetExtensions?
+mmenke for OWNERS review on net.gyp. https://codereview.chromium.org/17610006/diff/10001/net/websockets/websocket_... File net/websockets/websocket_stream.cc (right): https://codereview.chromium.org/17610006/diff/10001/net/websockets/websocket_... net/websockets/websocket_stream.cc:12: : WebSocketStreamBase() {} On 2013/06/26 04:05:10, tyoshino wrote: > 4 sp indent clang-format reformatted this onto on line, so I stuck with that. https://codereview.chromium.org/17610006/diff/10001/net/websockets/websocket_... File net/websockets/websocket_stream.h (right): https://codereview.chromium.org/17610006/diff/10001/net/websockets/websocket_... net/websockets/websocket_stream.h:131: virtual std::string Protocol() = 0; On 2013/06/26 04:05:10, tyoshino wrote: > GetProtocol? Done. https://codereview.chromium.org/17610006/diff/10001/net/websockets/websocket_... net/websockets/websocket_stream.h:136: virtual std::string Extensions() = 0; On 2013/06/26 04:05:10, tyoshino wrote: > GetExtensions? Done.
https://codereview.chromium.org/17610006/diff/18001/net/websockets/websocket_... File net/websockets/websocket_stream.h (right): https://codereview.chromium.org/17610006/diff/18001/net/websockets/websocket_... net/websockets/websocket_stream.h:49: typedef base::Callback<void(unsigned short)> FailureCallback; Rather than having two callbacks, I suggest a delegate interface - that's more common in the network stack, when we have multiple callbacks. https://codereview.chromium.org/17610006/diff/18001/net/websockets/websocket_... net/websockets/websocket_stream.h:59: const std::vector<std::string>& requested_protocols, subprotocols, maybe? (Particularly for those who, not being familiar with the spec, turn to Google..."websocket protocol" doesn't work too well). https://codereview.chromium.org/17610006/diff/18001/net/websockets/websocket_... net/websockets/websocket_stream.h:63: const FailureCallback& on_failure); There doesn't seem to be any way to cancel this. https://codereview.chromium.org/17610006/diff/18001/net/websockets/websocket_... net/websockets/websocket_stream.h:63: const FailureCallback& on_failure); We're also going to want a BoundNetLog. https://codereview.chromium.org/17610006/diff/18001/net/websockets/websocket_... net/websockets/websocket_stream.h:125: // Closes the stream. All pending I/O operations (if any) are cancelled Poor canceled, so unloved, though the correct American spelling (No, I don't care, either way). https://codereview.chromium.org/17610006/diff/18001/net/websockets/websocket_... net/websockets/websocket_stream.h:131: virtual std::string GetProtocol() = 0; Subprotocol? https://codereview.chromium.org/17610006/diff/18001/net/websockets/websocket_... net/websockets/websocket_stream.h:131: virtual std::string GetProtocol() = 0; virtual const std::string& GetProtocol() const? https://codereview.chromium.org/17610006/diff/18001/net/websockets/websocket_... net/websockets/websocket_stream.h:136: virtual std::string GetExtensions() = 0; virtual const std::string& GetExtensions() const? https://codereview.chromium.org/17610006/diff/18001/net/websockets/websocket_... net/websockets/websocket_stream.h:136: virtual std::string GetExtensions() = 0; Also, is this a comma delimited list without spaces, or what?
https://codereview.chromium.org/17610006/diff/18001/net/websockets/websocket_... File net/websockets/websocket_stream.h (right): https://codereview.chromium.org/17610006/diff/18001/net/websockets/websocket_... net/websockets/websocket_stream.h:49: typedef base::Callback<void(unsigned short)> FailureCallback; On 2013/06/26 20:30:38, mmenke wrote: > Rather than having two callbacks, I suggest a delegate interface - that's more > common in the network stack, when we have multiple callbacks. Done. https://codereview.chromium.org/17610006/diff/18001/net/websockets/websocket_... net/websockets/websocket_stream.h:59: const std::vector<std::string>& requested_protocols, On 2013/06/26 20:30:38, mmenke wrote: > subprotocols, maybe? (Particularly for those who, not being familiar with the > spec, turn to Google..."websocket protocol" doesn't work too well). Good point, thank you. https://codereview.chromium.org/17610006/diff/18001/net/websockets/websocket_... net/websockets/websocket_stream.h:63: const FailureCallback& on_failure); On 2013/06/26 20:30:38, mmenke wrote: > There doesn't seem to be any way to cancel this. That's... very embarrassing. I have added a simple cancellation mechanism. PTAL. https://codereview.chromium.org/17610006/diff/18001/net/websockets/websocket_... net/websockets/websocket_stream.h:63: const FailureCallback& on_failure); On 2013/06/26 20:30:38, mmenke wrote: > We're also going to want a BoundNetLog. I was sort of hoping I could get away with the net_log from the URLRequestContext. Never mind. Added. https://codereview.chromium.org/17610006/diff/18001/net/websockets/websocket_... net/websockets/websocket_stream.h:131: virtual std::string GetProtocol() = 0; On 2013/06/26 20:30:38, mmenke wrote: > Subprotocol? Done. https://codereview.chromium.org/17610006/diff/18001/net/websockets/websocket_... net/websockets/websocket_stream.h:131: virtual std::string GetProtocol() = 0; On 2013/06/26 20:30:38, mmenke wrote: > virtual const std::string& GetProtocol() const? At first I didn't use a reference simple because it's hard to make guarantees about lifetime from an abstract base class. Having thought about it a bit more, I don't think there's a lifetime issue, because any implementation is going to have to keep the information around. I would still like to keep the option of storing it in something other than a std::string, at least until we have several working implementations. I made the method const as you suggested, since it is an accessor. https://codereview.chromium.org/17610006/diff/18001/net/websockets/websocket_... net/websockets/websocket_stream.h:136: virtual std::string GetExtensions() = 0; On 2013/06/26 20:30:38, mmenke wrote: > virtual const std::string& GetExtensions() const? See my justification above for keeping return-by-value semantics for the time being. https://codereview.chromium.org/17610006/diff/18001/net/websockets/websocket_... net/websockets/websocket_stream.h:136: virtual std::string GetExtensions() = 0; On 2013/06/26 20:30:38, mmenke wrote: > Also, is this a comma delimited list without spaces, or what? I added a clarifying comment. Let me know if it actually clarifies anything.
https://codereview.chromium.org/17610006/diff/18001/net/websockets/websocket_... File net/websockets/websocket_stream.h (right): https://codereview.chromium.org/17610006/diff/18001/net/websockets/websocket_... net/websockets/websocket_stream.h:63: const FailureCallback& on_failure); On 2013/06/27 15:10:49, Adam Rice wrote: > On 2013/06/26 20:30:38, mmenke wrote: > > We're also going to want a BoundNetLog. > > I was sort of hoping I could get away with the net_log from the > URLRequestContext. Never mind. Added. We need the BoundNetLog to say "This is the thing that issued the WebSocket request". https://codereview.chromium.org/17610006/diff/25001/net/websockets/websocket_... File net/websockets/websocket_stream.h (right): https://codereview.chromium.org/17610006/diff/25001/net/websockets/websocket_... net/websockets/websocket_stream.h:65: }; Do we even need ConnectDelegate or WebSocketStreamRequest? We have HttpStreamRequest and HttpStreamRequest::Delegate, both of which now handle WebSockets. Seems like we can just take in an HttpStreamRequest::Delegate and return an HttpStreamRequest. Or is there some subtlety in the difference between a WebSocketStream and a WebSocketStreamBase that I'm missing? I don't think we get a whole lot from the separation of the two classes. With this arrangement, we'd presumably need to duplicate the SSL cert/proxy auth handlers HttpStreamRequest::Delegate has for WebSocketStreamRequest as well, if we want to handle those cases.
[+szym] Who will be taking over the WebSocket reviews for me.
https://codereview.chromium.org/17610006/diff/25001/net/websockets/websocket_... File net/websockets/websocket_stream.h (right): https://codereview.chromium.org/17610006/diff/25001/net/websockets/websocket_... net/websockets/websocket_stream.h:65: }; On 2013/06/27 15:25:09, mmenke wrote: > Do we even need ConnectDelegate or WebSocketStreamRequest? We have > HttpStreamRequest and HttpStreamRequest::Delegate, both of which now handle > WebSockets. Seems like we can just take in an HttpStreamRequest::Delegate and > return an HttpStreamRequest. > > Or is there some subtlety in the difference between a WebSocketStream and a > WebSocketStreamBase that I'm missing? I don't think we get a whole lot from the > separation of the two classes. > > With this arrangement, we'd presumably need to duplicate the SSL cert/proxy auth > handlers HttpStreamRequest::Delegate has for WebSocketStreamRequest as well, if > we want to handle those cases. To be honest, I'm not sure at the moment. In the case where we start a new logical channel on an existing physical multiplexed WebSocketStream, I am guessing that we won't use HttpStreamRequest at all. WebSockets will only use existing authentication details or client certificates that the user has established with HTTP/HTTPS connections to the same site or proxy. We will never popup UI saying "Enter password" or "Select certificate". I think this means that the SSL cert/proxy auth handler stuff can be kept out of the higher-level APIs.
https://codereview.chromium.org/17610006/diff/25001/net/websockets/websocket_... File net/websockets/websocket_stream.cc (right): https://codereview.chromium.org/17610006/diff/25001/net/websockets/websocket_... net/websockets/websocket_stream.cc:13: WebSocketStream::WebSocketStream() : WebSocketStreamBase() {} nit: ": WebSocketStreamBase() " not needed. https://codereview.chromium.org/17610006/diff/25001/net/websockets/websocket_... File net/websockets/websocket_stream.h (right): https://codereview.chromium.org/17610006/diff/25001/net/websockets/websocket_... net/websockets/websocket_stream.h:34: public: nit: indent 1 https://codereview.chromium.org/17610006/diff/25001/net/websockets/websocket_... net/websockets/websocket_stream.h:65: }; On 2013/06/27 16:27:08, Adam Rice wrote: > On 2013/06/27 15:25:09, mmenke wrote: > > Do we even need ConnectDelegate or WebSocketStreamRequest? We have > > HttpStreamRequest and HttpStreamRequest::Delegate, both of which now handle > > WebSockets. Seems like we can just take in an HttpStreamRequest::Delegate and > > return an HttpStreamRequest. > > > > Or is there some subtlety in the difference between a WebSocketStream and a > > WebSocketStreamBase that I'm missing? I don't think we get a whole lot from > the > > separation of the two classes. > > > > With this arrangement, we'd presumably need to duplicate the SSL cert/proxy > auth > > handlers HttpStreamRequest::Delegate has for WebSocketStreamRequest as well, > if > > we want to handle those cases. > > To be honest, I'm not sure at the moment. In the case where we start a new > logical channel on an existing physical multiplexed WebSocketStream, I am > guessing that we won't use HttpStreamRequest at all. > > WebSockets will only use existing authentication details or client certificates > that the user has established with HTTP/HTTPS connections to the same site or > proxy. We will never popup UI saying "Enter password" or "Select certificate". I > think this means that the SSL cert/proxy auth handler stuff can be kept out of > the higher-level APIs. Good point. May want to think about modelling things after how SpdySessionPool works, to make it easier for people who deal with both classes. https://codereview.chromium.org/17610006/diff/25001/net/websockets/websocket_... net/websockets/websocket_stream.h:71: // WebSocketStream. If it failed, then the on_failure callback is called with "on_success callback" -> "OnSuccess", same for on_failure. https://codereview.chromium.org/17610006/diff/25001/net/websockets/websocket_... net/websockets/websocket_stream.h:137: // Otherwise it will return an appropriate ERR_ code. suggest "ERR_ code" -> "net error code" https://codereview.chromium.org/17610006/diff/25001/net/websockets/websocket_... net/websockets/websocket_stream.h:151: // WebSocketStream implements. The primary purpose of this accessor is to make So WebSocketStream::GetExtensions is not what the WebSocketStream implements, but rather what the WebSocketSession (Or whatever we choose to call it) implements? Suggest renaming it to something along the lines of GetExtensionsForSession(). https://codereview.chromium.org/17610006/diff/25001/net/websockets/websocket_... net/websockets/websocket_stream.h:158: virtual std::string GetExtensions() = 0; Didn't you say you were going to make this and GetSubProtocol() const?
https://codereview.chromium.org/17610006/diff/25001/net/websockets/websocket_... File net/websockets/websocket_stream.h (right): https://codereview.chromium.org/17610006/diff/25001/net/websockets/websocket_... net/websockets/websocket_stream.h:34: public: On 2013/06/27 16:56:23, mmenke (incorrect) wrote: > nit: indent 1 Fixed in a future patch. https://codereview.chromium.org/17610006/diff/25001/net/websockets/websocket_... net/websockets/websocket_stream.h:71: // WebSocketStream. If it failed, then the on_failure callback is called with On 2013/06/27 16:56:23, mmenke (incorrect) wrote: > "on_success callback" -> "OnSuccess", same for on_failure. I rewrote this comment. https://codereview.chromium.org/17610006/diff/25001/net/websockets/websocket_... net/websockets/websocket_stream.h:151: // WebSocketStream implements. The primary purpose of this accessor is to make On 2013/06/27 16:56:23, mmenke (incorrect) wrote: > So WebSocketStream::GetExtensions is not what the WebSocketStream implements, > but rather what the WebSocketSession (Or whatever we choose to call it) > implements? > > Suggest renaming it to something along the lines of GetExtensionsForSession(). Yes. The whole thing seems like a gross layering violation to me. Maybe I should just name it GetExtensionsForJavascript(). https://codereview.chromium.org/17610006/diff/25001/net/websockets/websocket_... net/websockets/websocket_stream.h:158: virtual std::string GetExtensions() = 0; On 2013/06/27 16:56:23, mmenke (incorrect) wrote: > Didn't you say you were going to make this and GetSubProtocol() const? Yes, there was a race condition between my fingers and my brain and git won.
https://codereview.chromium.org/17610006/diff/34001/net/websockets/websocket_... File net/websockets/websocket_stream.h (right): https://codereview.chromium.org/17610006/diff/34001/net/websockets/websocket_... net/websockets/websocket_stream.h:71: // WebSocketStream implementation. If it failed, then nit: "implementation" is generally used to mean a class, not an object instance. Suggest either just removing implementation, or using "instance" instead. https://codereview.chromium.org/17610006/diff/34001/net/websockets/websocket_... net/websockets/websocket_stream.h:76: // being called. Unless cancellation is required, the caller should keep the Wait...So the WebSocketStreamRequest owns the delegate? Isn't it typically the other way around. class WebSocketStreamSendyThing : public WebSocketStream::Delegate { Start() { request_.reset(CreateAndConnectStream(this, ...)); } OnSuccess(stream) { // No longer needed. Notice that |this| outlives the request. request_.reset(); stream_.reset(stream); // Read/write data ... } ~WebSocketStreamSendyThing() { // stream_ and request_ do whatever cleanup is necessary on destruction. } }
https://codereview.chromium.org/17610006/diff/34001/net/websockets/websocket_... File net/websockets/websocket_stream.h (right): https://codereview.chromium.org/17610006/diff/34001/net/websockets/websocket_... net/websockets/websocket_stream.h:71: // WebSocketStream implementation. If it failed, then On 2013/06/27 18:09:01, mmenke (incorrect) wrote: > nit: "implementation" is generally used to mean a class, not an object > instance. Suggest either just removing implementation, or using "instance" > instead. Thanks. Done. https://codereview.chromium.org/17610006/diff/34001/net/websockets/websocket_... net/websockets/websocket_stream.h:76: // being called. Unless cancellation is required, the caller should keep the On 2013/06/27 18:09:01, mmenke (incorrect) wrote: > Wait...So the WebSocketStreamRequest owns the delegate? Isn't it typically the > other way around. > > class WebSocketStreamSendyThing : public WebSocketStream::Delegate { > Start() { > request_.reset(CreateAndConnectStream(this, ...)); > } > > OnSuccess(stream) { > // No longer needed. Notice that |this| outlives the request. > request_.reset(); > > stream_.reset(stream); > > // Read/write data > ... > } > > ~WebSocketStreamSendyThing() { > // stream_ and request_ do whatever cleanup is necessary on destruction. > } > } I'm not really comfortable clogging up my public interfaces with implementation details. Also, tying the lifetime of the delegate to the lifetime of the connect process seems extremely tidy to me. Incidentally, WebSocketStreamSendyThing is actually called WebSocketChannel and if you are curious its under early review at https://codereview.chromium.org/12764006/ .
https://codereview.chromium.org/17610006/diff/25001/net/websockets/websocket_... File net/websockets/websocket_stream.cc (right): https://codereview.chromium.org/17610006/diff/25001/net/websockets/websocket_... net/websockets/websocket_stream.cc:13: WebSocketStream::WebSocketStream() : WebSocketStreamBase() {} On 2013/06/27 16:56:23, mmenke (incorrect) wrote: > nit: ": WebSocketStreamBase() " not needed. Done.
On 2013/06/28 04:32:44, Adam Rice wrote: > https://codereview.chromium.org/17610006/diff/34001/net/websockets/websocket_... > File net/websockets/websocket_stream.h (right): > > https://codereview.chromium.org/17610006/diff/34001/net/websockets/websocket_... > net/websockets/websocket_stream.h:71: // WebSocketStream implementation. If it > failed, then > On 2013/06/27 18:09:01, mmenke (incorrect) wrote: > > nit: "implementation" is generally used to mean a class, not an object > > instance. Suggest either just removing implementation, or using "instance" > > instead. > > Thanks. Done. > > https://codereview.chromium.org/17610006/diff/34001/net/websockets/websocket_... > net/websockets/websocket_stream.h:76: // being called. Unless cancellation is > required, the caller should keep the > On 2013/06/27 18:09:01, mmenke (incorrect) wrote: > > Wait...So the WebSocketStreamRequest owns the delegate? Isn't it typically > the > > other way around. > > > > class WebSocketStreamSendyThing : public WebSocketStream::Delegate { > > Start() { > > request_.reset(CreateAndConnectStream(this, ...)); > > } > > > > OnSuccess(stream) { > > // No longer needed. Notice that |this| outlives the request. > > request_.reset(); > > > > stream_.reset(stream); > > > > // Read/write data > > ... > > } > > > > ~WebSocketStreamSendyThing() { > > // stream_ and request_ do whatever cleanup is necessary on destruction. > > } > > } > > I'm not really comfortable clogging up my public interfaces with implementation > details. Also, tying the lifetime of the delegate to the lifetime of the connect > process seems extremely tidy to me. Ah, so you're concerned about exposing the delegate in websocket_channel.h. That still doesn't make it necessary for the request to own the delegate, but looks reasonable. > Incidentally, WebSocketStreamSendyThing is actually called WebSocketChannel and > if you are curious its under early review at > https://codereview.chromium.org/12764006/ . I think you should seriously consider renaming the WebSocketChannel to WebSocketStreamSendyThing. The latter just sounds much more professional. :) LGTM.
On 2013/06/27 15:25:50, mmenke wrote: > [+szym] Who will be taking over the WebSocket reviews for me. szym, any comments, or okay to commit?
https://codereview.chromium.org/17610006/diff/49001/net/websockets/websocket_... File net/websockets/websocket_stream.h (right): https://codereview.chromium.org/17610006/diff/49001/net/websockets/websocket_... net/websockets/websocket_stream.h:87: scoped_ptr<ConnectDelegate> connect_delegate); I agree with mmenke that passing ownership of delegate like this is a bit unusual, but I do recognize the benefits. At this point, my review serves more for me to get acquainted with the websocket system than give any constructive feedback, so don't hold back for my sake. I'll take a closer look when I get back from vacation.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ricea@chromium.org/17610006/61001
Message was sent while issue was closed.
Change committed as 209675 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
