Move server socket functionality from TCPServerSocket into TCPSocket.

Move server socket functionality from TCPServerSocket into TCPSocket.

TCPServerSocket becomes a wrapper around TCPSocket to expose a server-only interface.

There will be separate CLs to move client socket functionality from TCPClientSocket into TCPSocket.


BUG=262601
TEST=newly added tcp_socket_unittest.cc

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220752

[yzshen1, 2013-08-23 00:07:08]

Hi, Fred and James.

This is part of the net::TCP.*Socket refactoring that we discussed in thread:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/UbAhFjxpDRk

Would you please take a look?
Thanks!

[yzshen1, 2013-08-26 16:59:13]

On 2013/08/23 00:07:08, yzshen1 wrote:
> Hi, Fred and James.
> 
> This is part of the net::TCP.*Socket refactoring that we discussed in thread:
>
https://groups.google.com/a/chromium.org/forum/#%21topic/chromium-dev/UbAhFjx...
> 
> Would you please take a look?
> Thanks!

Friendly ping.

[akalin, 2013-08-26 18:53:21]

few initial comments

https://codereview.chromium.org/22861033/diff/1/net/socket/tcp_client_socket_...
File net/socket/tcp_client_socket_libevent.cc (right):

https://codereview.chromium.org/22861033/diff/1/net/socket/tcp_client_socket_...
net/socket/tcp_client_socket_libevent.cc:166: if (HANDLE_EINTR(close(socket)) <
0)
may be worth splitting off this into its own change, if it is useful to have
independent of the new TCPSocket class.

https://codereview.chromium.org/22861033/diff/1/net/socket/tcp_server_socket.cc
File net/socket/tcp_server_socket.cc (right):

