Add DefaultListenSocket.

Refactor TCPListenSocket.
    
This is part of Chrome for Android upstreaming.
    
This CL adds a common base class, StreamListenSocket, providing a default implementation inherited by TCPListenSocket and the upcoming UnixDomainSocket.
That lets us share the common code used by both TCPListenSocket and UnixDomainSocket.
    
This also removes the recently introduced ListenSocket class which is unnecessary now we have StreamListenSocket.

TEST=net_unittests

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

[Philippe, 2012-04-24 12:49:04]

http://codereview.chromium.org/10161005/diff/1/net/socket/transport_client_so...
File net/socket/transport_client_socket_unittest.cc (right):

http://codereview.chromium.org/10161005/diff/1/net/socket/transport_client_so...
net/socket/transport_client_socket_unittest.cc:34: }  // namespace
This change is needed to make DefaultListenSocket's testing-only methods (e.g.
PauseReads()) private (instead of public initially). A friend declaration is
added to let net::TransportClientSocketTest use them.

[mmenke, 2012-04-25 17:25:30]

Sorry I didn't get to this yesterday.

A number of these comments are about the pre-existing state of the code - the
class hasn't received a whole lot of love and attention, but not suggesting
anything that should take too much effort.

I'm taking Thursday and Friday off.  I'll probably take a quick look at any
followup CL you submit over that time, but won't sign off on the change until
I've taken a closer look at the classes.  If this delay is a problem for you, we
can find someone else in the meantime.  I expect to sign off on things Monday. 
Sorry for any inconvenience.

http://codereview.chromium.org/10161005/diff/12003/net/base/default_listen_so...
File net/base/default_listen_socket.h (right):

http://codereview.chromium.org/10161005/diff/12003/net/base/default_listen_so...
net/base/default_listen_socket.h:5: // Default (abstract) listen socket
implementation that can be used by clients
Default isn't really an accurate description, or at least isn't very clear. 
Suggest you mention that this is a stream-based socket implementation, that this
handles reading and writing to the socket, but does not handle creating the
listen socket or connected sockets, which are handled by subclasses on creation
and in Accept, respectively.

http://codereview.chromium.org/10161005/diff/12003/net/base/default_listen_so...
net/base/default_listen_socket.h:41: class NET_EXPORT DefaultListenSocket
Suggest you call this ListenSocketBase.  It has two advantages over the current
name:

1)  In alphabetical sorting of files, it's right next to listen_socket.

2)  "default" isn't a terribly accurate description.

Other options would be ListenSocketStream, or StreamListenSocket, which would
perhaps be even more accurate.

http://codereview.chromium.org/10161005/diff/12003/net/base/default_listen_so...
net/base/default_listen_socket.h:41: class NET_EXPORT DefaultListenSocket
Are there any plans to make any other direct subclass of ListenSocket?

http://codereview.chromium.org/10161005/diff/12003/net/base/default_listen_so...
net/base/default_listen_socket.h:48: protected:
nit:  Please add a blank line above the "protected:"

http://codereview.chromium.org/10161005/diff/12003/net/base/default_listen_so...
net/base/default_listen_socket.h:61: static SOCKET Accept(SOCKET s);
There doesn't seem to be any reason to make the current Accept function static,
that I'm aware of.  I suggest just a normal protected function, and then it
looks like we could make socket_ private, unless it's needed by unit tests.

http://codereview.chromium.org/10161005/diff/12003/net/base/default_listen_so...
net/base/default_listen_socket.h:66: virtual void Listen();
It looks to me like Listen, Read, Close, and CloseSocket don't need to be
virtual (Even before you get rid of one HttpListenSocket::Listen and rename the
other, actually, but afterwards, too).

http://codereview.chromium.org/10161005/diff/12003/net/base/default_listen_so...
net/base/default_listen_socket.h:67: virtual void AcceptInternal() = 0;
This naming seems a little weird.  SendInternal is how things generally work:  A
public Send function does stuff and then call the private child class's Send
function.

With AcceptInternal, not only which one calls the other reversed, but Accept is
also a private function.

My suggestion is to rename AcceptInternal to Accept, and Accept to AcceptSocket.

http://codereview.chromium.org/10161005/diff/12003/net/base/default_listen_so...
net/base/default_listen_socket.h:97: friend class TransportClientSocketTest;
nit:  Please add a linebreak between the friend declaration and the first
function.

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

http://codereview.chromium.org/10161005/diff/12003/net/base/listen_socket.h#n...
net/base/listen_socket.h:32: // server is the original listening Socket,
connection is the new
nit:  |server|

http://codereview.chromium.org/10161005/diff/12003/net/base/tcp_listen_socket.cc
File net/base/tcp_listen_socket.cc (right):

