Implement parts of the Mojo TCP interfaces.

Implement parts of the Mojo TCP interfaces.

Does some cleanup on the interfaces themselves and implements most of the plumbing except actually connecting, reading, and writing data to a connected socket. That will be done in a followup.

Renames TCPClientSocket to TCPConnectedSocket for clarity.

Implements a simple TCP server for testing and demo purposes.

Adds net address type convertrs from an un-checked-in patch by Yuzhu: https://codereview.chromium.org/596383002/

R=yzshen@chromium.org

Committed: https://crrev.com/b9f57823fd24753ed51e395486db293c5da5d817
Cr-Commit-Position: refs/heads/master@{#298931}

[brettw, 2014-10-06 23:25:56]

brettw@chromium.org changed reviewers:
+ viettrungluu@chromium.org, yzshen@chromium.org

[yzshen1, 2014-10-07 20:14:35]

https://codereview.chromium.org/634713002/diff/20001/mojo/examples/http_serve...
File mojo/examples/http_server/http_server.cc (right):

https://codereview.chromium.org/634713002/diff/20001/mojo/examples/http_serve...
mojo/examples/http_server/http_server.cc:12: #include
"mojo/public/cpp/utility/run_loop.h"
nit: this is not needed.

https://codereview.chromium.org/634713002/diff/20001/mojo/examples/http_serve...
mojo/examples/http_server/http_server.cc:51: net_address->ipv4 =
NetAddressIPv4::New().Pass();
nit:Pass() is not needed.

https://codereview.chromium.org/634713002/diff/20001/mojo/examples/http_serve...
mojo/examples/http_server/http_server.cc:61:
base::Bind(&HttpServerApp::OnSocketBound, base::Unretained(this)));
nit: It seems you probably don't need to depend on base. You could use what is
provided in mojo/public/cpp/bindings/callback.h

(But I am not sure what will be the policy about using base functionality in
"standalone" mojo applications. Do you know?)

https://codereview.chromium.org/634713002/diff/20001/mojo/services/network/ne...
File mojo/services/network/net_adapters.cc (right):

https://codereview.chromium.org/634713002/diff/20001/mojo/services/network/ne...
mojo/services/network/net_adapters.cc:14: error->description =
net::ErrorToString(error_code);
Maybe we should handle the error_code > 0 case (say, by not setting
description). IIRC, it will set "unknown" or something like that.

https://codereview.chromium.org/634713002/diff/20001/mojo/services/network/tc...
File mojo/services/network/tcp_bound_socket_impl.cc (right):

https://codereview.chromium.org/634713002/diff/20001/mojo/services/network/tc...
mojo/services/network/tcp_bound_socket_impl.cc:32: // TODO(brettw) can we set
the options after we start listening or connecting
I don't think so. :/ This call sets "address reuse" and I think that should be
done before bind.

https://codereview.chromium.org/634713002/diff/20001/mojo/services/network/tc...
mojo/services/network/tcp_bound_socket_impl.cc:33: // so we knoe whether to do
default options for client or server?
knoe->know

