Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10783028:
Implement ChromiumSocketFactory. (Closed)
|
Created:
8 years, 5 months ago by Sergey Ulanov Modified:
8 years, 5 months ago Reviewers:
Wez CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement ChromiumSocketFactory.
The new PacketSocketFactory will be used by chromoting host in order to be able to
use chromium UDP sockets instead of libjingle sockets
BUG=137140
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147934
Patch Set 1 : #
Total comments: 45
Patch Set 2 : #Patch Set 3 : #
Messages
Total messages: 10 (0 generated)
http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... File remoting/jingle_glue/chromium_socket_factory.cc (right): http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:24: // Maximum amount of data in the send buffers. This is necessary to Your line-wrap seems to be at ten characters short? http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:32: explicit UdpPacketSocket(); No need for explicit? http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:36: // |should be assigned by the OS. typo: |should http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:36: // |should be assigned by the OS. nit: Suggest "|min_port| and |max_port| limit the range of ports the socket may use. If both are zero then the OS assigns a port number." http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:37: bool Init(const talk_base::SocketAddress& local_address, nit: Does local_address accept the usual INADDR_ANY, etc? http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:79: scoped_refptr<net::IOBuffer> receive_buffer_; nit: Add a comment explaining why we need these in the class - because ReadFrom() completes asynchronously and we have to pass it the buffer & IPEndPoint to populate? http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:119: net::IPEndPoint address_with_port(local_endpoint.address(), port); nit: endpoint_with_port http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:166: // are bound. Fix that problem and change this to DCHECK. nit: OS socket APIs will typically return EINVAL rather than crashing the caller, in this case, so are you sure you prefer a DCHECK? It'll make us fragile to changes in libjingle that implicitly assume the EINVAL behaviour. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:166: // are bound. Fix that problem and change this to DCHECK. Doesn't that mean before Init() has been called? How can that happen? http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:185: DoSend(); nit: Blank line here, for consistency with Init() http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:202: return 0; nit: return -1; ? http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:206: DCHECK(socket_.get()); Shouldn't this be an EINVAL return akin to Send()? http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:214: return success ? 0 : -1; nit: Should we SetError() in case of failure? http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:249: send_queue_.front().data, send_queue_.front().data->size(), nit: Ick. Pull out send_queue.front() as a local reference variable to make this readable. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:252: base::Unretained(this))); off-topic: Whenever I see this it frightens me because I forget that net::UdpSocket drops all callbacks when deleted - I wish there were a way to document that a use of base::Unretained is safe, as opposed to all the broken usages we have elsewhere... http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:265: // errors. nit: No need for the second use of "errors". http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:273: send_queue_size_ -= send_queue_.front().data->size(); nit: Clarify that we don't need to worry about partial sends since this is a datagram socket. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:285: HandleReadResult(result, &read_again); It seems cleaner to check whether result > 0 after HandleReadResult and break if not, rather than keep a read_again flag. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:297: void UdpPacketSocket::HandleReadResult(int result, bool* read_again) { Why not have read_again be the return value of HandleReadResult()? http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:306: LOG(ERROR) << "Failed to covert address received from RecvFrom()."; I think you want to read again even if the peer address didn't convert for some reason (although it always will...)? http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:306: LOG(ERROR) << "Failed to covert address received from RecvFrom()."; typo: covert http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... File remoting/jingle_glue/chromium_socket_factory_unittest.cc (right): http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory_unittest.cc:71: kAttemptPeriod); nit: Indentation http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory_unittest.cc:79: EXPECT_EQ(0, socket_->SetOption(talk_base::Socket::OPT_SNDBUF, 4096)); nit: Indentation
http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... File remoting/jingle_glue/chromium_socket_factory.cc (right): http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:24: // Maximum amount of data in the send buffers. This is necessary to On 2012/07/19 00:40:20, Wez wrote: > Your line-wrap seems to be at ten characters short? Emacs wraps comments at 70 chars (by default and also with the standard settings file for Google's C++ style). IMHO it's more readable that way. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:32: explicit UdpPacketSocket(); On 2012/07/19 00:40:20, Wez wrote: > No need for explicit? Done. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:36: // |should be assigned by the OS. On 2012/07/19 00:40:20, Wez wrote: > typo: |should Done. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:36: // |should be assigned by the OS. On 2012/07/19 00:40:20, Wez wrote: > nit: Suggest "|min_port| and |max_port| limit the range of ports the socket may > use. If both are zero then the OS assigns a port number." Just removed this comment. meaning of these values is defined by the PacketSocketFactory interface. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:37: bool Init(const talk_base::SocketAddress& local_address, On 2012/07/19 00:40:20, Wez wrote: > nit: Does local_address accept the usual INADDR_ANY, etc? Yes, but it's never used that way. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:79: scoped_refptr<net::IOBuffer> receive_buffer_; On 2012/07/19 00:40:20, Wez wrote: > nit: Add a comment explaining why we need these in the class - because > ReadFrom() completes asynchronously and we have to pass it the buffer & > IPEndPoint to populate? Done. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:119: net::IPEndPoint address_with_port(local_endpoint.address(), port); On 2012/07/19 00:40:20, Wez wrote: > nit: endpoint_with_port removed this variable. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:166: // are bound. Fix that problem and change this to DCHECK. On 2012/07/19 00:40:20, Wez wrote: > nit: OS socket APIs will typically return EINVAL rather than crashing the > caller, in this case, so are you sure you prefer a DCHECK? It'll make us fragile > to changes in libjingle that implicitly assume the EINVAL behaviour. I copied that TODO from PepperPacketSocketFactory, but it's not relevant here. With the pepper interface bind() is asynchronous but current StunPort doesn't handle asynchronous bind properly. DCHECK would be helpful to detect such bugs. Anyway removed this TODO and added NOTREACHED() here. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:185: DoSend(); On 2012/07/19 00:40:20, Wez wrote: > nit: Blank line here, for consistency with Init() Done. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:202: return 0; On 2012/07/19 00:40:20, Wez wrote: > nit: return -1; ? Done. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:206: DCHECK(socket_.get()); On 2012/07/19 00:40:20, Wez wrote: > Shouldn't this be an EINVAL return akin to Send()? Done. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:214: return success ? 0 : -1; On 2012/07/19 00:40:20, Wez wrote: > nit: Should we SetError() in case of failure? PhysicalSocketServer doesn't do it. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:249: send_queue_.front().data, send_queue_.front().data->size(), On 2012/07/19 00:40:20, Wez wrote: > nit: Ick. Pull out send_queue.front() as a local reference variable to make this > readable. Done. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:252: base::Unretained(this))); On 2012/07/19 00:40:20, Wez wrote: > off-topic: Whenever I see this it frightens me because I forget that > net::UdpSocket drops all callbacks when deleted - I wish there were a way to > document that a use of base::Unretained is safe, as opposed to all the broken > usages we have elsewhere... I agree. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:265: // errors. On 2012/07/19 00:40:20, Wez wrote: > nit: No need for the second use of "errors". Done. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:273: send_queue_size_ -= send_queue_.front().data->size(); On 2012/07/19 00:40:20, Wez wrote: > nit: Clarify that we don't need to worry about partial sends since this is a > datagram socket. Done. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:285: HandleReadResult(result, &read_again); On 2012/07/19 00:40:20, Wez wrote: > It seems cleaner to check whether result > 0 after HandleReadResult and break if > not, rather than keep a read_again flag. Done. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:297: void UdpPacketSocket::HandleReadResult(int result, bool* read_again) { On 2012/07/19 00:40:20, Wez wrote: > Why not have read_again be the return value of HandleReadResult()? It's more readable with the parameter instead of return value. Anyway I removed it now. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:306: LOG(ERROR) << "Failed to covert address received from RecvFrom()."; On 2012/07/19 00:40:20, Wez wrote: > typo: covert Done. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory.cc:306: LOG(ERROR) << "Failed to covert address received from RecvFrom()."; On 2012/07/19 00:40:20, Wez wrote: > I think you want to read again even if the peer address didn't convert for some > reason (although it always will...)? Done. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... File remoting/jingle_glue/chromium_socket_factory_unittest.cc (right): http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory_unittest.cc:71: kAttemptPeriod); On 2012/07/19 00:40:20, Wez wrote: > nit: Indentation Done. http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... remoting/jingle_glue/chromium_socket_factory_unittest.cc:79: EXPECT_EQ(0, socket_->SetOption(talk_base::Socket::OPT_SNDBUF, 4096)); On 2012/07/19 00:40:20, Wez wrote: > nit: Indentation Done.
On 2012/07/19 20:16:39, sergeyu wrote: > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > File remoting/jingle_glue/chromium_socket_factory.cc (right): > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > remoting/jingle_glue/chromium_socket_factory.cc:24: // Maximum amount of data in > the send buffers. This is necessary to > On 2012/07/19 00:40:20, Wez wrote: > > Your line-wrap seems to be at ten characters short? > > Emacs wraps comments at 70 chars (by default and also with the standard settings > file for Google's C++ style). IMHO it's more readable that way. > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > remoting/jingle_glue/chromium_socket_factory.cc:32: explicit UdpPacketSocket(); > On 2012/07/19 00:40:20, Wez wrote: > > No need for explicit? > > Done. > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > remoting/jingle_glue/chromium_socket_factory.cc:36: // |should be assigned by > the OS. > On 2012/07/19 00:40:20, Wez wrote: > > typo: |should > > Done. > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > remoting/jingle_glue/chromium_socket_factory.cc:36: // |should be assigned by > the OS. > On 2012/07/19 00:40:20, Wez wrote: > > nit: Suggest "|min_port| and |max_port| limit the range of ports the socket > may > > use. If both are zero then the OS assigns a port number." > > Just removed this comment. meaning of these values is defined by the > PacketSocketFactory interface. > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > remoting/jingle_glue/chromium_socket_factory.cc:37: bool Init(const > talk_base::SocketAddress& local_address, > On 2012/07/19 00:40:20, Wez wrote: > > nit: Does local_address accept the usual INADDR_ANY, etc? > > Yes, but it's never used that way. > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > remoting/jingle_glue/chromium_socket_factory.cc:79: scoped_refptr<net::IOBuffer> > receive_buffer_; > On 2012/07/19 00:40:20, Wez wrote: > > nit: Add a comment explaining why we need these in the class - because > > ReadFrom() completes asynchronously and we have to pass it the buffer & > > IPEndPoint to populate? > > Done. > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > remoting/jingle_glue/chromium_socket_factory.cc:119: net::IPEndPoint > address_with_port(local_endpoint.address(), port); > On 2012/07/19 00:40:20, Wez wrote: > > nit: endpoint_with_port > > removed this variable. > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > remoting/jingle_glue/chromium_socket_factory.cc:166: // are bound. Fix that > problem and change this to DCHECK. > On 2012/07/19 00:40:20, Wez wrote: > > nit: OS socket APIs will typically return EINVAL rather than crashing the > > caller, in this case, so are you sure you prefer a DCHECK? It'll make us > fragile > > to changes in libjingle that implicitly assume the EINVAL behaviour. > > I copied that TODO from PepperPacketSocketFactory, but it's not relevant here. > With the pepper interface bind() is asynchronous but current StunPort doesn't > handle asynchronous bind properly. DCHECK would be helpful to detect such bugs. > Anyway removed this TODO and added NOTREACHED() here. > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > remoting/jingle_glue/chromium_socket_factory.cc:185: DoSend(); > On 2012/07/19 00:40:20, Wez wrote: > > nit: Blank line here, for consistency with Init() > > Done. > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > remoting/jingle_glue/chromium_socket_factory.cc:202: return 0; > On 2012/07/19 00:40:20, Wez wrote: > > nit: return -1; ? > > Done. > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > remoting/jingle_glue/chromium_socket_factory.cc:206: DCHECK(socket_.get()); > On 2012/07/19 00:40:20, Wez wrote: > > Shouldn't this be an EINVAL return akin to Send()? > > Done. > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > remoting/jingle_glue/chromium_socket_factory.cc:214: return success ? 0 : -1; > On 2012/07/19 00:40:20, Wez wrote: > > nit: Should we SetError() in case of failure? > > PhysicalSocketServer doesn't do it. > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > remoting/jingle_glue/chromium_socket_factory.cc:249: send_queue_.front().data, > send_queue_.front().data->size(), > On 2012/07/19 00:40:20, Wez wrote: > > nit: Ick. Pull out send_queue.front() as a local reference variable to make > this > > readable. > > Done. > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > remoting/jingle_glue/chromium_socket_factory.cc:252: base::Unretained(this))); > On 2012/07/19 00:40:20, Wez wrote: > > off-topic: Whenever I see this it frightens me because I forget that > > net::UdpSocket drops all callbacks when deleted - I wish there were a way to > > document that a use of base::Unretained is safe, as opposed to all the broken > > usages we have elsewhere... > > I agree. > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > remoting/jingle_glue/chromium_socket_factory.cc:265: // errors. > On 2012/07/19 00:40:20, Wez wrote: > > nit: No need for the second use of "errors". > > Done. > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > remoting/jingle_glue/chromium_socket_factory.cc:273: send_queue_size_ -= > send_queue_.front().data->size(); > On 2012/07/19 00:40:20, Wez wrote: > > nit: Clarify that we don't need to worry about partial sends since this is a > > datagram socket. > > Done. > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > remoting/jingle_glue/chromium_socket_factory.cc:285: HandleReadResult(result, > &read_again); > On 2012/07/19 00:40:20, Wez wrote: > > It seems cleaner to check whether result > 0 after HandleReadResult and break > if > > not, rather than keep a read_again flag. > > Done. > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > remoting/jingle_glue/chromium_socket_factory.cc:297: void > UdpPacketSocket::HandleReadResult(int result, bool* read_again) { > On 2012/07/19 00:40:20, Wez wrote: > > Why not have read_again be the return value of HandleReadResult()? > > It's more readable with the parameter instead of return value. Anyway I removed > it now. > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > remoting/jingle_glue/chromium_socket_factory.cc:306: LOG(ERROR) << "Failed to > covert address received from RecvFrom()."; > On 2012/07/19 00:40:20, Wez wrote: > > typo: covert > > Done. > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > remoting/jingle_glue/chromium_socket_factory.cc:306: LOG(ERROR) << "Failed to > covert address received from RecvFrom()."; > On 2012/07/19 00:40:20, Wez wrote: > > I think you want to read again even if the peer address didn't convert for > some > > reason (although it always will...)? > > Done. > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > File remoting/jingle_glue/chromium_socket_factory_unittest.cc (right): > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > remoting/jingle_glue/chromium_socket_factory_unittest.cc:71: kAttemptPeriod); > On 2012/07/19 00:40:20, Wez wrote: > > nit: Indentation > > Done. > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > remoting/jingle_glue/chromium_socket_factory_unittest.cc:79: EXPECT_EQ(0, > socket_->SetOption(talk_base::Socket::OPT_SNDBUF, 4096)); > On 2012/07/19 00:40:20, Wez wrote: > > nit: Indentation > > Done. Upload new patch-set?
On 2012/07/20 21:33:51, Wez wrote: > On 2012/07/19 20:16:39, sergeyu wrote: > > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > > File remoting/jingle_glue/chromium_socket_factory.cc (right): > > > > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > > remoting/jingle_glue/chromium_socket_factory.cc:24: // Maximum amount of data > in > > the send buffers. This is necessary to > > On 2012/07/19 00:40:20, Wez wrote: > > > Your line-wrap seems to be at ten characters short? > > > > Emacs wraps comments at 70 chars (by default and also with the standard > settings > > file for Google's C++ style). IMHO it's more readable that way. > > > > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > > remoting/jingle_glue/chromium_socket_factory.cc:32: explicit > UdpPacketSocket(); > > On 2012/07/19 00:40:20, Wez wrote: > > > No need for explicit? > > > > Done. > > > > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > > remoting/jingle_glue/chromium_socket_factory.cc:36: // |should be assigned by > > the OS. > > On 2012/07/19 00:40:20, Wez wrote: > > > typo: |should > > > > Done. > > > > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > > remoting/jingle_glue/chromium_socket_factory.cc:36: // |should be assigned by > > the OS. > > On 2012/07/19 00:40:20, Wez wrote: > > > nit: Suggest "|min_port| and |max_port| limit the range of ports the socket > > may > > > use. If both are zero then the OS assigns a port number." > > > > Just removed this comment. meaning of these values is defined by the > > PacketSocketFactory interface. > > > > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > > remoting/jingle_glue/chromium_socket_factory.cc:37: bool Init(const > > talk_base::SocketAddress& local_address, > > On 2012/07/19 00:40:20, Wez wrote: > > > nit: Does local_address accept the usual INADDR_ANY, etc? > > > > Yes, but it's never used that way. > > > > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > > remoting/jingle_glue/chromium_socket_factory.cc:79: > scoped_refptr<net::IOBuffer> > > receive_buffer_; > > On 2012/07/19 00:40:20, Wez wrote: > > > nit: Add a comment explaining why we need these in the class - because > > > ReadFrom() completes asynchronously and we have to pass it the buffer & > > > IPEndPoint to populate? > > > > Done. > > > > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > > remoting/jingle_glue/chromium_socket_factory.cc:119: net::IPEndPoint > > address_with_port(local_endpoint.address(), port); > > On 2012/07/19 00:40:20, Wez wrote: > > > nit: endpoint_with_port > > > > removed this variable. > > > > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > > remoting/jingle_glue/chromium_socket_factory.cc:166: // are bound. Fix that > > problem and change this to DCHECK. > > On 2012/07/19 00:40:20, Wez wrote: > > > nit: OS socket APIs will typically return EINVAL rather than crashing the > > > caller, in this case, so are you sure you prefer a DCHECK? It'll make us > > fragile > > > to changes in libjingle that implicitly assume the EINVAL behaviour. > > > > I copied that TODO from PepperPacketSocketFactory, but it's not relevant here. > > With the pepper interface bind() is asynchronous but current StunPort doesn't > > handle asynchronous bind properly. DCHECK would be helpful to detect such > bugs. > > Anyway removed this TODO and added NOTREACHED() here. > > > > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > > remoting/jingle_glue/chromium_socket_factory.cc:185: DoSend(); > > On 2012/07/19 00:40:20, Wez wrote: > > > nit: Blank line here, for consistency with Init() > > > > Done. > > > > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > > remoting/jingle_glue/chromium_socket_factory.cc:202: return 0; > > On 2012/07/19 00:40:20, Wez wrote: > > > nit: return -1; ? > > > > Done. > > > > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > > remoting/jingle_glue/chromium_socket_factory.cc:206: DCHECK(socket_.get()); > > On 2012/07/19 00:40:20, Wez wrote: > > > Shouldn't this be an EINVAL return akin to Send()? > > > > Done. > > > > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > > remoting/jingle_glue/chromium_socket_factory.cc:214: return success ? 0 : -1; > > On 2012/07/19 00:40:20, Wez wrote: > > > nit: Should we SetError() in case of failure? > > > > PhysicalSocketServer doesn't do it. > > > > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > > remoting/jingle_glue/chromium_socket_factory.cc:249: send_queue_.front().data, > > send_queue_.front().data->size(), > > On 2012/07/19 00:40:20, Wez wrote: > > > nit: Ick. Pull out send_queue.front() as a local reference variable to make > > this > > > readable. > > > > Done. > > > > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > > remoting/jingle_glue/chromium_socket_factory.cc:252: base::Unretained(this))); > > On 2012/07/19 00:40:20, Wez wrote: > > > off-topic: Whenever I see this it frightens me because I forget that > > > net::UdpSocket drops all callbacks when deleted - I wish there were a way to > > > document that a use of base::Unretained is safe, as opposed to all the > broken > > > usages we have elsewhere... > > > > I agree. > > > > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > > remoting/jingle_glue/chromium_socket_factory.cc:265: // errors. > > On 2012/07/19 00:40:20, Wez wrote: > > > nit: No need for the second use of "errors". > > > > Done. > > > > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > > remoting/jingle_glue/chromium_socket_factory.cc:273: send_queue_size_ -= > > send_queue_.front().data->size(); > > On 2012/07/19 00:40:20, Wez wrote: > > > nit: Clarify that we don't need to worry about partial sends since this is a > > > datagram socket. > > > > Done. > > > > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > > remoting/jingle_glue/chromium_socket_factory.cc:285: HandleReadResult(result, > > &read_again); > > On 2012/07/19 00:40:20, Wez wrote: > > > It seems cleaner to check whether result > 0 after HandleReadResult and > break > > if > > > not, rather than keep a read_again flag. > > > > Done. > > > > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > > remoting/jingle_glue/chromium_socket_factory.cc:297: void > > UdpPacketSocket::HandleReadResult(int result, bool* read_again) { > > On 2012/07/19 00:40:20, Wez wrote: > > > Why not have read_again be the return value of HandleReadResult()? > > > > It's more readable with the parameter instead of return value. Anyway I > removed > > it now. > > > > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > > remoting/jingle_glue/chromium_socket_factory.cc:306: LOG(ERROR) << "Failed to > > covert address received from RecvFrom()."; > > On 2012/07/19 00:40:20, Wez wrote: > > > typo: covert > > > > Done. > > > > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > > remoting/jingle_glue/chromium_socket_factory.cc:306: LOG(ERROR) << "Failed to > > covert address received from RecvFrom()."; > > On 2012/07/19 00:40:20, Wez wrote: > > > I think you want to read again even if the peer address didn't convert for > > some > > > reason (although it always will...)? > > > > Done. > > > > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > > File remoting/jingle_glue/chromium_socket_factory_unittest.cc (right): > > > > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > > remoting/jingle_glue/chromium_socket_factory_unittest.cc:71: kAttemptPeriod); > > On 2012/07/19 00:40:20, Wez wrote: > > > nit: Indentation > > > > Done. > > > > > http://codereview.chromium.org/10783028/diff/2001/remoting/jingle_glue/chromi... > > remoting/jingle_glue/chromium_socket_factory_unittest.cc:79: EXPECT_EQ(0, > > socket_->SetOption(talk_base::Socket::OPT_SNDBUF, 4096)); > > On 2012/07/19 00:40:20, Wez wrote: > > > nit: Indentation > > > > Done. > > Upload new patch-set? done
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/10783028/9001
Failed to apply patch for remoting/remoting.gyp: While running patch -p1 --forward --force; patching file remoting/remoting.gyp Hunk #1 succeeded at 1480 (offset 2 lines). Hunk #2 FAILED at 1706. 1 out of 2 hunks FAILED -- saving rejects to file remoting/remoting.gyp.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/10783028/6002
Change committed as 147934 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
