Upstream changes making ListenSocket an abstract class.

Upstream changes making ListenSocket an abstract class.

This is part of Chrome for Android upstreaming.

This CL makes ListenSocket an abstract class instead of a concrete class
implementing a TCP Listen Socket. This abstraction will be used later to make
HttpServer seamlessly operate on TCP sockets and Unix domain sockets (will be
upstreamed in a separate CL).

The TCP Listen socket implementation, previously in listen_socket.{cc,h}, is
now in tcp_listen_socket.{cc,h}.

TEST=net_unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133480

[willchan no longer on Chromium, 2012-04-19 16:44:27]

+mmenke

Matt, I'm not sure how much you've looked at ListenSocket before. Take a look,
and if you're not comfortable approving, I'll take a look too. If you are
comfortable with it, go for it :)

[mmenke, 2012-04-20 17:48:34]

Just a couple nits.

http://codereview.chromium.org/10108015/diff/15001/net/base/listen_socket.h
File net/base/listen_socket.h (left):

http://codereview.chromium.org/10108015/diff/15001/net/base/listen_socket.h#o...
net/base/listen_socket.h:27: #include "base/basictypes.h"
You should still include this, for "DISALLOW_COPY_AND_ASSIGN".

http://codereview.chromium.org/10108015/diff/15001/net/base/listen_socket.h
File net/base/listen_socket.h (right):

http://codereview.chromium.org/10108015/diff/15001/net/base/listen_socket.h#n...
net/base/listen_socket.h:45: if (append_linefeed) SendInternal("\r\n", 2);
nit:  Please split this into two lines.  While the Google style guide allows it,
putting conditionals on a single line is pretty rare in the Chrome code base.

http://codereview.chromium.org/10108015/diff/15001/net/base/listen_socket.h#n...
net/base/listen_socket.h:60: ListenSocketDelegate *socket_delegate_;
I suggest you make this private and give it a protected accessor, to make clear
it's never modified by the subclasses.

http://codereview.chromium.org/10108015/diff/15001/net/base/tcp_listen_socket.h
File net/base/tcp_listen_socket.h (right):

http://codereview.chromium.org/10108015/diff/15001/net/base/tcp_listen_socket...
net/base/tcp_listen_socket.h:38: // Implements a raw socket interface
nit:  Suggest you add a comment that this is ref counted.

http://codereview.chromium.org/10108015/diff/15001/net/base/tcp_listen_socket...
net/base/tcp_listen_socket.h:73: virtual void SendInternal(const char* bytes,
int len) OVERRIDE;
nit:  Add a comment that this is the implementation of ListenSocket.

[Philippe, 2012-04-23 09:28:22]

I also did a sync which explains the new changes in the gyp file.

http://codereview.chromium.org/10108015/diff/15001/net/base/listen_socket.h
File net/base/listen_socket.h (left):

http://codereview.chromium.org/10108015/diff/15001/net/base/listen_socket.h#o...
net/base/listen_socket.h:27: #include "base/basictypes.h"
On 2012/04/20 17:48:34, Matt Menke wrote:
> You should still include this, for "DISALLOW_COPY_AND_ASSIGN".

Done.

http://codereview.chromium.org/10108015/diff/15001/net/base/listen_socket.h
File net/base/listen_socket.h (right):

http://codereview.chromium.org/10108015/diff/15001/net/base/listen_socket.h#n...
net/base/listen_socket.h:45: if (append_linefeed) SendInternal("\r\n", 2);
On 2012/04/20 17:48:34, Matt Menke wrote:
> nit:  Please split this into two lines.  While the Google style guide allows
it,
> putting conditionals on a single line is pretty rare in the Chrome code base.

Done.

http://codereview.chromium.org/10108015/diff/15001/net/base/listen_socket.h#n...
net/base/listen_socket.h:60: ListenSocketDelegate *socket_delegate_;
On 2012/04/20 17:48:34, Matt Menke wrote:
> I suggest you make this private and give it a protected accessor, to make
clear
> it's never modified by the subclasses.

I made the pointer non-reassignable which should have the same effect.
I have a next CL (http://codereview.chromium.org/10161005/) adding a common base
class, DefaultListenSocket, for TCPListenSocket and UnixDomainSocket. This CL is
also doing some polishing including the const I have just added here.

http://codereview.chromium.org/10108015/diff/15001/net/base/tcp_listen_socket.h
File net/base/tcp_listen_socket.h (right):

http://codereview.chromium.org/10108015/diff/15001/net/base/tcp_listen_socket...
net/base/tcp_listen_socket.h:38: // Implements a raw socket interface
On 2012/04/20 17:48:34, Matt Menke wrote:
> nit:  Suggest you add a comment that this is ref counted.

Done.

http://codereview.chromium.org/10108015/diff/15001/net/base/tcp_listen_socket...
net/base/tcp_listen_socket.h:73: virtual void SendInternal(const char* bytes,
int len) OVERRIDE;
On 2012/04/20 17:48:34, Matt Menke wrote:
> nit:  Add a comment that this is the implementation of ListenSocket.

Done.

[mmenke, 2012-04-23 14:57:39]

LGTM, modulo two small issues.

http://codereview.chromium.org/10108015/diff/26001/net/base/listen_socket.h
File net/base/listen_socket.h (right):

http://codereview.chromium.org/10108015/diff/26001/net/base/listen_socket.h#n...
net/base/listen_socket.h:48: }
You should de-inline these, per the Chromium style guide
(http://dev.chromium.org/developers/coding-style), which strongly discourages
inlining anything other than getters and setters.  Same goes for the
constructor/destructors.

There are a huge number of violations of this, but I prefer to stick to the
guide.

http://codereview.chromium.org/10108015/diff/26001/net/base/listen_socket.h#n...
net/base/listen_socket.h:55: friend class
base::RefCountedThreadSafe<ListenSocket>;
Know it was like this before, but while you're here, could you move this into
the private section, per the Google style guide?  Thanks.

[Philippe, 2012-04-23 15:17:24]

Thanks for the review Matt! I will send you another CL very soon.

http://codereview.chromium.org/10108015/diff/26001/net/base/listen_socket.h
File net/base/listen_socket.h (right):

http://codereview.chromium.org/10108015/diff/26001/net/base/listen_socket.h#n...
net/base/listen_socket.h:48: }
On 2012/04/23 14:57:39, Matt Menke wrote:
> You should de-inline these, per the Chromium style guide
> (http://dev.chromium.org/developers/coding-style), which strongly discourages
> inlining anything other than getters and setters.  Same goes for the
> constructor/destructors.
> 
> There are a huge number of violations of this, but I prefer to stick to the
> guide.

I agree. This is especially true with non-trivial constructors/destructors
(which can have an empty body but can still result in a lot of duplicated code
due to members construction, vptr initialization...).

http://codereview.chromium.org/10108015/diff/26001/net/base/listen_socket.h#n...
net/base/listen_socket.h:55: friend class
base::RefCountedThreadSafe<ListenSocket>;
On 2012/04/23 14:57:39, Matt Menke wrote:
> Know it was like this before, but while you're here, could you move this into
> the private section, per the Google style guide?  Thanks.

Done.

[commit-bot: I haz the power, 2012-04-23 16:48:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/10108015/38002

[commit-bot: I haz the power, 2012-04-23 18:23:42]

Change committed as 133480