https://codereview.chromium.org/634713002/diff/20001/mojo/services/network/tc...
mojo/services/network/tcp_bound_socket_impl.cc:49: return
mojo::TypeConverter<NetAddressPtr, net::IPEndPoint>::Convert(
You could do
return NetAddress::From(resolved_local_address);

https://codereview.chromium.org/634713002/diff/20001/mojo/services/network/tc...
mojo/services/network/tcp_bound_socket_impl.cc:60:
callback.Run(MakeNetworkError(net::ERR_SOCKET_IS_CONNECTED));
Not quite sure whether this is the appropriate code for the server socket case.

https://codereview.chromium.org/634713002/diff/20001/mojo/services/network/tc...
mojo/services/network/tcp_bound_socket_impl.cc:66: if (result !=
net::ERR_IO_PENDING) {
I thought it should be net::OK?

https://codereview.chromium.org/634713002/diff/20001/mojo/services/network/tc...
File mojo/services/network/tcp_server_socket_impl.cc (right):

https://codereview.chromium.org/634713002/diff/20001/mojo/services/network/tc...
mojo/services/network/tcp_server_socket_impl.cc:45:
callback.Run(MakeNetworkError(result), NetAddressPtr());
We need to call into OnAcceptCompleted for this case, too. (at least for
net::OK)

https://codereview.chromium.org/634713002/diff/20001/mojo/services/network/tc...
mojo/services/network/tcp_server_socket_impl.cc:51:
pending_callback_.Run(MakeNetworkError(result), NetAddressPtr());
Please release those pending_XXX members.

https://codereview.chromium.org/634713002/diff/20001/mojo/services/network/tc...
mojo/services/network/tcp_server_socket_impl.cc:59:
mojo::TypeConverter<NetAddressPtr, net::IPEndPoint>::Convert(
you could do NetAddress::From(blah).

https://codereview.chromium.org/634713002/diff/20001/mojo/services/network/tc...
File mojo/services/network/tcp_server_socket_impl.h (right):

https://codereview.chromium.org/634713002/diff/20001/mojo/services/network/tc...
mojo/services/network/tcp_server_socket_impl.h:21:
TCPServerSocketImpl(scoped_ptr<net::TCPSocket> socket);
explicit?

https://codereview.chromium.org/634713002/diff/20001/mojo/services/public/int...
File mojo/services/public/interfaces/network/tcp_bound_socket.mojom (right):

https://codereview.chromium.org/634713002/diff/20001/mojo/services/public/int...
mojo/services/public/interfaces/network/tcp_bound_socket.mojom:18: // the bound
socket object will no longer
Please complete the comment.

https://codereview.chromium.org/634713002/diff/20001/mojo/services/public/int...
File mojo/services/public/interfaces/network/tcp_server_socket.mojom (right):

https://codereview.chromium.org/634713002/diff/20001/mojo/services/public/int...
mojo/services/public/interfaces/network/tcp_server_socket.mojom:13: // Waits for
an incoming connection request and hooks up a TCPClientSocket
TCPConnectedSocket, please.

[brettw, 2014-10-07 23:12:16]

Should have gotten the rest of the comments, thanks!

https://codereview.chromium.org/634713002/diff/40001/mojo/services/network/tc...
File mojo/services/network/tcp_server_socket_impl.cc (right):

https://codereview.chromium.org/634713002/diff/40001/mojo/services/network/tc...
mojo/services/network/tcp_server_socket_impl.cc:56: pending_client_socket_ =
InterfaceRequest<TCPConnectedSocket>();
Is this the right thing to do for an interface request? It's not clear to me if
this is sufficient to clean up all resources.

[yzshen1, 2014-10-07 23:51:36]

LGTM

https://codereview.chromium.org/634713002/diff/40001/mojo/services/network/ne...
File mojo/services/network/net_adapters.cc (right):

https://codereview.chromium.org/634713002/diff/40001/mojo/services/network/ne...
mojo/services/network/net_adapters.cc:14: if (error_code < 0)
nit: <= 0

https://codereview.chromium.org/634713002/diff/40001/mojo/services/network/tc...
File mojo/services/network/tcp_server_socket_impl.cc (right):

https://codereview.chromium.org/634713002/diff/40001/mojo/services/network/tc...
mojo/services/network/tcp_server_socket_impl.cc:56: pending_client_socket_ =
InterfaceRequest<TCPConnectedSocket>();
On 2014/10/07 23:12:16, brettw wrote:
> Is this the right thing to do for an interface request? It's not clear to me
if
> this is sufficient to clean up all resources.

It is sufficient. InterfaceRequest is just a wrapper of a message pipe.

[brettw, 2014-10-07 23:57:11]

The CQ bit was checked by brettw@chromium.org

[commit-bot: I haz the power, 2014-10-07 23:58:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/634713002/60001

[commit-bot: I haz the power, 2014-10-08 00:15:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-08 00:15:59]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[brettw, 2014-10-08 18:03:52]

The CQ bit was checked by brettw@chromium.org

[commit-bot: I haz the power, 2014-10-08 18:05:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/634713002/100001

[commit-bot: I haz the power, 2014-10-08 19:02:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-08 19:02:46]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[brettw, 2014-10-08 19:10:32]

The CQ bit was checked by brettw@chromium.org

[commit-bot: I haz the power, 2014-10-08 19:12:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/634713002/120001

[commit-bot: I haz the power, 2014-10-09 01:21:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-09 01:21:18]

Exceeded time limit waiting for builds to trigger.

[brettw, 2014-10-09 17:50:48]

Committed patchset #7 (id:120001) manually as
b9f57823fd24753ed51e395486db293c5da5d817 (presubmit successful).

[commit-bot: I haz the power, 2014-10-09 17:50:48]

Patchset 7 (id:??) landed as
https://crrev.com/b9f57823fd24753ed51e395486db293c5da5d817
Cr-Commit-Position: refs/heads/master@{#298931}