Implement STUN support for P2P Transport API.

content/renderer/p2p/port_allocator.cc

+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/renderer/p2p/port_allocator.h"
+
+#include "base/bind.h"
+#include "content/renderer/p2p/host_address_request.h"
+#include "jingle/glue/utils.cc"
+#include "net/base/ip_endpoint.h"
+
+namespace content {
+
+P2PPortAllocator::P2PPortAllocator(
+    P2PSocketDispatcher* socket_dispatcher,
+    talk_base::NetworkManager* network_manager,
+    talk_base::PacketSocketFactory* socket_factory,
+    const webkit_glue::P2PTransport::Config& config)
+    : cricket::BasicPortAllocator(network_manager, socket_factory),
+      socket_dispatcher_(socket_dispatcher),
+      config_(config) {
+}
+
+P2PPortAllocator::~P2PPortAllocator() {
+}
+
+cricket::PortAllocatorSession* P2PPortAllocator::CreateSession(
+    const std::string& name,
+    const std::string& session_type) {
+  return new P2PPortAllocatorSession(this, name, session_type);
+}
+
+P2PPortAllocatorSession::P2PPortAllocatorSession(
+    P2PPortAllocator* allocator,
+    const std::string& name,
+    const std::string& session_type)
+    : cricket::BasicPortAllocatorSession(allocator, name, session_type),
+      allocator_(allocator),
+      stun_server_port_(0) {
+}
+
+P2PPortAllocatorSession::~P2PPortAllocatorSession() {
+  if (stun_address_request_)
+    stun_address_request_->Cancel();
+}
+
+void P2PPortAllocatorSession::GetPortConfigurations() {
+  // Add empty configuration synchronously, so local connection can be
+  // started immediately.

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

nit: Missing "an" and "a".

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

Done.

+  ConfigReady(new cricket::PortConfiguration(
+      talk_base::SocketAddress(), "", "", ""));
+
+  ResolveStunAddress();
+
+  // TODO(sergeyu): Implement relay server support.

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

Do you have a bug created to track that?

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

No, but I have a CL :)
There is crbug.com/41776 all work on this API.

+}
+
+void P2PPortAllocatorSession::ResolveStunAddress() {
+ if (allocator_->config_.stun_server.empty())
+   return;
+
+ std::string hostname;
+ if (!net::ParseHostAndPort(allocator_->config_.stun_server,
+                            &hostname, &stun_server_port_)) {
+   LOG(ERROR) << "Invalid stun server address: "
+              << allocator_->config_.stun_server;

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

Shouldn't this expose an error to the caller, rather than logging?

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

Yes, It should. I moved this to ppb_transport_impl.cc

+   return;
+ }
+
+ DCHECK(!stun_address_request_);
+ stun_address_request_ =
+     new P2PHostAddressRequest(allocator_->socket_dispatcher_);

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

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. :)

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

Added TODO for this in host_address_request.h.
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.

+ stun_address_request_->Request(hostname, base::Bind(
+     &P2PPortAllocatorSession::OnStunAddress,
+     base::Unretained(this)));
+}
+
+void P2PPortAllocatorSession::OnStunAddress(

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

nit: OnStunAddress->OnStunHostAddress.

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

Renamed it to OnStunServerAddress

+    const net::IPAddressNumber& address) {
+  talk_base::SocketAddress socket_address;
+  if (!jingle_glue::IPEndPointToSocketAddress(
+          net::IPEndPoint(address, stun_server_port_), &socket_address)) {

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

nit: I'd wrap &socket_address, too, for readability.

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

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. :)

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

Done.

+    return;
+  }
+
+  ConfigReady(new cricket::PortConfiguration(socket_address, "", "", ""));
+}
+
+}  // namespace content