https://codereview.chromium.org/22861033/diff/1/net/socket/tcp_server_socket....
net/socket/tcp_server_socket.cc:57: CompletionCallback accept_callback =
base::Bind(
this call is really awkward. :/ Would it help if tcp_socket and address were
member variables of TCPServerSocket?

https://codereview.chromium.org/22861033/diff/1/net/socket/tcp_server_socket....
net/socket/tcp_server_socket.cc:100: socket->reset(client_socket.release());
*socket = client_socket.Pass();

You might have to do:

*socket = client_socket.PassAs<StreamSocket>();

https://codereview.chromium.org/22861033/diff/1/net/socket/tcp_socket_libevent.h
File net/socket/tcp_socket_libevent.h (right):

https://codereview.chromium.org/22861033/diff/1/net/socket/tcp_socket_libeven...
net/socket/tcp_socket_libevent.h:36: int Listen(int backlog);
can you explain again why TCPSocket needs to have server-specific methods like
Listen and Accept? I would have thought they'd stay with TCPServerSocket, which
would adopt TCPSocket's socket once Listen and Accept are needed.

[yzshen1, 2013-08-26 23:09:57]

Thanks Fred. Please take another look.

https://codereview.chromium.org/22861033/diff/1/net/socket/tcp_client_socket_...
File net/socket/tcp_client_socket_libevent.cc (right):

https://codereview.chromium.org/22861033/diff/1/net/socket/tcp_client_socket_...
net/socket/tcp_client_socket_libevent.cc:166: if (HANDLE_EINTR(close(socket)) <
0)
On 2013/08/26 18:53:21, akalin wrote:
> may be worth splitting off this into its own change, if it is useful to have
> independent of the new TCPSocket class.

Done.

https://codereview.chromium.org/22861033/diff/1/net/socket/tcp_server_socket.cc
File net/socket/tcp_server_socket.cc (right):

https://codereview.chromium.org/22861033/diff/1/net/socket/tcp_server_socket....
net/socket/tcp_server_socket.cc:57: CompletionCallback accept_callback =
base::Bind(
I changed them to members. Thanks!

On 2013/08/26 18:53:21, akalin wrote:
> this call is really awkward. :/ Would it help if tcp_socket and address were
> member variables of TCPServerSocket?

https://codereview.chromium.org/22861033/diff/1/net/socket/tcp_server_socket....
net/socket/tcp_server_socket.cc:100: socket->reset(client_socket.release());
On 2013/08/26 18:53:21, akalin wrote:
> *socket = client_socket.Pass();
> 
> You might have to do:
> 
> *socket = client_socket.PassAs<StreamSocket>();

Done.

https://codereview.chromium.org/22861033/diff/1/net/socket/tcp_socket_libevent.h
File net/socket/tcp_socket_libevent.h (right):

https://codereview.chromium.org/22861033/diff/1/net/socket/tcp_socket_libeven...
net/socket/tcp_socket_libevent.h:36: int Listen(int backlog);
On 2013/08/26 18:53:21, akalin wrote:
> can you explain again why TCPSocket needs to have server-specific methods like
> Listen and Accept? I would have thought they'd stay with TCPServerSocket,
which
> would adopt TCPSocket's socket once Listen and Accept are needed.

In this discussion thread
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/UbAhFjxpDRk

I listed the following two options:
------------------------------------
option 2:
class TCPSocket(Win|Libevent) {
  // Provides both client and server operations. It is more POSIX-like.
  // Pepper will directly use this class.
};

class TCPClientSocket {
  // Wrapper around TCPSocket. Exposes a client-only interface.
 private:
  TCPSocket socket_impl_;
};
class TCPServerSocket {
  // Wrapper around TCPSocket. Exposes a server-only interface.
 private:
  TCPSocket socket_impl_;
};
-------------------------------------
and
-------------------------------------
option 3:
class TCPSocket(Win|Libevent) {
  // All the common operations before we know whether it is a client
  // or server socket.
  int Bind(const IPEndPoint& address);
  int GetLocalAddress(IPEndPoint* address);
  void AllowAddressReuse();
  ...
}

class TCPClientSocket(Win|Libevent) {
  int AdoptTCPSocket(TCPSocket* socket);
  ...
}

class TCPServerSocket(Win|Libevent) {
  int AdoptTCPSocket(TCPSocket* socket);
  ...
}
-------------------------------------

Here I am using #2 and I think you mean #3, right?

I think option #2 is better because:
In option #2, TCPSocket is on a different level than TCPClient/ServerSocket. The
interface of TCPSocket doesn't depend on that of TCPClient/ServerSocket (and
vice versa). We have the flexibility to control how "POSIX-like" it is. On the
other hand, in option #3 the three interfaces are tied together. Here is one
example:

The existing net::TCPServerSocket has the following method:
int Listen(const net::IPEndPoint& address, int backlog)
which is basically POSIX bind() + listen().

If we adopt option #3, because Bind() is an operation that can be performed on
both client and server socket, therefore Bind() belongs to TCPSocket, we will
need to provide a different signature for TCPServerSocket::Listen():
int Listen(int backlog)
Otherwise the following sequence doesn't make sense:
tcp_socket->Bind(some_addr);
tcp_server_socket->AdoptTCPSocket(tcp_socket);
tcp_server_socket->Listen(another_addr, backlog);  // |another_addr| is not
needed!

Option #2 has clear layering:
- net::TCPSocket hides OS API differences, supports watching I/O readiness and
completion callback, etc.
- net::TCP(Client|Server)Socket and Pepper socket are built on top of
net::TCPSocket, but independent of each other.

I think the design guidelines of net::TCP(Client|Server)Socket are different
from those of the Pepper socket API. net::TCP(Client|Server)Socket provide a
clear, high-level API that is sufficient for upper layers of Chrome. The Pepper
socket API tries to provide a flexible, slightly lower level API for external
developers. Option #3 tries to unify the two and will probably ends up with a
confusing mixture.

What do you think? I would be happy to listen to your suggestions and have
further discussions.

[akalin, 2013-08-26 23:29:05]

On 2013/08/26 23:09:57, yzshen1 wrote:
> In this discussion thread
>
https://groups.google.com/a/chromium.org/forum/#%21topic/chromium-dev/UbAhFjx...
> 
> I listed the following two options:
> ------------------------------------
> option 2:
> class TCPSocket(Win|Libevent) {
>   // Provides both client and server operations. It is more POSIX-like.
>   // Pepper will directly use this class.
> };
>
> option 3:
> class TCPSocket(Win|Libevent) {
>   // All the common operations before we know whether it is a client
>   // or server socket.
>   int Bind(const IPEndPoint& address);
>   int GetLocalAddress(IPEndPoint* address);
>   void AllowAddressReuse();
>   ...
> }
> Here I am using #2 and I think you mean #3, right?

Ah, I see. You also plan to move the client socket functionality into TCPSocket,
right? If so, then I'm okay with this. I was just confused because TCPSocket
seemed to just be TCPServerSocket.

I'll take another look with this in mind.

[akalin, 2013-08-26 23:29:14]

https://codereview.chromium.org/22861033/diff/1/net/socket/tcp_client_socket_...
File net/socket/tcp_client_socket_libevent.cc (right):

https://codereview.chromium.org/22861033/diff/1/net/socket/tcp_client_socket_...
net/socket/tcp_client_socket_libevent.cc:166: if (HANDLE_EINTR(close(socket)) <
0)
On 2013/08/26 23:09:57, yzshen1 wrote:
> On 2013/08/26 18:53:21, akalin wrote:
> > may be worth splitting off this into its own change, if it is useful to have
> > independent of the new TCPSocket class.
> 
> Done.

Do you have a link to the new change? Or were you planning to do this after this
one lands. (I'm OK either way.)

[akalin, 2013-08-26 23:48:51]

few more, just looking at TCPServerSocket

https://codereview.chromium.org/22861033/diff/24001/net/socket/tcp_server_soc...
File net/socket/tcp_server_socket.cc (right):

https://codereview.chromium.org/22861033/diff/24001/net/socket/tcp_server_soc...
net/socket/tcp_server_socket.cc:30: result = socket_.SetExclusiveAddrUse();
ideally this platform difference would be abstracted out. Are you planning to do
this? (perhaps in a future CL?)

https://codereview.chromium.org/22861033/diff/24001/net/socket/tcp_server_soc...
net/socket/tcp_server_socket.cc:35: (result = socket_.Bind(address)) != OK ||
assignments in conditionals are against the style guide, or at least not usual,
I believe.

You're doing this to avoid repeating the Close() call, right?

What about this:

Define a Swap() function for TCPSocket. Then use a local variable socket in this
function, and swap it with socket_ only at the very end (with OK). Assuming
TCPSocket's destructor does the same cleanup as Close(), this should work (and
be arguably more elegant).

https://codereview.chromium.org/22861033/diff/24001/net/socket/tcp_server_soc...
net/socket/tcp_server_socket.cc:59:
base::Bind(&TCPServerSocket::OnAcceptCompleted, base::Unretained(this),
you're doing Bind() twice!

https://codereview.chromium.org/22861033/diff/24001/net/socket/tcp_server_soc...
net/socket/tcp_server_socket.cc:60: socket, callback));
line up socket with &TCPServerSocket

