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..90c2619e92695cd0e37c50623d0646aacb0a149a 100644
--- a/webrtc/p2p/client/basicportallocator.cc
+++ b/webrtc/p2p/client/basicportallocator.cc
@@ -454,12 +454,15 @@ void BasicPortAllocatorSession::OnCandidateReady(
     SignalCandidatesReady(this, candidates);

[juberti1, 2015/08/06 00:35:26]

Will we push 0.0.0.0 to the application here? We want to avoid that.

[pthatcher1, 2015/08/06 01:06:19]

I believe candidate_allowed_to_send will be false because CheckCandidateFilter
filters it out.

[guoweis_webrtc, 2015/08/06 08:50:07]

Correct. It'll be filtered out

   }
 
-  // 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) {
+  // Moving to READY state as we have at least one candidate from the port or

[juberti1, 2015/08/06 00:35:26]

This comment could be worded better. It's important to understand why
candidate_allowed_to_send is false for the any address.

[guoweis_webrtc, 2015/08/06 08:50:07]

Done.

+  // the candidate has the any address. When this port has at least one
+  // candidate we should forward this port to listeners, to allow connections
+  // from this port. Also we should make sure that candidate gathered from this
+  // port is allowed to send outside. When the candidate address is the any
+  // address, we need to move the port to READY such that connectivity is
+  // possible if the remote peer has a routable IP address on public Internet.

[pthatcher1, 2015/08/06 01:06:19]

I think we can re-word this in terms of "we're ready if we have a candidate that
can send or a candidate that can receive" (see my next comment).

[guoweis_webrtc, 2015/08/06 08:50:06]

Done.

+  if (!data->ready() && (candidate_allowed_to_send ||
+                         (c.address().IsAnyIP() && port->SharedSocket()))) {

[pthatcher1, 2015/08/06 01:06:19]

I think this would be a lot more readable if we broke up CheckCandidateFilter
and this logic into two predicate functions:

CandidateCanReceive (worth signalling)
CandidateCanSend (worth pinging from)


That way, much of this method can be somewhat like:

if (CandidateCanReceive(c)) {
  std::vector<Candidate> candidates;
  candidates.push_back(c);
  SignalCandidatesReady(this, candidates);
}

if ((CandidateCanReceive(c) || CandidateCanSend(c)) && !data->ready()) {
  data->set_ready();
  SignalPortReady(this, port);
}

[guoweis_webrtc, 2015/08/06 08:50:07]

I rephrase it a bit. Send and Receive is not very clear to me. TCP active
candidate is worth to signal but can't receive, for example.

     data->set_ready();
     SignalPortReady(this, port);
   }