Bug 4865: Enable connectivity when the remote peer is on public internet.

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 3f5aa0a1ff396cbb512d65166d46c54c598b70b9..81795d28583a273020a10d96afafa2c98751e131 100644
--- a/webrtc/p2p/client/basicportallocator.cc
+++ b/webrtc/p2p/client/basicportallocator.cc
@@ -21,6 +21,7 @@
 #include "webrtc/p2p/base/tcpport.h"
 #include "webrtc/p2p/base/turnport.h"
 #include "webrtc/p2p/base/udpport.h"
+#include "webrtc/base/checks.h"
 #include "webrtc/base/common.h"
 #include "webrtc/base/helpers.h"
 #include "webrtc/base/logging.h"
@@ -440,26 +441,33 @@ void BasicPortAllocatorSession::OnCandidateReady(
   if (data->complete())
     return;
 
-  // Send candidates whose protocol is enabled.
-  std::vector<Candidate> candidates;
   ProtocolType pvalue;
-  bool candidate_allowed_to_send = CheckCandidateFilter(c);
-  if (StringToProto(c.protocol().c_str(), &pvalue) &&
-      data->sequence()->ProtocolEnabled(pvalue) &&
-      candidate_allowed_to_send) {
+  bool candidate_type_enabled = CheckCandidateFilter(c);

[pthatcher1, 2015/08/07 21:28:19]

I think candidate_signalable would be a good name here.

[guoweis_webrtc, 2015/08/13 14:17:28]

Done.

+  bool candidate_is_any_address = c.address().IsAnyIP() && port->SharedSocket();

[pthatcher1, 2015/08/07 21:28:19]

I think something like candidate_unsignalable_but_pairable would be good, to
indicate that we wan't signal it, but we still want to pair it with signalled
candidates from the remote side.

Or if that name is too long, you could do:

bool candidate_pairable = candidate_signalable || (c.address().IsAnyIP() &&
port->SharedSocket());

[guoweis_webrtc, 2015/08/13 14:17:28]

I don't mind the long name. I think candidate_pairable is a bit too subtle.

+  bool candidate_protocol_enabled =
+      StringToProto(c.protocol().c_str(), &pvalue) &&
+      data->sequence()->ProtocolEnabled(pvalue);
+
+  // These can't be true at the same time. The any address candidate shouldn't
+  // be allowed to send.
+  DCHECK(!candidate_type_enabled || !candidate_is_any_address);

[pthatcher1, 2015/08/07 21:28:19]

DCHECK(!(candidate_type_enabled && candidate_is_any_address));

Would be more clear, and match the comment more clearly.

Or even better:

DCHECK(!(candidate_signalable && candidate_unsignalable_but_pairable))


Or just remove the DCHECK, since it's really covered by CheckCandidateFilter
anyway.

[guoweis_webrtc, 2015/08/13 14:17:28]

Done.

+
+  if (candidate_type_enabled && candidate_protocol_enabled) {

[pthatcher1, 2015/08/07 21:28:19]

if (candidate_signalable && candidate_protocol_enabled) {
  // ...
}

Would look nice here

[guoweis_webrtc, 2015/08/13 14:17:28]

Done.

+    std::vector<Candidate> candidates;
     candidates.push_back(c);
+    SignalCandidatesReady(this, candidates);
   }
 
-  if (!candidates.empty()) {
-    SignalCandidatesReady(this, candidates);
+  // Port has been made ready. Nothing to do here.
+  if (data->ready()) {
+    return;
   }
 
-  // Moving to READY state as we have atleast one candidate from the port.
-  // Since this port has atleast one candidate we should forward this port
-  // to listners, to allow connections from this port.
-  // Also we should make sure that candidate gathered from this port is allowed
-  // to send outside.
-  if (!data->ready() && candidate_allowed_to_send) {
+  // Move the port to the READY state, either because we have a usable candidate
+  // from the port, or simply because the port is bound to the any address and
+  // therefore has no host candidate. This will trigger the port to start
+  // creating candidate pairs (connections) and issue connectivity checks.
+  if (candidate_type_enabled || candidate_is_any_address) {

[pthatcher1, 2015/08/07 21:28:19]

if (candidate_pairable) {
  // ...
}

Would look nice here.

     data->set_ready();
     SignalPortReady(this, port);
   }
@@ -508,9 +516,10 @@ void BasicPortAllocatorSession::OnProtocolEnabled(AllocationSequence* seq,
       if (!CheckCandidateFilter(potentials[i]))
         continue;
       ProtocolType pvalue;
-      if (!StringToProto(potentials[i].protocol().c_str(), &pvalue))
-        continue;
-      if (pvalue == proto) {
+      bool candidate_protocol_enabled =
+          StringToProto(potentials[i].protocol().c_str(), &pvalue) &&
+          pvalue == proto;
+      if (candidate_protocol_enabled) {
         candidates.push_back(potentials[i]);
       }
     }