Implement STUN support for P2P Transport API.

Implement STUN support for P2P Transport API.

BUG=41776
TEST=None

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

[Sergey Ulanov, 2011-08-26 22:33:52]

jam: please approve gyp changes
wez: please review code in content/renderer/p2p

[jam, 2011-08-26 23:38:50]

On 2011/08/26 22:33:52, sergeyu wrote:
> jam: please approve gyp changes

actually, you don't need approval for gyp changes. we specifically moved down
the OWNERS files from content to the sub dirs so that owners of subdirs under
renderer, for ex, don't need someone else to approve gyp changes.

> wez: please review code in content/renderer/p2p

[Wez, 2011-08-29 18:01:57]

LGTM but please see comments below.

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/p2p_tra...
File content/renderer/p2p/p2p_transport_impl.cc (right):

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/p2p_tra...
content/renderer/p2p/p2p_transport_impl.cc:65: // Use raw basic port allocator
is used for tests.
This comment needs rewording, although it could just be removed, I think, since
the test-only limitation is specified on the ctor definition already.

Is the P2P vs Basic port allocator usage here correct?  What does an application
do if it wants LAN-only peer-to-peer?  Seems preferable to avoid unnecessary
work on STUN, etc, in that case?

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/p2p_tra...
File content/renderer/p2p/p2p_transport_impl.h (right):

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/p2p_tra...
content/renderer/p2p/p2p_transport_impl.h:42: // Create P2PTransportImpl using
specified NetworkManager and
nit: Create->Creates.

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/p2p_tra...
content/renderer/p2p/p2p_transport_impl.h:44: // |socket_factory|. Must be used
for tests only.
You mean it's provided only for use by tests?

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/port_al...
File content/renderer/p2p/port_allocator.cc (right):

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/port_al...
content/renderer/p2p/port_allocator.cc:49: // started immediately.
nit: Missing "an" and "a".

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/port_al...
content/renderer/p2p/port_allocator.cc:55: // TODO(sergeyu): Implement relay
server support.
Do you have a bug created to track that?

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/port_al...
content/renderer/p2p/port_allocator.cc:66: << allocator_->config_.stun_server;
Shouldn't this expose an error to the caller, rather than logging?

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/port_al...
content/renderer/p2p/port_allocator.cc:72: new
P2PHostAddressRequest(allocator_->socket_dispatcher_);
Could this request class have a more descriptive name, e.g. 
P2PHostnameResolver?

I think you need to be careful in the P2P code about using the term "host",
since it is so often used to refer to the end of a P2P connection that is not
the initiator.  Initially I had assumed that this class was responsible for
doing STUN, because it sounded like it determined this P2P host's address. :)

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/port_al...
content/renderer/p2p/port_allocator.cc:78: void
P2PPortAllocatorSession::OnStunAddress(
nit: OnStunAddress->OnStunHostAddress.

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/port_al...
content/renderer/p2p/port_allocator.cc:82: net::IPEndPoint(address,
stun_server_port_), &socket_address)) {
nit: I'd wrap &socket_address, too, for readability.

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/port_al...
content/renderer/p2p/port_allocator.cc:82: net::IPEndPoint(address,
stun_server_port_), &socket_address)) {
Initially I thought this strange - it seemed to be creating a SocketAddress
containing the external IP address (determined via STUN) and the STUN server's
port number.  Don't we want to know _our_ external port, as reported by the STUN
server?

But of course what you're actually doing is resolving the DNS name of the STUN
server to an address. :)

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/port_al...
File content/renderer/p2p/port_allocator.h (right):

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/port_al...
content/renderer/p2p/port_allocator.h:49: // Override for
cricket::BasicPortAllocatorSession::GetPortConfigurations().
nit: Does this comment add anything?

[Sergey Ulanov, 2011-08-29 22:13:39]

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/p2p_tra...
File content/renderer/p2p/p2p_transport_impl.cc (right):

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/p2p_tra...
content/renderer/p2p/p2p_transport_impl.cc:65: // Use raw basic port allocator
is used for tests.
On 2011/08/29 18:01:57, Wez wrote:
> This comment needs rewording, although it could just be removed, I think,
since
> the test-only limitation is specified on the ctor definition already.

