Added unit test for DevTools' ephemeral port support.

Added unit test for DevTools' ephemeral port support.

Necessarily added support for mocking StreamListenSocket::GetLocalAddress.
Also cleaned up the int-to-string conversion in DevToolsHttpHandlerImpl.

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

[Ken Russell (switch to Gerrit), 2014-06-09 21:55:26]

pfeldman, kaznacheev: please review content/browser/devtools/

jar, wtc: please review net/socket/

Thanks.

[jar (doing other things), 2014-06-09 23:13:40]

https://codereview.chromium.org/319593013/diff/1/net/socket/stream_listen_soc...
File net/socket/stream_listen_socket.h (right):

https://codereview.chromium.org/319593013/diff/1/net/socket/stream_listen_soc...
net/socket/stream_listen_socket.h:131: virtual int
GetLocalAddressImpl(IPEndPoint* address);
Should this be in the protected section?

nit: const method?

[Ken Russell (switch to Gerrit), 2014-06-09 23:32:12]

Also fixed the include order in the unit test.

https://codereview.chromium.org/319593013/diff/1/net/socket/stream_listen_soc...
File net/socket/stream_listen_socket.h (right):

https://codereview.chromium.org/319593013/diff/1/net/socket/stream_listen_soc...
net/socket/stream_listen_socket.h:131: virtual int
GetLocalAddressImpl(IPEndPoint* address);
On 2014/06/09 23:13:40, jar wrote:
> Should this be in the protected section?

Thanks for catching that. I don't know why it compiled in the first place, but
it definitely did. Fixed.

> nit: const method?

None of the existing methods like GetLocalAddress or GetPeerAddress are const.
If you'd like me to clean some of them up I'll do them all at once, but
otherwise I think it would look weird for this one method (and GetLocalAddress)
to be const.

[jar (doing other things), 2014-06-10 01:01:39]

net/* LGTM

[Ken Russell (switch to Gerrit), 2014-06-10 15:48:03]

+yurys

[wtc, 2014-06-10 17:46:44]

Patch set 2 LGTM. I think we should just declare
StreamListenSocket::GetPeerAddress as a virtual method.

https://codereview.chromium.org/319593013/diff/20001/content/browser/devtools...
File content/browser/devtools/devtools_http_handler_impl.cc (right):

https://codereview.chromium.org/319593013/diff/20001/content/browser/devtools...
content/browser/devtools/devtools_http_handler_impl.cc:703: if
(base::WriteFile(path, port_string.c_str(), port_string.length()) < 0) {

Nit: we can use data() instead of c_str(), which in theory does not require the
conversion to a C string.

https://codereview.chromium.org/319593013/diff/20001/net/socket/stream_listen...
File net/socket/stream_listen_socket.h (right):

https://codereview.chromium.org/319593013/diff/20001/net/socket/stream_listen...
net/socket/stream_listen_socket.h:104: virtual int
GetLocalAddressImpl(IPEndPoint* address);

Nit: please document how GetLocalAddressImpl differs from GetLocalAddress. They
have exactly the same prototype.

Update: after reading the entire CL, I now understand how you intend
GetLocalAddressImpl to be used. Why don't you just declare GetPeerAddress as a
virtual method?

[wtc, 2014-06-10 17:48:28]

https://codereview.chromium.org/319593013/diff/20001/net/socket/stream_listen...
File net/socket/stream_listen_socket.h (right):

https://codereview.chromium.org/319593013/diff/20001/net/socket/stream_listen...
net/socket/stream_listen_socket.h:104: virtual int
GetLocalAddressImpl(IPEndPoint* address);

Just to clarify: if you keep GetLocalAddressImpl, please document how it is
intended to be used, because the only way for a reader to figure that out now is
to read content/browser/devtools/devtools_http_handler_unittest.cc, which is
outside the net/ directory.

[Ken Russell (switch to Gerrit), 2014-06-10 18:57:07]

https://codereview.chromium.org/319593013/diff/20001/net/socket/stream_listen...
File net/socket/stream_listen_socket.h (right):

https://codereview.chromium.org/319593013/diff/20001/net/socket/stream_listen...
net/socket/stream_listen_socket.h:104: virtual int
GetLocalAddressImpl(IPEndPoint* address);
On 2014/06/10 17:46:44, wtc wrote:
> 
> Nit: please document how GetLocalAddressImpl differs from GetLocalAddress.
They
> have exactly the same prototype.
> 
> Update: after reading the entire CL, I now understand how you intend
> GetLocalAddressImpl to be used. Why don't you just declare GetPeerAddress as a
> virtual method?

Thanks for the suggestion. That's fine with me, and simpler. Made both
GetLocalAddress and GetPeerAddress virtual, and documented them as virtual for
unit test purposes.

[wtc, 2014-06-10 22:14:16]

Patch set 3 LGTM.

[yurys, 2014-06-11 05:17:08]

lgtm

[commit-bot: I haz the power, 2014-06-11 05:19:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kbr@chromium.org/319593013/40001

[commit-bot: I haz the power, 2014-06-11 11:22:31]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-06-11 15:21:58]

Change committed as 276405