Ensuring that UDP TURN servers are always used as STUN servers.

webrtc/p2p/client/basicportallocator.cc

Index: webrtc/p2p/client/basicportallocator.cc
diff --git a/webrtc/p2p/client/basicportallocator.cc b/webrtc/p2p/client/basicportallocator.cc
index 41edd85e9eb9ff5e901ad0baa75188da4888d6af..a5ee01cca69799bf4174a8786f681a822a44e5e3 100644
--- a/webrtc/p2p/client/basicportallocator.cc
+++ b/webrtc/p2p/client/basicportallocator.cc
@@ -925,18 +925,10 @@ void AllocationSequence::CreateUDPPorts() {
 
       // If STUN is not disabled, setting stun server address to port.
       if (!IsFlagSet(PORTALLOCATOR_DISABLE_STUN)) {
-        // If config has stun_servers, use it to get server reflexive candidate
-        // otherwise use first TURN server which supports UDP.
         if (config_ && !config_->StunServers().empty()) {

[pthatcher1, 2015/07/06 22:28:32]

Does it make sense to check for !empty() still?  Should we just always
set_server_addresses?

[Taylor Brandstetter, 2015/07/07 00:00:04]

It's a little redundant but at least it means you won't see a log message about
STUN candidates if there are none.

           LOG(LS_INFO) << "AllocationSequence: UDPPort will be handling the "
                        <<  "STUN candidate generation.";
           port->set_server_addresses(config_->StunServers());
-        } else if (config_ &&
-                   config_->SupportsProtocol(RELAY_TURN, PROTO_UDP)) {
-          port->set_server_addresses(config_->GetRelayServerAddresses(
-              RELAY_TURN, PROTO_UDP));
-          LOG(LS_INFO) << "AllocationSequence: TURN Server address will be "
-                       << " used for generating STUN candidate.";
         }
       }
     }
@@ -1169,6 +1161,15 @@ ServerAddresses PortConfiguration::StunServers() {
       stun_servers.find(stun_address) == stun_servers.end()) {
     stun_servers.insert(stun_address);
   }
+  // Every UDP TURN server should also be used as a STUN server.
+  ServerAddresses turn_servers = GetRelayServerAddresses(RELAY_TURN,
+                                                         PROTO_UDP);

[pthatcher1, 2015/07/06 22:28:32]

Please run "git cl format" and use whatever style it chooses here.

[pthatcher1, 2015/07/06 22:28:32]

Do we still need the check for SupportsProtocol(RELAY_TURN, PROTO_UDP)?  

[Taylor Brandstetter, 2015/07/07 00:00:04]

Done

[Taylor Brandstetter, 2015/07/07 00:00:04]

GetRelayServerAddresses(RELAY_TURN, PROTO_UDP) will do that for us.

+  for (ServerAddresses::iterator turn_server = turn_servers.begin();

[juberti1, 2015/06/29 18:54:48]

I think this could be done as 

for (auto turn_server : turn_servers)

[Taylor Brandstetter, 2015/06/29 20:18:34]

Good point. In this case I think I'll use "for (rtc::SocketAddress turn_server :
turn_servers)", since "auto" doesn't really improve readability.

[pthatcher1, 2015/07/06 22:28:32]

I think 

for (const rtc::SocketAddress& turn_server : turn_servers) {
  ...
}

Would be even better.  It's how we normally have it in the code base.

[Taylor Brandstetter, 2015/07/07 00:00:04]

Already did that. :)

+       turn_server != turn_servers.end(); ++turn_server) {
+    if (stun_servers.find(*turn_server) == stun_servers.end()) {
+      stun_servers.insert(*turn_server);
+    }
+  }
   return stun_servers;
 }