http://codereview.chromium.org/10161005/diff/12003/net/base/tcp_listen_socket...
net/base/tcp_listen_socket.cc:31: TCPListenSocket*
TCPListenSocket::CreateAndListen(
nit:  While you're here, this function should have "// static" above it.

http://codereview.chromium.org/10161005/diff/12003/net/base/tcp_listen_socket...
net/base/tcp_listen_socket.cc:76: SOCKET conn =
DefaultListenSocket::Accept(socket_);
This is a little unusual.  Generally, a parent class's public Blah function
calls its child's private BlahInternal function, not the other way around.

I suggest either just overriding Accept, or renaming this to Accept and Accept
to something else (AcceptSocket, AcceptInternal, or something).

http://codereview.chromium.org/10161005/diff/12003/net/base/tcp_listen_socket...
net/base/tcp_listen_socket.cc:81: // it's up to the delegate to AddRef if it
wants to keep it around
nit:  While you're here, could you capitalize this and add a period?  Thanks.

http://codereview.chromium.org/10161005/diff/12003/net/tools/fetch/http_liste...
File net/tools/fetch/http_listen_socket.cc (right):

http://codereview.chromium.org/10161005/diff/12003/net/tools/fetch/http_liste...
net/tools/fetch/http_listen_socket.cc:16: // must run in the IO thread
nit:  While you're here, there should be a comment that this is a static
function.

Also, the current comments for these functions should be capitalized and have a
period, and moved into the header.

http://codereview.chromium.org/10161005/diff/12003/net/tools/fetch/http_liste...
File net/tools/fetch/http_listen_socket.h (right):

http://codereview.chromium.org/10161005/diff/12003/net/tools/fetch/http_liste...
net/tools/fetch/http_listen_socket.h:30: HttpListenSocket::Delegate* delegate);
Just so we have a consistent interface, could you rename this to
CreateAndListen, as with TCPListenSocket, and get rid of the Lsiten override
below?

[mmenke, 2012-04-25 17:27:20]

http://codereview.chromium.org/10161005/diff/12003/net/base/default_listen_so...
File net/base/default_listen_socket.h (right):

http://codereview.chromium.org/10161005/diff/12003/net/base/default_listen_so...
net/base/default_listen_socket.h:61: static SOCKET Accept(SOCKET s);
On 2012/04/25 17:25:31, Matt Menke wrote:
> There doesn't seem to be any reason to make the current Accept function
static,
> that I'm aware of.  I suggest just a normal protected function, and then it
> looks like we could make socket_ private, unless it's needed by unit tests.

Just to clarify, as a non-static function, it wouldn't take an argument, but
rather just use our private socket_.

[Philippe, 2012-04-30 06:55:21]

Hi Matt,

Thanks for your comments.

I'm in a team offsite until Thursday, therefore I won't have much time to
address your next comments before Thursday.

I merged the top-level abstract class ListenSocket into StreamListenSocket since
it was not subclassed by classes other than StreamListenSocket as you pointed
out. This increased the number of modified files although most of them only
contain renames.

Cheers,
Philippe.

https://chromiumcodereview.appspot.com/10161005/diff/12003/net/base/default_l...
File net/base/default_listen_socket.h (right):

https://chromiumcodereview.appspot.com/10161005/diff/12003/net/base/default_l...
net/base/default_listen_socket.h:5: // Default (abstract) listen socket
implementation that can be used by clients
On 2012/04/25 17:25:31, Matt Menke wrote:
> Default isn't really an accurate description, or at least isn't very clear. 
> Suggest you mention that this is a stream-based socket implementation, that
this
> handles reading and writing to the socket, but does not handle creating the
> listen socket or connected sockets, which are handled by subclasses on
creation
> and in Accept, respectively.

Indeed.

https://chromiumcodereview.appspot.com/10161005/diff/12003/net/base/default_l...
net/base/default_listen_socket.h:41: class NET_EXPORT DefaultListenSocket
On 2012/04/25 17:25:31, Matt Menke wrote:
> Suggest you call this ListenSocketBase.  It has two advantages over the
current
> name:
> 
> 1)  In alphabetical sorting of files, it's right next to listen_socket.
> 
> 2)  "default" isn't a terribly accurate description.
> 
> Other options would be ListenSocketStream, or StreamListenSocket, which would
> perhaps be even more accurate.

I liked StreamListenSocket.

https://chromiumcodereview.appspot.com/10161005/diff/12003/net/base/default_l...
net/base/default_listen_socket.h:41: class NET_EXPORT DefaultListenSocket
On 2012/04/25 17:25:31, Matt Menke wrote:
> Are there any plans to make any other direct subclass of ListenSocket?

I don't think so. I guess that your underlying suggestion was to merge
ListenSocket into this class. I made the change.

https://chromiumcodereview.appspot.com/10161005/diff/12003/net/base/default_l...
net/base/default_listen_socket.h:48: protected:
On 2012/04/25 17:25:31, Matt Menke wrote:
> nit:  Please add a blank line above the "protected:"

Done.