https://codereview.chromium.org/22861033/diff/24001/net/socket/tcp_server_soc...
net/socket/tcp_server_socket.cc:79: if (result != OK)
Can you decomp the body of the do/while loop into a separate function instead?
Something like:

int DoAccept(int result, scoped_ptr<StreamSocket>* accepted_socket) {
  ...
  if (result != OK)
    return result;
  ...
  *accepted_socket = ...;
  return OK;
}

Then you can do:

result = DoAccept(result, accepted_socket);
...
if (!forward_callback.is_null())
  forward_callback.Run(result);

In fact, you can probably work this a bit more and call DoAccept() from the
non-IO_PENDING case in Accept() directly.

https://codereview.chromium.org/22861033/diff/24001/net/socket/tcp_server_soc...
File net/socket/tcp_server_socket.h (right):

https://codereview.chromium.org/22861033/diff/24001/net/socket/tcp_server_soc...
net/socket/tcp_server_socket.h:33: void
OnAcceptCompleted(scoped_ptr<StreamSocket>* output_accepted_socket,
put output param last (and probably don't need to prepend output_)

[yzshen1, 2013-08-26 23:55:44]

> Ah, I see. You also plan to move the client socket functionality into
TCPSocket,
> right? If so, then I'm okay with this. I was just confused because TCPSocket
> seemed to just be TCPServerSocket.
> 
> I'll take another look with this in mind.

Yes, I am planning to do that in a different CL.

On 2013/08/26 23:29:14, akalin wrote:
>
https://codereview.chromium.org/22861033/diff/1/net/socket/tcp_client_socket_...
> File net/socket/tcp_client_socket_libevent.cc (right):
> 
>
https://codereview.chromium.org/22861033/diff/1/net/socket/tcp_client_socket_...
> net/socket/tcp_client_socket_libevent.cc:166: if (HANDLE_EINTR(close(socket))
<
> 0)
> On 2013/08/26 23:09:57, yzshen1 wrote:
> > On 2013/08/26 18:53:21, akalin wrote:
> > > may be worth splitting off this into its own change, if it is useful to
have
> > > independent of the new TCPSocket class.
> > 
> > Done.
> 
> Do you have a link to the new change? Or were you planning to do this after
this
> one lands. (I'm OK either way.)
I realize that I don't need those changes. I leave it as it is (i.e.,
TCPClientSocket::AdoptSocket() won't take ownership on failure.)
And then in TCPServerSocket::OnAcceptCompleted() I take care of that. So there
won't be a separate CL for this.

[yzshen1, 2013-08-27 17:36:56]

Thanks, Fred!
Please take another look.

https://codereview.chromium.org/22861033/diff/24001/net/socket/tcp_server_soc...
File net/socket/tcp_server_socket.cc (right):

https://codereview.chromium.org/22861033/diff/24001/net/socket/tcp_server_soc...
net/socket/tcp_server_socket.cc:30: result = socket_.SetExclusiveAddrUse();
On 2013/08/26 23:48:51, akalin wrote:
> ideally this platform difference would be abstracted out. Are you planning to
do
> this? (perhaps in a future CL?)

Yeah, I wish to do it later. I added a TODO.

https://codereview.chromium.org/22861033/diff/24001/net/socket/tcp_server_soc...
net/socket/tcp_server_socket.cc:35: (result = socket_.Bind(address)) != OK ||
On 2013/08/26 23:48:51, akalin wrote:
> assignments in conditionals are against the style guide, or at least not
usual,
> I believe.
> 
> You're doing this to avoid repeating the Close() call, right?
> 
> What about this:
> 
> Define a Swap() function for TCPSocket. Then use a local variable socket in
this
> function, and swap it with socket_ only at the very end (with OK). Assuming
> TCPSocket's destructor does the same cleanup as Close(), this should work (and
> be arguably more elegant).

It doesn't seem to worth the effort to add Swap() just for avoiding multiple
Close()s.

If you don't mind, I will just do multiple Closes(). :)

https://codereview.chromium.org/22861033/diff/24001/net/socket/tcp_server_soc...
net/socket/tcp_server_socket.cc:59:
base::Bind(&TCPServerSocket::OnAcceptCompleted, base::Unretained(this),
Oops. :)

On 2013/08/26 23:48:51, akalin wrote:
> you're doing Bind() twice!

https://codereview.chromium.org/22861033/diff/24001/net/socket/tcp_server_soc...
net/socket/tcp_server_socket.cc:60: socket, callback));
On 2013/08/26 23:48:51, akalin wrote:
> line up socket with &TCPServerSocket

