Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
243077:
Add net/socket_stream. (Closed)
|
Created:
11 years, 2 months ago by ukai Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com Visibility:
Public. |
DescriptionAdd net/socket_stream.
This is used for WebSocket protocol.
BUG=12497
TEST=none
Patch Set 1 #Patch Set 2 : Add thin helper methods on SocketStreamRequest that just forward to the underlying Job object. #
Total comments: 10
Patch Set 3 : fix yuzo's comment #
Total comments: 26
Patch Set 4 : Fix comments #Patch Set 5 : Fix tyoshino's comment #
Total comments: 12
Patch Set 6 : Fix tyoshino comment. #
Total comments: 36
Patch Set 7 : fix wtc's comment #Patch Set 8 : minor fix #
Total comments: 8
Patch Set 9 : remove SocketStreamRequest #Patch Set 10 : Fix bugs #Patch Set 11 : Add DetachDelegate and fix comments #
Total comments: 41
Patch Set 12 : Rename SocketStreamJob to SocketStream #
Total comments: 3
Messages
Total messages: 30 (0 generated)
For now, ssl won't work, because current SSLClientSocket implementations don't provide full duplex connection.
SSLClientSocket (at least the Windows version) will become full-duplex when mbelshe's CL is checked in: http://codereview.chromium.org/225005 I'm wondering if we can unify socket streams and HTTP streams. See also vandebo's CL (part of his work on HTTP pipelining) at http://codereview.chromium.org/249031
It seems like the API here is inspired by URLRequest and URLRequestJob. One of the goals of URLRequest was to hide the URLRequestJob from the consumer. It seems like SocketStreamRequest could do the same by adding some thin helper methods on SocketStreamRequest that just forward to the underlying Job object.
On 2009/10/02 14:23:48, wtc wrote: > SSLClientSocket (at least the Windows version) will become > full-duplex when mbelshe's CL is checked in: > http://codereview.chromium.org/225005 Cool! I've a patch to ssl_client_socket_nss and send it as another CL. Does anyone work for mac? > I'm wondering if we can unify socket streams and HTTP > streams. See also vandebo's CL (part of his work on > HTTP pipelining) at > http://codereview.chromium.org/249031 We could share some code (establishing connection over proxy/socks/ssl), but socket stream (used by WebSocket) doesn't use request/response model, so I think we might need some work to share the code.
On 2009/10/02 16:37:28, darin wrote: > It seems like the API here is inspired by URLRequest and URLRequestJob. One of > the goals of URLRequest was to hide the URLRequestJob from the consumer. It > seems like SocketStreamRequest could do the same by adding some thin helper > methods on SocketStreamRequest that just forward to the underlying Job object. Thanks for suggestion. Added thin helper methods.
The following are my initial comments. I'll look again later. http://codereview.chromium.org/243077/diff/3002/4002 File net/socket_stream/socket_stream_job.cc (right): http://codereview.chromium.org/243077/diff/3002/4002#newcode61 Line 61: CHECK(!request_); Are you sure we need CHECK here? I guess the use of CHECK is discouraged? http://codereview.chromium.org/243077/diff/3002/4003 File net/socket_stream/socket_stream_job.h (right): http://codereview.chromium.org/243077/diff/3002/4003#newcode53 Line 53: // Injects HostResolver. Used for unittests. If you do not want production code to call this function, it's safer to say that explicitly, e.g., "For testing purposes only". http://codereview.chromium.org/243077/diff/3002/4003#newcode57 Line 57: // Used for unittests. Ditto. http://codereview.chromium.org/243077/diff/3002/4003#newcode158 Line 158: enum { kMaxTunnelResponseHeadersSize = 32768 }; // 32 kilobytes. Can you comment on the rationale behind this number, if any? http://codereview.chromium.org/243077/diff/3002/4004 File net/socket_stream/socket_stream_request.cc (right): http://codereview.chromium.org/243077/diff/3002/4004#newcode13 Line 13: static const size_t kMaxAmountSendAllowed = 32768; // 32 kilobytes. Can you comment on the rationale behind this number, if any?
http://codereview.chromium.org/243077/diff/3002/4002 File net/socket_stream/socket_stream_job.cc (right): http://codereview.chromium.org/243077/diff/3002/4002#newcode61 Line 61: CHECK(!request_); On 2009/10/05 09:54:45, Yuzo wrote: > Are you sure we need CHECK here? > I guess the use of CHECK is discouraged? Done. http://codereview.chromium.org/243077/diff/3002/4003 File net/socket_stream/socket_stream_job.h (right): http://codereview.chromium.org/243077/diff/3002/4003#newcode53 Line 53: // Injects HostResolver. Used for unittests. On 2009/10/05 09:54:45, Yuzo wrote: > If you do not want production code to call this function, > it's safer to say that explicitly, e.g., > "For testing purposes only". > Done. http://codereview.chromium.org/243077/diff/3002/4003#newcode57 Line 57: // Used for unittests. On 2009/10/05 09:54:45, Yuzo wrote: > Ditto. Done. http://codereview.chromium.org/243077/diff/3002/4003#newcode158 Line 158: enum { kMaxTunnelResponseHeadersSize = 32768 }; // 32 kilobytes. On 2009/10/05 09:54:45, Yuzo wrote: > Can you comment on the rationale behind this number, if any? Done. http://codereview.chromium.org/243077/diff/3002/4004 File net/socket_stream/socket_stream_request.cc (right): http://codereview.chromium.org/243077/diff/3002/4004#newcode13 Line 13: static const size_t kMaxAmountSendAllowed = 32768; // 32 kilobytes. On 2009/10/05 09:54:45, Yuzo wrote: > Can you comment on the rationale behind this number, if any? pick a number that is not so small and not so big. Maybe, we should measure performance (in future).
http://codereview.chromium.org/243077/diff/1006/10 File net/socket_stream/socket_stream_job.cc (right): http://codereview.chromium.org/243077/diff/1006/10#newcode262 Line 262: } while (rv != ERR_IO_PENDING && next_state_ != STATE_NONE); Will this loop work correctly if ERR_* (except ERR_IO_PENDING) is returned from Do* methods? Please also see my comments below. http://codereview.chromium.org/243077/diff/1006/10#newcode426 Line 426: return ERR_RESPONSE_HEADERS_TOO_BIG; Don't we need next_state_ = STATE_NONE; here? Otherwise, the main loop seems to loop indefinitely. http://codereview.chromium.org/243077/diff/1006/10#newcode439 Line 439: return ERR_TUNNEL_CONNECTION_FAILED; Ditto. http://codereview.chromium.org/243077/diff/1006/11 File net/socket_stream/socket_stream_job.h (right): http://codereview.chromium.org/243077/diff/1006/11#newcode41 Line 41: // |request_|. You might want to comment what will happen in case of error (returning false). I guess the data is not sent at all. http://codereview.chromium.org/243077/diff/1006/11#newcode53 Line 53: // Injects HostResolver. For testing purpose only. s/purpose/purposes/ http://codereview.chromium.org/243077/diff/1006/11#newcode158 Line 158: // Use the same number with HttpNetworkTransaction::kMaxHeaderBufSize. s/with/as/
http://codereview.chromium.org/243077/diff/1006/10 File net/socket_stream/socket_stream_job.cc (right): http://codereview.chromium.org/243077/diff/1006/10#newcode262 Line 262: } while (rv != ERR_IO_PENDING && next_state_ != STATE_NONE); On 2009/10/06 04:09:22, Yuzo wrote: > Will this loop work correctly if ERR_* (except ERR_IO_PENDING) > is returned from Do* methods? Please also see my comments below. yes. when ERR_* (except ERR_IO_PENDING) is returned, we'll not update next_state_ (and next_state_ is STATE_NONE at the top of this loop), so this loop exits. http://codereview.chromium.org/243077/diff/1006/10#newcode426 Line 426: return ERR_RESPONSE_HEADERS_TOO_BIG; On 2009/10/06 04:09:22, Yuzo wrote: > Don't we need > next_state_ = STATE_NONE; > here? Otherwise, the main loop seems to loop indefinitely. Unnecessary. All Do* methods is expected to be called with next_state_ = STATE_NONE (I believe it is convention of code in net/) http://codereview.chromium.org/243077/diff/1006/10#newcode439 Line 439: return ERR_TUNNEL_CONNECTION_FAILED; On 2009/10/06 04:09:22, Yuzo wrote: > Ditto. Ditto. http://codereview.chromium.org/243077/diff/1006/11 File net/socket_stream/socket_stream_job.h (right): http://codereview.chromium.org/243077/diff/1006/11#newcode41 Line 41: // |request_|. On 2009/10/06 04:09:22, Yuzo wrote: > You might want to comment what will happen in case of error (returning false). I > guess the data is not sent at all. Done. http://codereview.chromium.org/243077/diff/1006/11#newcode53 Line 53: // Injects HostResolver. For testing purpose only. On 2009/10/06 04:09:22, Yuzo wrote: > s/purpose/purposes/ > > Done. http://codereview.chromium.org/243077/diff/1006/11#newcode158 Line 158: // Use the same number with HttpNetworkTransaction::kMaxHeaderBufSize. On 2009/10/06 04:09:22, Yuzo wrote: > s/with/as/ Done.
http://codereview.chromium.org/243077/diff/1006/10 File net/socket_stream/socket_stream_job.cc (right): http://codereview.chromium.org/243077/diff/1006/10#newcode122 Line 122: request_ = 0; = NULL http://codereview.chromium.org/243077/diff/1006/12 File net/socket_stream/socket_stream_request.cc (right): http://codereview.chromium.org/243077/diff/1006/12#newcode8 Line 8: #include "net/base/ssl_config_service.h" include net/proxy/proxy_service.h or remove this for consistency. Looks like only pointers of SSLConfigService and ProxyService are used. http://codereview.chromium.org/243077/diff/1006/12#newcode11 Line 11: #include "googleurl/src/gurl.h" sort http://codereview.chromium.org/243077/diff/1006/12#newcode61 Line 61: return job_->SendData(data); what does happen when SendData called after Close? job_ is still non-NULL. http://codereview.chromium.org/243077/diff/1006/13 File net/socket_stream/socket_stream_request.h (right): http://codereview.chromium.org/243077/diff/1006/13#newcode55 Line 55: // Multiple user data vlaues can be stored under different keys. values http://codereview.chromium.org/243077/diff/1006/13#newcode78 Line 78: // received by the delegate. This method may be called many times. multiple times? http://codereview.chromium.org/243077/diff/1006/13#newcode98 Line 98: typedef std::map<const void *, linked_ptr<UserData> > UserDataMap; void*
http://codereview.chromium.org/243077/diff/1006/10 File net/socket_stream/socket_stream_job.cc (right): http://codereview.chromium.org/243077/diff/1006/10#newcode122 Line 122: request_ = 0; On 2009/10/06 06:54:38, tyoshino wrote: > = NULL Done. http://codereview.chromium.org/243077/diff/1006/12 File net/socket_stream/socket_stream_request.cc (right): http://codereview.chromium.org/243077/diff/1006/12#newcode8 Line 8: #include "net/base/ssl_config_service.h" On 2009/10/06 06:54:38, tyoshino wrote: > include net/proxy/proxy_service.h or remove this for consistency. Looks like > only pointers of SSLConfigService and ProxyService are used. Done. http://codereview.chromium.org/243077/diff/1006/12#newcode11 Line 11: #include "googleurl/src/gurl.h" On 2009/10/06 06:54:38, tyoshino wrote: > sort Done. http://codereview.chromium.org/243077/diff/1006/12#newcode61 Line 61: return job_->SendData(data); On 2009/10/06 06:54:38, tyoshino wrote: > what does happen when SendData called after Close? job_ is still non-NULL. Add check in SocketStreamJob so that SendData will return false after Close. http://codereview.chromium.org/243077/diff/1006/13 File net/socket_stream/socket_stream_request.h (right): http://codereview.chromium.org/243077/diff/1006/13#newcode55 Line 55: // Multiple user data vlaues can be stored under different keys. On 2009/10/06 06:54:38, tyoshino wrote: > values Done. http://codereview.chromium.org/243077/diff/1006/13#newcode78 Line 78: // received by the delegate. This method may be called many times. On 2009/10/06 06:54:38, tyoshino wrote: > multiple times? "many times" would be fine (e.g. url_request/url_request.h Cancel() method) http://codereview.chromium.org/243077/diff/1006/13#newcode98 Line 98: typedef std::map<const void *, linked_ptr<UserData> > UserDataMap; On 2009/10/06 06:54:38, tyoshino wrote: > void* Done.
http://codereview.chromium.org/243077/diff/7002/5009 File net/socket_stream/socket_stream_job.cc (right): http://codereview.chromium.org/243077/diff/7002/5009#newcode10 Line 10: #include "base/logging.h" Add #include "base/message_loop.h" http://codereview.chromium.org/243077/diff/7002/5009#newcode112 Line 112: return; indentation http://codereview.chromium.org/243077/diff/7002/5009#newcode454 Line 454: break; let's have a default: case. and put the return statement in it. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit... http://codereview.chromium.org/243077/diff/7002/5009#newcode557 Line 557: if (result == OK) Please add some comment to explain in what case we arrive here with |result| == OK. http://codereview.chromium.org/243077/diff/7002/5009#newcode569 Line 569: break; ditto L454 http://codereview.chromium.org/243077/diff/7002/5010 File net/socket_stream/socket_stream_job.h (right): http://codereview.chromium.org/243077/diff/7002/5010#newcode102 Line 102: STATE_CONNECTING, Hmm, STATE_CONNECTING sounds a bit odd. How about using STATE_READY or something? Rather can't we combine the last two states?
LGTM
On 2009/10/02 16:37:28, darin wrote: > It seems like the API here is inspired by URLRequest and URLRequestJob. Fumitoshi, is this true? I have the same question when I see the _request and _job in the file names.
On 2009/10/05 07:06:50, ukai wrote: > > I've a patch to ssl_client_socket_nss and send it as another CL. Does > anyone work for mac? No, no one is working on making ssl_client_socket_mac full-duplex. Re: sharing code with HTTP streams: please ignore that suggestion. I asked that question simply because I saw the word "stream" in both HTTP streams and socket streams. I know they're unrelated in spite of the similar names.
I haven't reviewed socket_stream_job.cc carefully. Here are my comments. http://codereview.chromium.org/243077/diff/8002/2006 File net/socket_stream/socket_stream_job.cc (right): http://codereview.chromium.org/243077/diff/8002/2006#newcode1 Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved. A lot of code in this file is copied from http_network_transaction.cc. We should think about ways to share code, if possible. http://codereview.chromium.org/243077/diff/8002/2006#newcode70 Line 70: MessageLoop::current()->PostTask( Why do we need to do this? Same question for the PostTask calls in SendData() and Close() below. http://codereview.chromium.org/243077/diff/8002/2007 File net/socket_stream/socket_stream_job.h (right): http://codereview.chromium.org/243077/diff/8002/2007#newcode53 Line 53: // Injects HostResolver. For testing purposes only. I think Sets an alternative HostResolver is more clear than Injects HostResolver. http://codereview.chromium.org/243077/diff/8002/2007#newcode56 Line 56: // Injects ClientSocketFactory. Doesn't take ownership of |factory|. Ditto. http://codereview.chromium.org/243077/diff/8002/2008 File net/socket_stream/socket_stream_request.cc (right): http://codereview.chromium.org/243077/diff/8002/2008#newcode28 Line 28: SocketStreamRequest::UserData* SocketStreamRequest::GetUserData( See if you can get rid of the SocketStreamRequest:: prefix for UserData. http://codereview.chromium.org/243077/diff/8002/2008#newcode54 Line 54: return; Nit: delete this return statement. http://codereview.chromium.org/243077/diff/8002/2008#newcode59 Line 59: return false; The comments in socket_stream_request.h says that SendData() "returns false if buffer size exceeds max buffer size." But that's not the case here when job_ is NULL. http://codereview.chromium.org/243077/diff/8002/2009 File net/socket_stream/socket_stream_request.h (right): http://codereview.chromium.org/243077/diff/8002/2009#newcode25 Line 25: // The lifetime of an instance of this class is completely controlled by the Remove the extra "the" at the end of this line. http://codereview.chromium.org/243077/diff/8002/2009#newcode32 Line 32: // SetUserData(). Should we include the parameters here too? SetUserData(key, data) http://codereview.chromium.org/243077/diff/8002/2009#newcode37 Line 37: }; Nit: add a blank line after this line. http://codereview.chromium.org/243077/diff/8002/2009#newcode42 Line 42: virtual void OnConnected(SocketStreamRequest* request, Please document these four callbacks. At least document the max_amount_send_allowed parameter of OnConnected. I couldn't figure out what it means by just looking at this class definition. http://codereview.chromium.org/243077/diff/8002/2009#newcode44 Line 44: virtual void OnDataSent(SocketStreamRequest* request, I suggest renaming these two methods OnSentData OnReceivedData http://codereview.chromium.org/243077/diff/8002/2009#newcode48 Line 48: virtual void OnClosed(SocketStreamRequest* request) = 0; Nit: rename this method OnClose to be consistent with all the existing OnClose methods in our source tree. http://codereview.chromium.org/243077/diff/8002/2009#newcode63 Line 63: size_t max_amount_send_allowed() const { return max_amount_send_allowed_; } This returns a size_t, but the same-named parameter for Delegate::OnConnected is an int. Can we use the same type? http://codereview.chromium.org/243077/diff/8002/2009#newcode72 Line 72: // The delegate will receive a OnConnected callback when the connection is Nit: a => an Nit: Start() and OnConnected() don't seem consistent. How about Connect() and OnConnected() or Start() and OnStarted() ? http://codereview.chromium.org/243077/diff/8002/2009#newcode78 Line 78: // received by the delegate. This method may be called many times. I agree that "multiple times" sounds better than "many times". http://codereview.chromium.org/243077/diff/8002/2009#newcode80 Line 80: // Returns false if buffer size exceeds max buffer size. Did you mean data size exceeds max buffer size ? http://codereview.chromium.org/243077/diff/8002/2009#newcode81 Line 81: bool SendData(const std::vector<char>& data); Why use a std::vector? We usually use const char* data, int data_len Why is there no ReceiveData method?
http://codereview.chromium.org/243077/diff/7002/5009 File net/socket_stream/socket_stream_job.cc (right): http://codereview.chromium.org/243077/diff/7002/5009#newcode10 Line 10: #include "base/logging.h" On 2009/10/06 09:33:07, tyoshino wrote: > Add #include "base/message_loop.h" Done. http://codereview.chromium.org/243077/diff/7002/5009#newcode112 Line 112: return; On 2009/10/06 09:33:07, tyoshino wrote: > indentation Done. http://codereview.chromium.org/243077/diff/7002/5009#newcode454 Line 454: break; On 2009/10/06 09:33:07, tyoshino wrote: > let's have a default: case. and put the return statement in it. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit... Done. http://codereview.chromium.org/243077/diff/7002/5009#newcode557 Line 557: if (result == OK) On 2009/10/06 09:33:07, tyoshino wrote: > Please add some comment to explain in what case we arrive here with |result| == > OK. Done. http://codereview.chromium.org/243077/diff/7002/5009#newcode569 Line 569: break; On 2009/10/06 09:33:07, tyoshino wrote: > ditto L454 Done. http://codereview.chromium.org/243077/diff/7002/5010 File net/socket_stream/socket_stream_job.h (right): http://codereview.chromium.org/243077/diff/7002/5010#newcode102 Line 102: STATE_CONNECTING, On 2009/10/06 09:33:07, tyoshino wrote: > Hmm, STATE_CONNECTING sounds a bit odd. How about using STATE_READY or > something? Rather can't we combine the last two states? make it STATE_READ_WRITE. STATE_NONE is used to close the connection.
On 2009/10/06 21:27:44, wtc wrote: > On 2009/10/02 16:37:28, darin wrote: > > It seems like the API here is inspired by URLRequest and URLRequestJob. > > Fumitoshi, is this true? > > I have the same question when I see the _request and _job > in the file names. Yes. Initially I thought I could use URLRequest and URLRequestJob with some modification, but it seems it would be simpler to develop own request/job for socket stream. (at least for now. we might want to refactor to integrate them in future.)
Thanks for review. http://codereview.chromium.org/243077/diff/8002/2006 File net/socket_stream/socket_stream_job.cc (right): http://codereview.chromium.org/243077/diff/8002/2006#newcode1 Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved. On 2009/10/07 02:27:01, wtc wrote: > A lot of code in this file is copied from > http_network_transaction.cc. We should think about ways > to share code, if possible. I agree, but I'd like to leave it TODO now. http://codereview.chromium.org/243077/diff/8002/2006#newcode70 Line 70: MessageLoop::current()->PostTask( On 2009/10/07 02:27:01, wtc wrote: > Why do we need to do this? > > Same question for the PostTask calls in SendData() and > Close() below. I'd like to let these methods asynchronous (in other words, I don't want to call callbacks in DoLoop called from these methods) http://codereview.chromium.org/243077/diff/8002/2007 File net/socket_stream/socket_stream_job.h (right): http://codereview.chromium.org/243077/diff/8002/2007#newcode53 Line 53: // Injects HostResolver. For testing purposes only. On 2009/10/07 02:27:01, wtc wrote: > I think > Sets an alternative HostResolver > is more clear than > Injects HostResolver. Done. http://codereview.chromium.org/243077/diff/8002/2007#newcode56 Line 56: // Injects ClientSocketFactory. Doesn't take ownership of |factory|. On 2009/10/07 02:27:01, wtc wrote: > Ditto. Done. http://codereview.chromium.org/243077/diff/8002/2008 File net/socket_stream/socket_stream_request.cc (right): http://codereview.chromium.org/243077/diff/8002/2008#newcode28 Line 28: SocketStreamRequest::UserData* SocketStreamRequest::GetUserData( On 2009/10/07 02:27:01, wtc wrote: > See if you can get rid of the SocketStreamRequest:: prefix > for UserData. compile error: expected constructor, destructor, or type conversion before '*' token http://codereview.chromium.org/243077/diff/8002/2008#newcode54 Line 54: return; On 2009/10/07 02:27:01, wtc wrote: > Nit: delete this return statement. Done. http://codereview.chromium.org/243077/diff/8002/2008#newcode59 Line 59: return false; On 2009/10/07 02:27:01, wtc wrote: > The comments in socket_stream_request.h says that > SendData() "returns false if buffer size exceeds max buffer size." > > But that's not the case here when job_ is NULL. Ah, it returns false if socket is closed, too. http://codereview.chromium.org/243077/diff/8002/2009 File net/socket_stream/socket_stream_request.h (right): http://codereview.chromium.org/243077/diff/8002/2009#newcode25 Line 25: // The lifetime of an instance of this class is completely controlled by the On 2009/10/07 02:27:01, wtc wrote: > Remove the extra "the" at the end of this line. Done. http://codereview.chromium.org/243077/diff/8002/2009#newcode32 Line 32: // SetUserData(). On 2009/10/07 02:27:01, wtc wrote: > Should we include the parameters here too? > SetUserData(key, data) Done. http://codereview.chromium.org/243077/diff/8002/2009#newcode37 Line 37: }; On 2009/10/07 02:27:01, wtc wrote: > Nit: add a blank line after this line. Done. http://codereview.chromium.org/243077/diff/8002/2009#newcode42 Line 42: virtual void OnConnected(SocketStreamRequest* request, On 2009/10/07 02:27:01, wtc wrote: > Please document these four callbacks. At least document > the max_amount_send_allowed parameter of OnConnected. I > couldn't figure out what it means by just looking at this > class definition. Done. http://codereview.chromium.org/243077/diff/8002/2009#newcode44 Line 44: virtual void OnDataSent(SocketStreamRequest* request, On 2009/10/07 02:27:01, wtc wrote: > I suggest renaming these two methods > OnSentData > OnReceivedData Done. http://codereview.chromium.org/243077/diff/8002/2009#newcode48 Line 48: virtual void OnClosed(SocketStreamRequest* request) = 0; On 2009/10/07 02:27:01, wtc wrote: > Nit: rename this method OnClose to be consistent with > all the existing OnClose methods in our source tree. Done. http://codereview.chromium.org/243077/diff/8002/2009#newcode63 Line 63: size_t max_amount_send_allowed() const { return max_amount_send_allowed_; } On 2009/10/07 02:27:01, wtc wrote: > This returns a size_t, but the same-named parameter for > Delegate::OnConnected is an int. Can we use the same type? Done. http://codereview.chromium.org/243077/diff/8002/2009#newcode72 Line 72: // The delegate will receive a OnConnected callback when the connection is On 2009/10/07 02:27:01, wtc wrote: > Nit: a => an > > Nit: Start() and OnConnected() don't seem consistent. > How about > Connect() and OnConnected() > or > Start() and OnStarted() > ? Done. http://codereview.chromium.org/243077/diff/8002/2009#newcode78 Line 78: // received by the delegate. This method may be called many times. On 2009/10/07 02:27:01, wtc wrote: > I agree that "multiple times" sounds better than "many times". Done. http://codereview.chromium.org/243077/diff/8002/2009#newcode80 Line 80: // Returns false if buffer size exceeds max buffer size. On 2009/10/07 02:27:01, wtc wrote: > Did you mean > data size exceeds max buffer size > ? it means pending data size exceeds max_pending_send_allowed. http://codereview.chromium.org/243077/diff/8002/2009#newcode81 Line 81: bool SendData(const std::vector<char>& data); On 2009/10/07 02:27:01, wtc wrote: > Why use a std::vector? We usually use > const char* data, int data_len Ok. changed. > Why is there no ReceiveData method? Once socket is connected, the job start reading automatically.
http://codereview.chromium.org/243077/diff/15007/16003 File net/socket_stream/socket_stream_job.cc (right): http://codereview.chromium.org/243077/diff/15007/16003#newcode84 Line 84: return false; indentation http://codereview.chromium.org/243077/diff/15007/16003#newcode260 Line 260: case STATE_CONNECTION_ESTABLISHED: Here, rv is assured to be OK, I believe. http://codereview.chromium.org/243077/diff/15007/16003#newcode549 Line 549: OnReadCompleted(result); This invokes recursive call on DoLoop to make the state transition complicated. For now it looks like no problem, but I think we should avoid this if we can. What do you think? http://codereview.chromium.org/243077/diff/15007/16003#newcode561 Line 561: OnWriteCompleted(result); ditto
Fumitoshi, I didn't have time to review the new Patch Set today. Since this is new code, please feel free to check it in with LGTM from Yuzo and tyoshino. I will try to review the new Patch Set tomorrow. Seeing how simple socket_stream_request.cc is, I feel that SocketStreamRequest and SocketStreamJob should be merged. URLRequest and URLRequestJob were written before I started working on the network stack, so I don't know the original reason for having a Request and a Job class. I can see two reasons for having two classes: 1. We need a different type of Job for each protocol (HttpJob, FtpJob, etc.). 2. When following an HTTP redirect transparently, we need to destroy the new Job and create a new Job. I suspect that these two reasons don't hold for WebSocket, and therefore you don't need a Request and a Job class.
On 2009/10/08 01:48:51, wtc wrote: > Fumitoshi, > > I didn't have time to review the new Patch Set today. > Since this is new code, please feel free to check it in > with LGTM from Yuzo and tyoshino. I will try to review > the new Patch Set tomorrow. > > Seeing how simple socket_stream_request.cc is, I feel > that SocketStreamRequest and SocketStreamJob should be > merged. > > URLRequest and URLRequestJob were written before I started > working on the network stack, so I don't know the original > reason for having a Request and a Job class. I can see > two reasons for having two classes: > 1. We need a different type of Job for each protocol > (HttpJob, FtpJob, etc.). > 2. When following an HTTP redirect transparently, we need > to destroy the new Job and create a new Job. > > I suspect that these two reasons don't hold for WebSocket, > and therefore you don't need a Request and a Job class. Thanks for suggestion. I removed SocketStreamRequest and merged it into SocketStreamJob.
http://codereview.chromium.org/243077/diff/15007/16003 File net/socket_stream/socket_stream_job.cc (right): http://codereview.chromium.org/243077/diff/15007/16003#newcode84 Line 84: return false; On 2009/10/07 10:04:25, tyoshino wrote: > indentation Done. http://codereview.chromium.org/243077/diff/15007/16003#newcode260 Line 260: case STATE_CONNECTION_ESTABLISHED: On 2009/10/07 10:04:25, tyoshino wrote: > Here, rv is assured to be OK, I believe. Add DCHECK in DoConnectionEstablished() http://codereview.chromium.org/243077/diff/15007/16003#newcode549 Line 549: OnReadCompleted(result); On 2009/10/07 10:04:25, tyoshino wrote: > This invokes recursive call on DoLoop to make the state transition complicated. > For now it looks like no problem, but I think we should avoid this if we can. > What do you think? Factor out OnReceivedData from OnReadCompleted http://codereview.chromium.org/243077/diff/15007/16003#newcode561 Line 561: OnWriteCompleted(result); On 2009/10/07 10:04:25, tyoshino wrote: > ditto Factor out OnSentData from OnWriteCompleted
LGTM. Please check this in after addressing my comments below. I don't need to review a new Patch Set. Feel free to ignore the suggestions that don't make sense to you. http://codereview.chromium.org/243077/diff/17002/17004 File net/socket_stream/socket_stream_job.cc (right): http://codereview.chromium.org/243077/diff/17002/17004#newcode92 Line 92: AddRef(); // Released in FinishJob() Please add a comment to explain why you're doing PostTask in Connect, SendData, and Close. http://codereview.chromium.org/243077/diff/17002/17004#newcode95 Line 95: NewRunnableMethod(this, &SocketStreamJob::OnConnect)); OnConnect simply sets next_state_ and calls DoLoop. Can we just set next_state_ here and post a task to call DoLoop, as we do in SendData and Close? http://codereview.chromium.org/243077/diff/17002/17004#newcode148 Line 148: delegate_ = NULL; So we don't want the delegate to receive the OnClose callback? http://codereview.chromium.org/243077/diff/17002/17004#newcode186 Line 186: return; It seems that we should also clear read_buf_ in this case, right? If so, we can write this method like this: read_buf_ = NULL; if (delegate_) delegate_->OnReceivedData(this, read_buf_->data(), result); http://codereview.chromium.org/243077/diff/17002/17004#newcode220 Line 220: void SocketStreamJob::OnReadCompleted(int result) { Are you sure you don't want to notify the delegate of read and write errors? Nit: we can remove one DoLoop call like this: if (result >= 0 && read_buf_) { OnReceivedData(result); result = OK; } DoLoop(result); Not sure if this is better though... http://codereview.chromium.org/243077/diff/17002/17004#newcode243 Line 243: int rv = result; Why do we need this local variable 'rv'? Is because you don't like to modify an input parameter (result)? Just curious... http://codereview.chromium.org/243077/diff/17002/17004#newcode306 Line 306: rv = ERR_FAILED; Let's use ERR_UNEXPECTED here because it's a bug if we ever reach here. http://codereview.chromium.org/243077/diff/17002/17004#newcode365 Line 365: int SocketStreamJob::DoResolveHostComplete(int result) { Nit: this method can be written as: if (result == OK) next_state_ = STATE_INIT_CONNECTION; return result; http://codereview.chromium.org/243077/diff/17002/17004#newcode544 Line 544: if (result != OK) Nit: write lines 544-548 as if (result == OK) next_state_ = STATE_CONNECTION_ESTABLISHED; return result; http://codereview.chromium.org/243077/diff/17002/17004#newcode588 Line 588: write_buf_, write_buf_size_); Nit: it seems that this can fit on the previous line. http://codereview.chromium.org/243077/diff/17002/17005 File net/socket_stream/socket_stream_job.h (right): http://codereview.chromium.org/243077/diff/17002/17005#newcode28 Line 28: class ProxyService; The forward declarations of ProxyService and SSLConfigService are not necessary because you include proxy_service.h and ssl_config_service.h. It seems that a forward declaration of SSLConfigService is enough for this header, but you may need to include proxy_service.h because of the ProxyService::PacRequest type. http://codereview.chromium.org/243077/diff/17002/17005#newcode32 Line 32: class SocketStreamJob : public base::RefCountedThreadSafe<SocketStreamJob> { It seems that this class can be simply named SocketStream, unless you are going to issue multiple jobs over the same socket stream. http://codereview.chromium.org/243077/diff/17002/17005#newcode88 Line 88: // Returns false if buffer size exceeds |max_pending_send_allowed_| Nit: I suggest you change buffer size exceeds to size of buffered data would exceed http://codereview.chromium.org/243077/diff/17002/17005#newcode125 Line 125: void set_data(size_t offset) { data_ = headers_.get() + offset; } Nit: this method is equivalent to the SetDataOffset method above, but the names are different. Would be nice to use the same name. http://codereview.chromium.org/243077/diff/17002/17005#newcode139 Line 139: STATE_INIT_CONNECTION, Please rename these two states STATE_TCP_CONNECT and STATE_TCP_CONNECT_COMPLETE, and the corresponding methods DoTCPConnect and DoTCPConnectComplete. In HttpNetworkTransaction, STATE_INIT_CONNECTION is for obtaining a connection from the connection pool, subject to the max. six connections per server limit. That's not the case here. http://codereview.chromium.org/243077/diff/17002/17005#newcode149 Line 149: STATE_CONNECTION_ESTABLISHED, The STATE_CONNECTION_ESTABLISHED state is not necessary. In general we should only have a state if it initiates an async operation, or we may go back to that state later. I suggest that you rename DoConnectionEstablished as DidEstablishConnection (we use this DidDoSomething naming convention elsewhere) and just call DidEstablishConnection directly, rather than via DoLoop. http://codereview.chromium.org/243077/diff/17002/17005#newcode151 Line 151: STATE_NONE Nit: List STATE_NONE first. http://codereview.chromium.org/243077/diff/17002/17005#newcode166 Line 166: void FinishJob(); Nit: we can shorten this name to Finish because Job is implied. http://codereview.chromium.org/243077/diff/17002/17005#newcode168 Line 168: void OnConnect(); Nit: OnConnect is easily confused with the delegate's OnConnected. I suggest that we either remove this method (it's just two lines) or rename this method CompleteConnect. (We use the CompleteFoo naming convention elsewhere, but it seems to be used for completing a task on another thread though.) http://codereview.chromium.org/243077/diff/17002/17005#newcode243 Line 243: scoped_refptr<IOBuffer> write_buf_; It's not clear what's the difference between write_buf_ and current_write_buf_.
http://codereview.chromium.org/243077/diff/17002/17004 File net/socket_stream/socket_stream_job.cc (right): http://codereview.chromium.org/243077/diff/17002/17004#newcode92 Line 92: AddRef(); // Released in FinishJob() On 2009/10/08 21:38:19, wtc wrote: > Please add a comment to explain why you're doing PostTask > in Connect, SendData, and Close. Done. http://codereview.chromium.org/243077/diff/17002/17004#newcode95 Line 95: NewRunnableMethod(this, &SocketStreamJob::OnConnect)); On 2009/10/08 21:38:19, wtc wrote: > OnConnect simply sets next_state_ and calls DoLoop. > Can we just set next_state_ here and post a task to call > DoLoop, as we do in SendData and Close? Done. http://codereview.chromium.org/243077/diff/17002/17004#newcode148 Line 148: delegate_ = NULL; On 2009/10/08 21:38:19, wtc wrote: > So we don't want the delegate to receive the OnClose callback? Yes, it should be called just before deleting delegate itself, so that OnClose won't call released delegate. http://codereview.chromium.org/243077/diff/17002/17004#newcode186 Line 186: return; On 2009/10/08 21:38:19, wtc wrote: > It seems that we should also clear read_buf_ in this case, > right? If so, we can write this method like this: > read_buf_ = NULL; > if (delegate_) > delegate_->OnReceivedData(this, read_buf_->data(), result); We need hold read_buf_ while we call OnReceviedData. http://codereview.chromium.org/243077/diff/17002/17004#newcode220 Line 220: void SocketStreamJob::OnReadCompleted(int result) { On 2009/10/08 21:38:19, wtc wrote: > Are you sure you don't want to notify the delegate of > read and write errors? Ah, yes, I'll add it later. > > Nit: we can remove one DoLoop call like this: > > if (result >= 0 && read_buf_) { > OnReceivedData(result); > result = OK; > } > DoLoop(result); > > Not sure if this is better though... Done. http://codereview.chromium.org/243077/diff/17002/17004#newcode243 Line 243: int rv = result; On 2009/10/08 21:38:19, wtc wrote: > Why do we need this local variable 'rv'? Is because you > don't like to modify an input parameter (result)? Just > curious... Done. http://codereview.chromium.org/243077/diff/17002/17004#newcode306 Line 306: rv = ERR_FAILED; On 2009/10/08 21:38:19, wtc wrote: > Let's use ERR_UNEXPECTED here because it's a bug if we ever > reach here. Done. http://codereview.chromium.org/243077/diff/17002/17004#newcode365 Line 365: int SocketStreamJob::DoResolveHostComplete(int result) { On 2009/10/08 21:38:19, wtc wrote: > Nit: this method can be written as: > if (result == OK) > next_state_ = STATE_INIT_CONNECTION; > return result; Done. http://codereview.chromium.org/243077/diff/17002/17004#newcode544 Line 544: if (result != OK) On 2009/10/08 21:38:19, wtc wrote: > Nit: write lines 544-548 as > if (result == OK) > next_state_ = STATE_CONNECTION_ESTABLISHED; > return result; Done. http://codereview.chromium.org/243077/diff/17002/17004#newcode588 Line 588: write_buf_, write_buf_size_); On 2009/10/08 21:38:19, wtc wrote: > Nit: it seems that this can fit on the previous line. Done. http://codereview.chromium.org/243077/diff/17002/17005 File net/socket_stream/socket_stream_job.h (right): http://codereview.chromium.org/243077/diff/17002/17005#newcode28 Line 28: class ProxyService; On 2009/10/08 21:38:19, wtc wrote: > The forward declarations of ProxyService and SSLConfigService > are not necessary because you include proxy_service.h and > ssl_config_service.h. > > It seems that a forward declaration of SSLConfigService is > enough for this header, but you may need to include > proxy_service.h because of the ProxyService::PacRequest > type. Done. http://codereview.chromium.org/243077/diff/17002/17005#newcode32 Line 32: class SocketStreamJob : public base::RefCountedThreadSafe<SocketStreamJob> { On 2009/10/08 21:38:19, wtc wrote: > It seems that this class can be simply named SocketStream, > unless you are going to issue multiple jobs over the same > socket stream. Done. http://codereview.chromium.org/243077/diff/17002/17005#newcode88 Line 88: // Returns false if buffer size exceeds |max_pending_send_allowed_| On 2009/10/08 21:38:19, wtc wrote: > Nit: I suggest you change > buffer size exceeds > to > size of buffered data would exceed Done. http://codereview.chromium.org/243077/diff/17002/17005#newcode125 Line 125: void set_data(size_t offset) { data_ = headers_.get() + offset; } On 2009/10/08 21:38:19, wtc wrote: > Nit: this method is equivalent to the SetDataOffset method > above, but the names are different. Would be nice to use > the same name. Yes. Should we fix in ResponseHeaders in HttpNetworkTransaction as well? http://codereview.chromium.org/243077/diff/17002/17005#newcode139 Line 139: STATE_INIT_CONNECTION, On 2009/10/08 21:38:19, wtc wrote: > Please rename these two states STATE_TCP_CONNECT and > STATE_TCP_CONNECT_COMPLETE, and the corresponding methods > DoTCPConnect and DoTCPConnectComplete. > > In HttpNetworkTransaction, STATE_INIT_CONNECTION is for > obtaining a connection from the connection pool, subject > to the max. six connections per server limit. That's not > the case here. Done. http://codereview.chromium.org/243077/diff/17002/17005#newcode149 Line 149: STATE_CONNECTION_ESTABLISHED, On 2009/10/08 21:38:19, wtc wrote: > The STATE_CONNECTION_ESTABLISHED state is not necessary. > In general we should only have a state if it initiates an > async operation, or we may go back to that state later. > > I suggest that you rename DoConnectionEstablished as > DidEstablishConnection (we use this DidDoSomething naming > convention elsewhere) and just call DidEstablishConnection > directly, rather than via DoLoop. Done. http://codereview.chromium.org/243077/diff/17002/17005#newcode151 Line 151: STATE_NONE On 2009/10/08 21:38:19, wtc wrote: > Nit: List STATE_NONE first. Done. http://codereview.chromium.org/243077/diff/17002/17005#newcode166 Line 166: void FinishJob(); On 2009/10/08 21:38:19, wtc wrote: > Nit: we can shorten this name to Finish because Job is implied. Done. http://codereview.chromium.org/243077/diff/17002/17005#newcode168 Line 168: void OnConnect(); On 2009/10/08 21:38:19, wtc wrote: > Nit: OnConnect is easily confused with the delegate's > OnConnected. I suggest that we either remove this method > (it's just two lines) or rename this method CompleteConnect. > (We use the CompleteFoo naming convention elsewhere, but > it seems to be used for completing a task on another thread > though.) Removed. http://codereview.chromium.org/243077/diff/17002/17005#newcode243 Line 243: scoped_refptr<IOBuffer> write_buf_; On 2009/10/08 21:38:19, wtc wrote: > It's not clear what's the difference between write_buf_ and > current_write_buf_. Done.
LGTM
http://codereview.chromium.org/243077/diff/17002/17005 File net/socket_stream/socket_stream_job.h (right): http://codereview.chromium.org/243077/diff/17002/17005#newcode125 Line 125: void set_data(size_t offset) { data_ = headers_.get() + offset; } On 2009/10/09 04:46:37, ukai wrote: > > Should we fix in ResponseHeaders in HttpNetworkTransaction as well? Yes, we should. (I know you copied the code from HttpNetworkTransaction.)
http://codereview.chromium.org/243077/diff/18002/18005 File net/socket_stream/socket_stream.h (right): http://codereview.chromium.org/243077/diff/18002/18005#newcode30 Line 30: class SocketStream : public base::RefCountedThreadSafe<SocketStream> { I forgot to email you this suggestion: I believe that SocketStream is used to implement WebSockets. If so, please add a comment to say that. The only mention of WebSockets in these two files is the "wss" string in SocketStream::is_secure().
http://codereview.chromium.org/243077/diff/18002/18004 File net/socket_stream/socket_stream.cc (right): http://codereview.chromium.org/243077/diff/18002/18004#newcode43 Line 43: host_resolver_(CreateSystemHostResolver()), The HostResolver should be passed in as a dependency instead. When running in chrome we should use the same HostResolver as the other code, so it can share the same cache, threadpool, etc.. (Also this gives unittests the flexibility to specify mock implementations.)
http://codereview.chromium.org/243077/diff/18002/18004 File net/socket_stream/socket_stream.cc (right): http://codereview.chromium.org/243077/diff/18002/18004#newcode43 Line 43: host_resolver_(CreateSystemHostResolver()), On 2009/11/09 22:22:11, eroman wrote: > The HostResolver should be passed in as a dependency instead. > > When running in chrome we should use the same HostResolver as the other code, so > it can share the same cache, threadpool, etc.. > > (Also this gives unittests the flexibility to specify mock implementations.) Also the HostResolver created in the chrome code is tied to some command-line flags, like disabling IPv6, which this would be missing out on. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