https://chromiumcodereview.appspot.com/10161005/diff/12003/net/base/default_l...
net/base/default_listen_socket.h:61: static SOCKET Accept(SOCKET s);
On 2012/04/25 17:27:20, Matt Menke wrote:
> On 2012/04/25 17:25:31, Matt Menke wrote:
> > There doesn't seem to be any reason to make the current Accept function
> static,
> > that I'm aware of.  I suggest just a normal protected function, and then it
> > looks like we could make socket_ private, unless it's needed by unit tests.
> 
> Just to clarify, as a non-static function, it wouldn't take an argument, but
> rather just use our private socket_.

Good point.

https://chromiumcodereview.appspot.com/10161005/diff/12003/net/base/default_l...
net/base/default_listen_socket.h:66: virtual void Listen();
On 2012/04/25 17:25:31, Matt Menke wrote:
> It looks to me like Listen, Read, Close, and CloseSocket don't need to be
> virtual (Even before you get rid of one HttpListenSocket::Listen and rename
the
> other, actually, but afterwards, too).

Done.

https://chromiumcodereview.appspot.com/10161005/diff/12003/net/base/default_l...
net/base/default_listen_socket.h:67: virtual void AcceptInternal() = 0;
On 2012/04/25 17:25:31, Matt Menke wrote:
> This naming seems a little weird.  SendInternal is how things generally work: 
A
> public Send function does stuff and then call the private child class's Send
> function.
> 
> With AcceptInternal, not only which one calls the other reversed, but Accept
is
> also a private function.
> 
> My suggestion is to rename AcceptInternal to Accept, and Accept to
AcceptSocket.

Done.

https://chromiumcodereview.appspot.com/10161005/diff/12003/net/base/default_l...
net/base/default_listen_socket.h:97: friend class TransportClientSocketTest;
On 2012/04/25 17:25:31, Matt Menke wrote:
> nit:  Please add a linebreak between the friend declaration and the first
> function.

Done.

https://chromiumcodereview.appspot.com/10161005/diff/12003/net/base/listen_so...
File net/base/listen_socket.h (right):

https://chromiumcodereview.appspot.com/10161005/diff/12003/net/base/listen_so...
net/base/listen_socket.h:32: // server is the original listening Socket,
connection is the new
On 2012/04/25 17:25:31, Matt Menke wrote:
> nit:  |server|

Done. Note that this interface is now part of StreamListenSocket (since I
removed listen_socket*).

https://chromiumcodereview.appspot.com/10161005/diff/12003/net/base/tcp_liste...
File net/base/tcp_listen_socket.cc (right):

https://chromiumcodereview.appspot.com/10161005/diff/12003/net/base/tcp_liste...
net/base/tcp_listen_socket.cc:31: TCPListenSocket*
TCPListenSocket::CreateAndListen(
On 2012/04/25 17:25:31, Matt Menke wrote:
> nit:  While you're here, this function should have "// static" above it.

Done.

https://chromiumcodereview.appspot.com/10161005/diff/12003/net/base/tcp_liste...
net/base/tcp_listen_socket.cc:76: SOCKET conn =
DefaultListenSocket::Accept(socket_);
On 2012/04/25 17:25:31, Matt Menke wrote:
> This is a little unusual.  Generally, a parent class's public Blah function
> calls its child's private BlahInternal function, not the other way around.
> 
> I suggest either just overriding Accept, or renaming this to Accept and Accept
> to something else (AcceptSocket, AcceptInternal, or something).

I renamed this method to Accept() and the other one to AcceptSocket().

https://chromiumcodereview.appspot.com/10161005/diff/12003/net/base/tcp_liste...
net/base/tcp_listen_socket.cc:81: // it's up to the delegate to AddRef if it
wants to keep it around
On 2012/04/25 17:25:31, Matt Menke wrote:
> nit:  While you're here, could you capitalize this and add a period?  Thanks.

Sure. I also fixed some other places.

https://chromiumcodereview.appspot.com/10161005/diff/12003/net/tools/fetch/ht...
File net/tools/fetch/http_listen_socket.cc (right):

https://chromiumcodereview.appspot.com/10161005/diff/12003/net/tools/fetch/ht...
net/tools/fetch/http_listen_socket.cc:16: // must run in the IO thread
On 2012/04/25 17:25:31, Matt Menke wrote:
> nit:  While you're here, there should be a comment that this is a static
> function.
> 
> Also, the current comments for these functions should be capitalized and have
a
> period, and moved into the header.

Done.

https://chromiumcodereview.appspot.com/10161005/diff/12003/net/tools/fetch/ht...
File net/tools/fetch/http_listen_socket.h (right):

https://chromiumcodereview.appspot.com/10161005/diff/12003/net/tools/fetch/ht...
net/tools/fetch/http_listen_socket.h:30: HttpListenSocket::Delegate* delegate);
On 2012/04/25 17:25:31, Matt Menke wrote:
> Just so we have a consistent interface, could you rename this to
> CreateAndListen, as with TCPListenSocket, and get rid of the Lsiten override
> below?

Sure.

[mmenke, 2012-04-30 19:11:44]

This is looking pretty good to me.  Just nits.

http://codereview.chromium.org/10161005/diff/12003/net/base/default_listen_so...
File net/base/default_listen_socket.h (right):