Done.

https://codereview.chromium.org/22861033/diff/24001/net/socket/tcp_server_soc...
net/socket/tcp_server_socket.cc:79: if (result != OK)
Thanks, this is a good idea.
Done.

On 2013/08/26 23:48:51, akalin wrote:
> Can you decomp the body of the do/while loop into a separate function instead?
> Something like:
> 
> int DoAccept(int result, scoped_ptr<StreamSocket>* accepted_socket) {
>   ...
>   if (result != OK)
>     return result;
>   ...
>   *accepted_socket = ...;
>   return OK;
> }
> 
> Then you can do:
> 
> result = DoAccept(result, accepted_socket);
> ...
> if (!forward_callback.is_null())
>   forward_callback.Run(result);
> 
> In fact, you can probably work this a bit more and call DoAccept() from the
> non-IO_PENDING case in Accept() directly.

https://codereview.chromium.org/22861033/diff/24001/net/socket/tcp_server_soc...
File net/socket/tcp_server_socket.h (right):

https://codereview.chromium.org/22861033/diff/24001/net/socket/tcp_server_soc...
net/socket/tcp_server_socket.h:33: void
OnAcceptCompleted(scoped_ptr<StreamSocket>* output_accepted_socket,
We cannot, because we use this method in a base::Bind() and
|output_accepted_socket| and |forward_callback| are bound first.

If I change |output_accepted_socket| to |accepted_socket|, it will be too
similar to |accepted_socket_|, right? I wish I could leave it as it is.

What do you think?

On 2013/08/26 23:48:51, akalin wrote:
> put output param last (and probably don't need to prepend output_)

[yzshen1, 2013-08-27 22:54:25]

https://codereview.chromium.org/22861033/diff/24001/net/socket/tcp_server_soc...
File net/socket/tcp_server_socket.cc (right):

https://codereview.chromium.org/22861033/diff/24001/net/socket/tcp_server_soc...
net/socket/tcp_server_socket.cc:30: result = socket_.SetExclusiveAddrUse();
After a second thought, I just did it in this CL because it is trivial.

On 2013/08/27 17:36:56, yzshen1 wrote:
> On 2013/08/26 23:48:51, akalin wrote:
> > ideally this platform difference would be abstracted out. Are you planning
to
> do
> > this? (perhaps in a future CL?)
> 
> Yeah, I wish to do it later. I added a TODO.

[akalin, 2013-08-29 22:08:32]

I have a few more nits, but I'm generally satisfied with this CL. However, I'm
not too familiar with TCP*Socket, so I'm adding Wan-Teh. wtc@, can you take a
look at this CL, too?

https://codereview.chromium.org/22861033/diff/42001/net/socket/tcp_server_soc...
File net/socket/tcp_server_socket.cc (right):

https://codereview.chromium.org/22861033/diff/42001/net/socket/tcp_server_soc...
net/socket/tcp_server_socket.cc:58: DCHECK(!pending_accept_);
would prefer:

if (pending_accept_) {
  NOTREACHED();
  return ERR_UNEXPECTED;
}

https://codereview.chromium.org/22861033/diff/42001/net/socket/tcp_server_soc...
net/socket/tcp_server_socket.cc:83: // Make sure the TCPSocket object is
destructed in any case.
destructed -> destroyed

https://codereview.chromium.org/22861033/diff/42001/net/socket/tcp_socket_lib...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/22861033/diff/42001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:61: default:
suggest removing the default case so that in case anything gets added to
AddressFamily (unlikely, I know) it'll be a compile-time warning/error. Instead,
do:

DCHECK_NE(address_family, AF_UNSPEC);

right below the switch

https://codereview.chromium.org/22861033/diff/42001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:240: if (address)
is this check necessary? can't you mandate that |address| is always non-null?

https://codereview.chromium.org/22861033/diff/42001/net/socket/tcp_socket_uni...
File net/socket/tcp_socket_unittest.cc (right):

https://codereview.chromium.org/22861033/diff/42001/net/socket/tcp_socket_uni...
net/socket/tcp_socket_unittest.cc:72: TEST_F(TCPSocketTest, Accept) {
can you add comments for Accept and AcceptIPV6?

https://codereview.chromium.org/22861033/diff/42001/net/socket/tcp_socket_win.cc
File net/socket/tcp_socket_win.cc (right):

https://codereview.chromium.org/22861033/diff/42001/net/socket/tcp_socket_win...
net/socket/tcp_socket_win.cc:40: int address_family = AF_UNSPEC;
decomp into a helper function somewhere? one doesn't already exist?

[yzshen1, 2013-08-29 23:04:17]

Thanks, Fred!

https://codereview.chromium.org/22861033/diff/42001/net/socket/tcp_server_soc...
File net/socket/tcp_server_socket.cc (right):

https://codereview.chromium.org/22861033/diff/42001/net/socket/tcp_server_soc...
net/socket/tcp_server_socket.cc:58: DCHECK(!pending_accept_);
I thought this is more consistent with the existing code. (I may be wrong.)

For example, TCPClientSocketWin::Read() has
  DCHECK(!waiting_read_);

The old TCPServerSocketWin::Accept() (which this CL removes) has
  DCHECK(accept_callback_.is_null());
It is equivalent to DCHECK(!pending_accept_);

By reading the code, I think TCP.*Socket expect callers to do right things:
using valid parameters and correct calling sequences. Therefore they only use
DCHECK()s. It is a design decision IMO. It would be quite some changes to
check-and-return-failure everywhere.

If you think it is okay, I am going to leave it as it is. What do you think?

On 2013/08/29 22:08:33, akalin wrote:
> would prefer:
> 
> if (pending_accept_) {
>   NOTREACHED();
>   return ERR_UNEXPECTED;
> }

https://codereview.chromium.org/22861033/diff/42001/net/socket/tcp_server_soc...
net/socket/tcp_server_socket.cc:83: // Make sure the TCPSocket object is
destructed in any case.
On 2013/08/29 22:08:33, akalin wrote:
> destructed -> destroyed

Done.

https://codereview.chromium.org/22861033/diff/42001/net/socket/tcp_socket_lib...
File net/socket/tcp_socket_libevent.cc (right):

https://codereview.chromium.org/22861033/diff/42001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:61: default:
On 2013/08/29 22:08:33, akalin wrote:
> suggest removing the default case so that in case anything gets added to
> AddressFamily (unlikely, I know) it'll be a compile-time warning/error.
Instead,
> do:
> 
> DCHECK_NE(address_family, AF_UNSPEC);
> 
> right below the switch

Done.

https://codereview.chromium.org/22861033/diff/42001/net/socket/tcp_socket_lib...
net/socket/tcp_socket_libevent.cc:240: if (address)
I was trying to follow POSIX accept(); its output addr parameter can be NULL.
But maybe that is not necessary, I just changed as you suggested.

On 2013/08/29 22:08:33, akalin wrote:
> is this check necessary? can't you mandate that |address| is always non-null?

https://codereview.chromium.org/22861033/diff/42001/net/socket/tcp_socket_uni...
File net/socket/tcp_socket_unittest.cc (right):

https://codereview.chromium.org/22861033/diff/42001/net/socket/tcp_socket_uni...
net/socket/tcp_socket_unittest.cc:72: TEST_F(TCPSocketTest, Accept) {
On 2013/08/29 22:08:33, akalin wrote:
> can you add comments for Accept and AcceptIPV6?

Done.

https://codereview.chromium.org/22861033/diff/42001/net/socket/tcp_socket_win.cc
File net/socket/tcp_socket_win.cc (right):

https://codereview.chromium.org/22861033/diff/42001/net/socket/tcp_socket_win...
net/socket/tcp_socket_win.cc:40: int address_family = AF_UNSPEC;
Done. I put it in net/base/net_util.h.

On 2013/08/29 22:08:33, akalin wrote:
> decomp into a helper function somewhere? one doesn't already exist?

[akalin, 2013-08-29 23:33:26]

https://codereview.chromium.org/22861033/diff/42001/net/socket/tcp_server_soc...
File net/socket/tcp_server_socket.cc (right):

https://codereview.chromium.org/22861033/diff/42001/net/socket/tcp_server_soc...
net/socket/tcp_server_socket.cc:58: DCHECK(!pending_accept_);
On 2013/08/29 23:04:18, yzshen1 wrote:
> I thought this is more consistent with the existing code. (I may be wrong.)
> 
> For example, TCPClientSocketWin::Read() has
>   DCHECK(!waiting_read_);
> 
> The old TCPServerSocketWin::Accept() (which this CL removes) has
>   DCHECK(accept_callback_.is_null());
> It is equivalent to DCHECK(!pending_accept_);
> 
> By reading the code, I think TCP.*Socket expect callers to do right things:
> using valid parameters and correct calling sequences. Therefore they only use
> DCHECK()s. It is a design decision IMO. It would be quite some changes to
> check-and-return-failure everywhere.

You know that NOTREACHED() is equivalent to DCHECK(false), not CHECK(false),
right? I feel like failing early in release mode (by returning ERR_UNEXPECTED,
which is the error code for this purpose) is worth doing here, even if other
code doesn't do it.

[yzshen1, 2013-08-29 23:50:14]

Thanks Fred!

> You know that NOTREACHED() is equivalent to DCHECK(false), not CHECK(false),
> right?
Yes. I understand.

> I feel like failing early in release mode (by returning ERR_UNEXPECTED,
> which is the error code for this purpose) is worth doing here, even if other
> code doesn't do it.
Done. :)

[akalin, 2013-08-29 23:54:36]

LGTM, but wait for wtc to look at it, too!

https://codereview.chromium.org/22861033/diff/67001/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/22861033/diff/67001/net/base/net_util.h#newco...
net/base/net_util.h:441: // Converts AddressFamily to socket family, AF_INET,
AF_INET6 or AF_UNSPEC.
how about:

// Maps the given AddressFamily to either AF_INET, AF_INET6, or AF_UNSPEC.

[yzshen1, 2013-08-30 00:12:07]

Great!

Thanks Fred. :)

https://codereview.chromium.org/22861033/diff/67001/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/22861033/diff/67001/net/base/net_util.h#newco...
net/base/net_util.h:441: // Converts AddressFamily to socket family, AF_INET,
AF_INET6 or AF_UNSPEC.
On 2013/08/29 23:54:36, akalin wrote:
> how about:
> 
> // Maps the given AddressFamily to either AF_INET, AF_INET6, or AF_UNSPEC.

Done.

[wtc, 2013-08-30 18:45:48]

Review comments on patch set 9:

This seems OK. I only took a quick look. Are you going to
rewrite TCPClientSocket to be a wrapper around TCPSocket?

https://chromiumcodereview.appspot.com/22861033/diff/71001/net/socket/tcp_soc...
File net/socket/tcp_socket_libevent.h (right):

https://chromiumcodereview.appspot.com/22861033/diff/71001/net/socket/tcp_soc...
net/socket/tcp_socket_libevent.h:42: void Close();

You should add a comment to point out this class is incomplete. It is
missing the Connect, Read, and Write methods. Right now this class looks
like a TCP server socket, and I was wondering why it is named simply
TCPSocket.

[yzshen1, 2013-08-30 18:59:47]

> This seems OK. I only took a quick look. Are you going to
> rewrite TCPClientSocket to be a wrapper around TCPSocket?

Yes. I am doing it. I will send the CLs to you.

Thanks!

https://chromiumcodereview.appspot.com/22861033/diff/71001/net/socket/tcp_soc...
File net/socket/tcp_socket_libevent.h (right):

https://chromiumcodereview.appspot.com/22861033/diff/71001/net/socket/tcp_soc...
net/socket/tcp_socket_libevent.h:42: void Close();
Done. Thanks!

On 2013/08/30 18:45:49, wtc wrote:
> 
> You should add a comment to point out this class is incomplete. It is
> missing the Connect, Read, and Write methods. Right now this class looks
> like a TCP server socket, and I was wondering why it is named simply
> TCPSocket.

[Wez, 2013-08-30 19:17:50]

Looking over this CL in the context of the surrounding net/socket/* code:

- Do we need the net::ServerSocket interface? It sounds generic but is in fact
TCP-specific.

- If TCPSocket is intended to be an internal implementation detail of
TCPClient/ServerSocket then can it be TCPSocketImpl, with a comment to indicate
that in general callers should use TCPClient/ServerSocket instead?

- IIUC a follow-up CL will switch TCPClientSocket to use the new TCPSocket
internally?

- Why do we have StreamListenSocket in parallel to the TCP[Client/Server]Socket
classes?

https://codereview.chromium.org/22861033/diff/71001/net/socket/tcp_socket.h
File net/socket/tcp_socket.h (right):

https://codereview.chromium.org/22861033/diff/71001/net/socket/tcp_socket.h#n...
net/socket/tcp_socket.h:18: // TCPSocket provides a platform-indenpendent
interface for TCP sockets.
typo: independent

[akalin, 2013-08-30 19:46:00]

On Fri, Aug 30, 2013 at 12:17 PM, <wez@chromium.org> wrote:

> Looking over this CL in the context of the surrounding net/socket/* code:
>
> - If TCPSocket is intended to be an internal implementation detail of
> TCPClient/ServerSocket then can it be TCPSocketImpl, with a comment to
> indicate
> that in general callers should use TCPClient/ServerSocket instead?
>

If there is already a legit use case for using TCPSocket directly, it seems
to me the current comment suffices.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2013-08-30 21:11:22]

> - Do we need the net::ServerSocket interface? It sounds generic but is in fact
> TCP-specific.

I don't know about the reason.
Maybe it is easier for testing? socket_host_tcp_server_unittest.cc defines a
subclass of net::ServerSocket.

In this CL, I think it is good to keep this unchanged. We could have an separate
discussion if necessary.

> 
> - If TCPSocket is intended to be an internal implementation detail of
> TCPClient/ServerSocket then can it be TCPSocketImpl, with a comment to
indicate
> that in general callers should use TCPClient/ServerSocket instead?

TCPSocket will be used by Pepper, which is outside of the net/ folder. So it is
not an internal impl detail.

> 
> - IIUC a follow-up CL will switch TCPClientSocket to use the new TCPSocket
> internally?

Yes. I updated the CL description to make this clear.

> 
> - Why do we have StreamListenSocket in parallel to the
TCP[Client/Server]Socket
> classes?

I don't know. :/ But maybe in the future, we could based it on top of TCPSocket,
too.

https://codereview.chromium.org/22861033/diff/71001/net/socket/tcp_socket.h
File net/socket/tcp_socket.h (right):

https://codereview.chromium.org/22861033/diff/71001/net/socket/tcp_socket.h#n...
net/socket/tcp_socket.h:18: // TCPSocket provides a platform-indenpendent
interface for TCP sockets.
On 2013/08/30 19:17:51, Wez wrote:
> typo: independent

Done.

[Wez, 2013-08-30 22:34:46]

On 2013/08/30 21:11:22, yzshen1 wrote:
> > - Do we need the net::ServerSocket interface? It sounds generic but is in
fact
> > TCP-specific.
> 
> I don't know about the reason.
> Maybe it is easier for testing? socket_host_tcp_server_unittest.cc defines a
> subclass of net::ServerSocket.
> 
> In this CL, I think it is good to keep this unchanged. We could have an
separate
> discussion if necessary.
> 
> > 
> > - If TCPSocket is intended to be an internal implementation detail of
> > TCPClient/ServerSocket then can it be TCPSocketImpl, with a comment to
> indicate
> > that in general callers should use TCPClient/ServerSocket instead?
> 
> TCPSocket will be used by Pepper, which is outside of the net/ folder. So it
is
> not an internal impl detail.
> 
> > 
> > - IIUC a follow-up CL will switch TCPClientSocket to use the new TCPSocket
> > internally?
> 
> Yes. I updated the CL description to make this clear.
> 
> > 
> > - Why do we have StreamListenSocket in parallel to the
> TCP[Client/Server]Socket
> > classes?
> 
> I don't know. :/ But maybe in the future, we could based it on top of
TCPSocket,
> too.
> 
> https://codereview.chromium.org/22861033/diff/71001/net/socket/tcp_socket.h
> File net/socket/tcp_socket.h (right):
> 
>
https://codereview.chromium.org/22861033/diff/71001/net/socket/tcp_socket.h#n...
> net/socket/tcp_socket.h:18: // TCPSocket provides a platform-indenpendent
> interface for TCP sockets.
> On 2013/08/30 19:17:51, Wez wrote:
> > typo: independent
> 
> Done.

Thanks for responding to my comments which, as you suggest, are beyond the scope
of this CL. LGTM

[commit-bot: I haz the power, 2013-09-01 08:19:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/22861033/87001

[commit-bot: I haz the power, 2013-09-01 23:04:30]

Change committed as 220752