I think it's useful to have this comment here.

> Is the P2P vs Basic port allocator usage here correct?  What does an
application
> do if it wants LAN-only peer-to-peer?  Seems preferable to avoid unnecessary
> work on STUN, etc, in that case?
I plan to add flags to disable/enable certain types of ports. These flags will
be passed to P2PPortAllocator (as part of |config|). P2PPortAllocator will not
do any additional work if some ports are disabled.

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/p2p_tra...
File content/renderer/p2p/p2p_transport_impl.h (right):

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/p2p_tra...
content/renderer/p2p/p2p_transport_impl.h:42: // Create P2PTransportImpl using
specified NetworkManager and
On 2011/08/29 18:01:57, Wez wrote:
> nit: Create->Creates.

Done.

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/p2p_tra...
content/renderer/p2p/p2p_transport_impl.h:44: // |socket_factory|. Must be used
for tests only.
On 2011/08/29 18:01:57, Wez wrote:
> You mean it's provided only for use by tests?

Done.

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/port_al...
File content/renderer/p2p/port_allocator.cc (right):

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/port_al...
content/renderer/p2p/port_allocator.cc:49: // started immediately.
On 2011/08/29 18:01:57, Wez wrote:
> nit: Missing "an" and "a".

Done.

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/port_al...
content/renderer/p2p/port_allocator.cc:55: // TODO(sergeyu): Implement relay
server support.
On 2011/08/29 18:01:57, Wez wrote:
> Do you have a bug created to track that?
No, but I have a CL :)
There is crbug.com/41776 all work on this API.

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/port_al...
content/renderer/p2p/port_allocator.cc:66: << allocator_->config_.stun_server;
On 2011/08/29 18:01:57, Wez wrote:
> Shouldn't this expose an error to the caller, rather than logging?
Yes, It should. I moved this to ppb_transport_impl.cc

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/port_al...
content/renderer/p2p/port_allocator.cc:72: new
P2PHostAddressRequest(allocator_->socket_dispatcher_);
On 2011/08/29 18:01:57, Wez wrote:
> Could this request class have a more descriptive name, e.g. 
> P2PHostnameResolver?
Added TODO for this in host_address_request.h.

> 
> I think you need to be careful in the P2P code about using the term "host",
> since it is so often used to refer to the end of a P2P connection that is not
> the initiator. 
There is no notion of initiator in the Transport API: it is symmetric in terms
of how the API is used on then ends. But, yes, I see how this term may be
confusing in this context.

> Initially I had assumed that this class was responsible for
> doing STUN, because it sounded like it determined this P2P host's address. :)

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/port_al...
content/renderer/p2p/port_allocator.cc:78: void
P2PPortAllocatorSession::OnStunAddress(
On 2011/08/29 18:01:57, Wez wrote:
> nit: OnStunAddress->OnStunHostAddress.

Renamed it to OnStunServerAddress

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/port_al...
content/renderer/p2p/port_allocator.cc:82: net::IPEndPoint(address,
stun_server_port_), &socket_address)) {
On 2011/08/29 18:01:57, Wez wrote:
> nit: I'd wrap &socket_address, too, for readability.

Done.

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/port_al...
File content/renderer/p2p/port_allocator.h (right):

http://codereview.chromium.org/7762008/diff/2001/content/renderer/p2p/port_al...
content/renderer/p2p/port_allocator.h:49: // Override for
cricket::BasicPortAllocatorSession::GetPortConfigurations().
On 2011/08/29 18:01:57, Wez wrote:
> nit: Does this comment add anything?
I think it's useful to annotate overridden methods to make it clear which
methods they override. It may be redundant in this case because there is only
one parent class, but I think it's better to keep it for consistency. I've
changed it to "Overrides for cricket::BasicPortAllocatorSession."