http://codereview.chromium.org/10161005/diff/12003/net/base/default_listen_so...
net/base/default_listen_socket.h:41: class NET_EXPORT DefaultListenSocket
On 2012/04/30 06:55:21, Philippe wrote:
> On 2012/04/25 17:25:31, Matt Menke wrote:
> > Are there any plans to make any other direct subclass of ListenSocket?
> 
> I don't think so. I guess that your underlying suggestion was to merge
> ListenSocket into this class. I made the change.

Yea, I was wondering if there was any need for it.

http://codereview.chromium.org/10161005/diff/29017/net/base/stream_listen_soc...
File net/base/stream_listen_socket.cc (right):

http://codereview.chromium.org/10161005/diff/29017/net/base/stream_listen_soc...
net/base/stream_listen_socket.cc:128: if (listen(socket_, backlog) == -1) {
nit:  Suggest you keep the error handling TODO here, since we aren't informing
anyone of the error.

http://codereview.chromium.org/10161005/diff/29017/net/base/stream_listen_soc...
File net/base/stream_listen_socket.h (right):

http://codereview.chromium.org/10161005/diff/29017/net/base/stream_listen_soc...
net/base/stream_listen_socket.h:10: // Server that handles IO asynchronously in
the specified MessageLoop. These
nit:  "Server that" -> "This class" or "StreamListenSocket".

http://codereview.chromium.org/10161005/diff/29017/net/base/stream_listen_soc...
net/base/stream_listen_socket.h:100: #if defined(OS_WIN)
Looks like this whole block (WIN and POSIX) can be made private.

http://codereview.chromium.org/10161005/diff/29017/net/base/stream_listen_soc...
net/base/stream_listen_socket.h:124: friend class TransportClientSocketTest;
This friend declaration should be above SendInternal.

http://codereview.chromium.org/10161005/diff/29017/net/base/tcp_listen_socket.cc
File net/base/tcp_listen_socket.cc (right):

http://codereview.chromium.org/10161005/diff/29017/net/base/tcp_listen_socket...
net/base/tcp_listen_socket.cc:43: : StreamListenSocket(s, del) {
nit:  Indent 4 spaces.

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

http://codereview.chromium.org/10161005/diff/29017/net/base/tcp_listen_socket...
net/base/tcp_listen_socket.h:25: // responsibility of the caller.
nit:  Think "The caller must take ownership of a pointer to the returned object"
is clearer.  Might also be a good idea to make it return a
scoped_refptr<TCPListenSocket> to make it even clearer.

http://codereview.chromium.org/10161005/diff/29017/net/base/tcp_listen_socket...
net/base/tcp_listen_socket.h:28: protected:
nit:  Line break needed above this line.

[Philippe, 2012-05-03 14:27:52]

http://codereview.chromium.org/10161005/diff/29017/net/base/stream_listen_soc...
File net/base/stream_listen_socket.cc (right):

http://codereview.chromium.org/10161005/diff/29017/net/base/stream_listen_soc...
net/base/stream_listen_socket.cc:128: if (listen(socket_, backlog) == -1) {
On 2012/04/30 19:11:44, Matt Menke wrote:
> nit:  Suggest you keep the error handling TODO here, since we aren't informing
> anyone of the error.

Done.

http://codereview.chromium.org/10161005/diff/29017/net/base/stream_listen_soc...
File net/base/stream_listen_socket.h (right):

http://codereview.chromium.org/10161005/diff/29017/net/base/stream_listen_soc...
net/base/stream_listen_socket.h:10: // Server that handles IO asynchronously in
the specified MessageLoop. These
On 2012/04/30 19:11:44, Matt Menke wrote:
> nit:  "Server that" -> "This class" or "StreamListenSocket".

Done.

http://codereview.chromium.org/10161005/diff/29017/net/base/stream_listen_soc...
net/base/stream_listen_socket.h:100: #if defined(OS_WIN)
Done.

http://codereview.chromium.org/10161005/diff/29017/net/base/stream_listen_soc...
net/base/stream_listen_socket.h:124: friend class TransportClientSocketTest;
On 2012/04/30 19:11:44, Matt Menke wrote:
> This friend declaration should be above SendInternal.

Done.

http://codereview.chromium.org/10161005/diff/29017/net/base/tcp_listen_socket.cc
File net/base/tcp_listen_socket.cc (right):

http://codereview.chromium.org/10161005/diff/29017/net/base/tcp_listen_socket...
net/base/tcp_listen_socket.cc:43: : StreamListenSocket(s, del) {
On 2012/04/30 19:11:44, Matt Menke wrote:
> nit:  Indent 4 spaces.

Done.

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

http://codereview.chromium.org/10161005/diff/29017/net/base/tcp_listen_socket...
net/base/tcp_listen_socket.h:25: // responsibility of the caller.
On 2012/04/30 19:11:44, Matt Menke wrote:
> nit:  Think "The caller must take ownership of a pointer to the returned
object"
> is clearer.  Might also be a good idea to make it return a
> scoped_refptr<TCPListenSocket> to make it even clearer.

Done.

http://codereview.chromium.org/10161005/diff/29017/net/base/tcp_listen_socket...
net/base/tcp_listen_socket.h:28: protected:
On 2012/04/30 19:11:44, Matt Menke wrote:
> nit:  Line break needed above this line.

Done.

[Philippe, 2012-05-03 14:30:35]

I'm adding Erik Wright for changes in chrome_frame/.

[erikwright (departed), 2012-05-03 18:10:04]

LGTM for the changes in chrome_frame/ .

[Philippe, 2012-05-04 11:50:45]

On 2012/05/03 18:10:04, erikwright wrote:
> LGTM for the changes in chrome_frame/ .

I'm currently fixing the new failures in the unit tests.

[mmenke, 2012-05-04 15:09:29]

On 2012/05/04 11:50:45, Philippe wrote:
> On 2012/05/03 18:10:04, erikwright wrote:
> > LGTM for the changes in chrome_frame/ .
> 
> I'm currently fixing the new failures in the unit tests.

The network stack team is having some discussion about the future of the class,
since it currently uses sync IO and was only originally intended for use in
testing, though it's also now used by devtools in production code.

What exactly are you using the class for?

[Philippe, 2012-05-04 15:18:05]

On 2012/05/04 15:09:29, Matt Menke wrote:
> On 2012/05/04 11:50:45, Philippe wrote:
> > On 2012/05/03 18:10:04, erikwright wrote:
> > > LGTM for the changes in chrome_frame/ .
> > 
> > I'm currently fixing the new failures in the unit tests.
> 
> The network stack team is having some discussion about the future of the
class,
> since it currently uses sync IO and was only originally intended for use in
> testing, though it's also now used by devtools in production code.
> 
> What exactly are you using the class for?

We made some changes internally allowing HttpServer to work on a
StreamListenSocket (as opposed to a concrete TCPListenSocket). That lets us make
DevTools use a UnixDomainSocket on Chrome for Android.

In these changes that I'm upstreaming we don't use TCPListenSocket directly
although we use UnixDomainSocket which inherits from a large part of
TCPListenSocket through the StreamListenSocket base class.

Are you planning to remove this class or refactor it?

[mnaganov (inactive), 2012-05-04 15:18:47]

On 2012/05/04 15:09:29, Matt Menke wrote:
> On 2012/05/04 11:50:45, Philippe wrote:
> > On 2012/05/03 18:10:04, erikwright wrote:
> > > LGTM for the changes in chrome_frame/ .
> > 
> > I'm currently fixing the new failures in the unit tests.
> 
> The network stack team is having some discussion about the future of the
class,
> since it currently uses sync IO and was only originally intended for use in
> testing, though it's also now used by devtools in production code.
> 
> What exactly are you using the class for?

Hi Matt,

This is also for DevTools from Chrome on Android. We have moved from TCP socket
to UDS socket for increased security (to only allow adb to connect), everything
else is the same.

[mmenke, 2012-05-04 15:20:26]

On 2012/05/04 15:18:47, Mikhail Naganov (Chromium) wrote:
> On 2012/05/04 15:09:29, Matt Menke wrote:
> > On 2012/05/04 11:50:45, Philippe wrote:
> > > On 2012/05/03 18:10:04, erikwright wrote:
> > > > LGTM for the changes in chrome_frame/ .
> > > 
> > > I'm currently fixing the new failures in the unit tests.
> > 
> > The network stack team is having some discussion about the future of the
> class,
> > since it currently uses sync IO and was only originally intended for use in
> > testing, though it's also now used by devtools in production code.
> > 
> > What exactly are you using the class for?
> 
> Hi Matt,
> 
> This is also for DevTools from Chrome on Android. We have moved from TCP
socket
> to UDS socket for increased security (to only allow adb to connect),
everything
> else is the same.

That's the discussion now.  It was just instigated this morning, or I would have
mentioned it earlier.  I'll rope you both in on the discussion.

[mmenke, 2012-05-04 17:41:04]

On 2012/05/04 15:20:26, Matt Menke wrote:
> On 2012/05/04 15:18:47, Mikhail Naganov (Chromium) wrote:
> > On 2012/05/04 15:09:29, Matt Menke wrote:
> > > On 2012/05/04 11:50:45, Philippe wrote:
> > > > On 2012/05/03 18:10:04, erikwright wrote:
> > > > > LGTM for the changes in chrome_frame/ .
> > > > 
> > > > I'm currently fixing the new failures in the unit tests.
> > > 
> > > The network stack team is having some discussion about the future of the
> > class,
> > > since it currently uses sync IO and was only originally intended for use
in
> > > testing, though it's also now used by devtools in production code.
> > > 
> > > What exactly are you using the class for?
> > 
> > Hi Matt,
> > 
> > This is also for DevTools from Chrome on Android. We have moved from TCP
> socket
> > to UDS socket for increased security (to only allow adb to connect),
> everything
> > else is the same.
> 
> That's the discussion now.  It was just instigated this morning, or I would
have
> mentioned it earlier.  I'll rope you both in on the discussion.

Sorry, forgot to CC you when I said I was going to do so.  I've CCed you both on
the thread, but basically:  The plan is to eventually get rid of the
TCPListenSocket in favor of TCPServerSocket (in net/socket), which already
supports asynchronous IO.

[Philippe, 2012-05-07 08:32:34]

On 2012/05/04 17:41:04, Matt Menke wrote:
> On 2012/05/04 15:20:26, Matt Menke wrote:
> > On 2012/05/04 15:18:47, Mikhail Naganov (Chromium) wrote:
> > > On 2012/05/04 15:09:29, Matt Menke wrote:
> > > > On 2012/05/04 11:50:45, Philippe wrote:
> > > > > On 2012/05/03 18:10:04, erikwright wrote:
> > > > > > LGTM for the changes in chrome_frame/ .
> > > > > 
> > > > > I'm currently fixing the new failures in the unit tests.
> > > > 
> > > > The network stack team is having some discussion about the future of the
> > > class,
> > > > since it currently uses sync IO and was only originally intended for use
> in
> > > > testing, though it's also now used by devtools in production code.
> > > > 
> > > > What exactly are you using the class for?
> > > 
> > > Hi Matt,
> > > 
> > > This is also for DevTools from Chrome on Android. We have moved from TCP
> > socket
> > > to UDS socket for increased security (to only allow adb to connect),
> > everything
> > > else is the same.
> > 
> > That's the discussion now.  It was just instigated this morning, or I would
> have
> > mentioned it earlier.  I'll rope you both in on the discussion.
> 
> Sorry, forgot to CC you when I said I was going to do so.  I've CCed you both
on
> the thread, but basically:  The plan is to eventually get rid of the
> TCPListenSocket in favor of TCPServerSocket (in net/socket), which already
> supports asynchronous IO.

On 2012/05/04 17:41:04, Matt Menke wrote:
> On 2012/05/04 15:20:26, Matt Menke wrote:
> > On 2012/05/04 15:18:47, Mikhail Naganov (Chromium) wrote:
> > > On 2012/05/04 15:09:29, Matt Menke wrote:
> > > > On 2012/05/04 11:50:45, Philippe wrote:
> > > > > On 2012/05/03 18:10:04, erikwright wrote:
> > > > > > LGTM for the changes in chrome_frame/ .
> > > > > 
> > > > > I'm currently fixing the new failures in the unit tests.
> > > > 
> > > > The network stack team is having some discussion about the future of the
> > > class,
> > > > since it currently uses sync IO and was only originally intended for use
> in
> > > > testing, though it's also now used by devtools in production code.
> > > > 
> > > > What exactly are you using the class for?
> > > 
> > > Hi Matt,
> > > 
> > > This is also for DevTools from Chrome on Android. We have moved from TCP
> > socket
> > > to UDS socket for increased security (to only allow adb to connect),
> > everything
> > > else is the same.
> > 
> > That's the discussion now.  It was just instigated this morning, or I would
> have
> > mentioned it earlier.  I'll rope you both in on the discussion.
> 
> Sorry, forgot to CC you when I said I was going to do so.  I've CCed you both
on
> the thread, but basically:  The plan is to eventually get rid of the
> TCPListenSocket in favor of TCPServerSocket (in net/socket), which already
> supports asynchronous IO.

Thanks Matt.

I CCed Pavel and William.

If I understand correctly Pavel is considering moving DevTools off IO as a short
term solution. I think that this would make this CL still valid.

However, later on when we move to TCPServerSocket, there might be some
refactoring needed again to make UnixDomainServerSocket (not upstreamed yet) a
subclass of ServerSocket (see net/socket/server_socket.h). ServerSocket was
apparently designed to abstract a TCP socket only. We need a more generic
abstraction to let HTTPServer (used by DevTools) operate seamlessly on
TCPServerSocket/UnixDomainServerSocket depending on whether we build for
Android.

What do you think?

[mmenke, 2012-05-07 15:01:46]

Assuming its use in devtools Android is analogous to its use in devtools on
other platforms, this sounds reasonable.  We should probably have an owner
for that refactoring Android-side, though it sounds to me like getting rid
of these classes is going to be a medium-to-long-term goal.

My main concerns here are adding a new dependency to a class we plan to get
rid of, and having a tentative path to eventually get rid of these classes.
 Since a functional Chrome for Android can't currently be built/tested from
trunk, we should probably have an owner for that refactoring Android-side.

On Mon, May 7, 2012 at 4:32 AM, <pliard@chromium.org> wrote:

> On 2012/05/04 17:41:04, Matt Menke wrote:
>
>> Sorry, forgot to CC you when I said I was going to do so.  I've CCed you
>> both
>
> on
>
>> the thread, but basically:  The plan is to eventually get rid of the
>> TCPListenSocket in favor of TCPServerSocket (in net/socket), which already
>> supports asynchronous IO.
>>
>
> Thanks Matt.
>
> I CCed Pavel and William.
>
> If I understand correctly Pavel is considering moving DevTools off IO as a
> short
> term solution. I think that this would make this CL still valid.
>
> However, later on when we move to TCPServerSocket, there might be some
> refactoring needed again to make UnixDomainServerSocket (not upstreamed
> yet) a
> subclass of ServerSocket (see net/socket/server_socket.h). ServerSocket was
> apparently designed to abstract a TCP socket only. We need a more generic
> abstraction to let HTTPServer (used by DevTools) operate seamlessly on
> TCPServerSocket/**UnixDomainServerSocket depending on whether we build for
> Android.
>
> What do you think?
>
>
https://chromiumcodereview.**appspot.com/10161005/<https://chromiumcodereview...
>

[pliard, 2012-05-07 15:22:42]

In DevTools Android, the use of UnixDomainSocket (with the linux-specific
abstract namespace capability) is only a way to increase security if I
remember correctly.

If no owner for the future refactoring comes to your mind, I could take
care of it. It should not be too much work. Erik Wright said he would be
interested in having his intern start on that though.

On Mon, May 7, 2012 at 5:01 PM, Matt Menke <mmenke@chromium.org> wrote:

> Assuming its use in devtools Android is analogous to its use in devtools
> on other platforms, this sounds reasonable.  We should probably have an
> owner for that refactoring Android-side, though it sounds to me like
> getting rid of these classes is going to be a medium-to-long-term goal.
>
> My main concerns here are adding a new dependency to a class we plan to
> get rid of, and having a tentative path to eventually get rid of these
> classes.  Since a functional Chrome for Android can't currently be
> built/tested from trunk, we should probably have an owner for that
> refactoring Android-side.
>
> On Mon, May 7, 2012 at 4:32 AM, <pliard@chromium.org> wrote:
>
>> On 2012/05/04 17:41:04, Matt Menke wrote:
>>
>>> Sorry, forgot to CC you when I said I was going to do so.  I've CCed you
>>> both
>>
>> on
>>
>>> the thread, but basically:  The plan is to eventually get rid of the
>>> TCPListenSocket in favor of TCPServerSocket (in net/socket), which
>>> already
>>> supports asynchronous IO.
>>>
>>
>> Thanks Matt.
>>
>> I CCed Pavel and William.
>>
>> If I understand correctly Pavel is considering moving DevTools off IO as
>> a short
>> term solution. I think that this would make this CL still valid.
>>
>> However, later on when we move to TCPServerSocket, there might be some
>> refactoring needed again to make UnixDomainServerSocket (not upstreamed
>> yet) a
>> subclass of ServerSocket (see net/socket/server_socket.h). ServerSocket
>> was
>> apparently designed to abstract a TCP socket only. We need a more generic
>> abstraction to let HTTPServer (used by DevTools) operate seamlessly on
>> TCPServerSocket/**UnixDomainServerSocket depending on whether we build
>> for
>> Android.
>>
>> What do you think?
>>
>>
https://chromiumcodereview.**appspot.com/10161005/<https://chromiumcodereview...
>>
>
>

[mmenke, 2012-05-07 20:50:27]

I expect to sign off on this pretty much as-is, barring any objections, which I
don't think are likely to happen, at this point.  However, there's another CL
out for review now that affects this file: 
http://codereview.chromium.org/10389007/.

Skimming over the CL, it looks to me like there are no significant conflicts
between the two - should just involve moving stuff around.  Current plan:  I
review the other CL, it lands tomorrow or Wednesday, you resolve any conflicts
(Again, I believe these will be trivial), I re-review, and you land.

I think we should land the other CL first because it's a smaller CL, and fixes
one of the main concerns about the classes involved, I hope you don't mind.

[mmenke, 2012-05-07 20:53:39]

On 2012/05/07 20:50:27, Matt Menke wrote:
> I expect to sign off on this pretty much as-is, barring any objections, which
I
> don't think are likely to happen, at this point.  However, there's another CL
> out for review now that affects this file: 
> http://codereview.chromium.org/10389007/.
> 
> Skimming over the CL, it looks to me like there are no significant conflicts
> between the two - should just involve moving stuff around.  Current plan:  I
> review the other CL, it lands tomorrow or Wednesday, you resolve any conflicts
> (Again, I believe these will be trivial), I re-review, and you land.
> 
> I think we should land the other CL first because it's a smaller CL, and fixes
> one of the main concerns about the classes involved, I hope you don't mind.

Well, that timeline is assuming all goes well, of course.

[Philippe, 2012-05-09 13:42:51]

On 2012/05/07 20:53:39, Matt Menke wrote:
> On 2012/05/07 20:50:27, Matt Menke wrote:
> > I expect to sign off on this pretty much as-is, barring any objections,
which
> I
> > don't think are likely to happen, at this point.  However, there's another
CL
> > out for review now that affects this file: 
> > http://codereview.chromium.org/10389007/.
> > 
> > Skimming over the CL, it looks to me like there are no significant conflicts
> > between the two - should just involve moving stuff around.  Current plan:  I
> > review the other CL, it lands tomorrow or Wednesday, you resolve any
conflicts
> > (Again, I believe these will be trivial), I re-review, and you land.
> > 
> > I think we should land the other CL first because it's a smaller CL, and
fixes
> > one of the main concerns about the classes involved, I hope you don't mind.
> 
> Well, that timeline is assuming all goes well, of course.

Great! I will do a sync once the other CL is submitted.

[Philippe, 2012-05-14 09:40:04]

On 2012/05/09 13:42:51, Philippe wrote:
> On 2012/05/07 20:53:39, Matt Menke wrote:
> > On 2012/05/07 20:50:27, Matt Menke wrote:
> > > I expect to sign off on this pretty much as-is, barring any objections,
> which
> > I
> > > don't think are likely to happen, at this point.  However, there's another
> CL
> > > out for review now that affects this file: 
> > > http://codereview.chromium.org/10389007/.
> > > 
> > > Skimming over the CL, it looks to me like there are no significant
conflicts
> > > between the two - should just involve moving stuff around.  Current plan: 
I
> > > review the other CL, it lands tomorrow or Wednesday, you resolve any
> conflicts
> > > (Again, I believe these will be trivial), I re-review, and you land.
> > > 
> > > I think we should land the other CL first because it's a smaller CL, and
> fixes
> > > one of the main concerns about the classes involved, I hope you don't
mind.
> > 
> > Well, that timeline is assuming all goes well, of course.
> 
> Great! I will do a sync once the other CL is submitted.

I did a sync on Friday after Marshall landed his change. Unfortunately it was
reverted due to a failing test. I guess that he's working on a fix and that the
current plan is still to wait that he lands his change first.

I have two CLs depending on this one. I will send them for review so that we can
move forward.

[mmenke, 2012-05-14 13:18:20]

On 2012/05/14 09:40:04, Philippe wrote:
> On 2012/05/09 13:42:51, Philippe wrote:
> > On 2012/05/07 20:53:39, Matt Menke wrote:
> > > On 2012/05/07 20:50:27, Matt Menke wrote:
> > > > I expect to sign off on this pretty much as-is, barring any objections,
> > which
> > > I
> > > > don't think are likely to happen, at this point.  However, there's
another
> > CL
> > > > out for review now that affects this file: 
> > > > http://codereview.chromium.org/10389007/.
> > > > 
> > > > Skimming over the CL, it looks to me like there are no significant
> conflicts
> > > > between the two - should just involve moving stuff around.  Current
plan: 
> I
> > > > review the other CL, it lands tomorrow or Wednesday, you resolve any
> > conflicts
> > > > (Again, I believe these will be trivial), I re-review, and you land.
> > > > 
> > > > I think we should land the other CL first because it's a smaller CL, and
> > fixes
> > > > one of the main concerns about the classes involved, I hope you don't
> mind.
> > > 
> > > Well, that timeline is assuming all goes well, of course.
> > 
> > Great! I will do a sync once the other CL is submitted.
> 
> I did a sync on Friday after Marshall landed his change. Unfortunately it was
> reverted due to a failing test. I guess that he's working on a fix and that
the
> current plan is still to wait that he lands his change first.
> 
> I have two CLs depending on this one. I will send them for review so that we
can
> move forward.

SGTM

[mmenke, 2012-05-15 15:46:50]

LGTM, though make sure that when you merge, the DCHECK in tcp_listen_socket.cc
goes away.

[mmenke, 2012-05-15 15:49:43]

On 2012/05/15 15:46:50, Matt Menke wrote:
> LGTM, though make sure that when you merge, the DCHECK in tcp_listen_socket.cc
> goes away.

Sorry, that should be the "DCHECK in stream_listen_socket.cc", but looks like
you've already done that.

[pliard, 2012-05-15 16:01:11]

Thanks for the review Matt!

Yes, I already did the merge. I was waiting for the win_rel build bot to
finish before I ping you. Now it's still failing but not because of me.

I will commit that and keep an eye on the build waterfall.

Thanks Matt for the review and
On Tue, May 15, 2012 at 5:49 PM, <mmenke@chromium.org> wrote:

> On 2012/05/15 15:46:50, Matt Menke wrote:
>
>> LGTM, though make sure that when you merge, the DCHECK in
>> tcp_listen_socket.cc
>> goes away.
>>
>
> Sorry, that should be the "DCHECK in stream_listen_socket.cc", but looks
> like
> you've already done that.
>
>
http://codereview.chromium.**org/10161005/<http://codereview.chromium.org/101...
>

[commit-bot: I haz the power, 2012-05-15 16:02:20]

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

[commit-bot: I haz the power, 2012-05-15 18:38:13]

Try job failure for 10161005-71040 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-16 08:06:50]

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

[commit-bot: I haz the power, 2012-05-16 09:22:33]

Change committed as 137387