Signal to remove remote candidates if ports are pruned.

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 e94feb4d15b0e9aa8305ae03f46b7fe625b46d29..42ca32ee22c0bbb79dd63eb41b3d13a7592a0672 100644
--- a/webrtc/p2p/client/basicportallocator.cc
+++ b/webrtc/p2p/client/basicportallocator.cc
@@ -314,7 +314,13 @@ void BasicPortAllocatorSession::RegatherOnFailedNetworks() {
   }
   // Remove ports from being used locally and send signaling to remove
   // the candidates on the remote side.
-  RemovePortsAndCandidates(failed_networks);
+  std::vector<PortData*> port_data_list =

[pthatcher1, 2016/09/15 18:54:38]

Why not "ports_to_prune"?

[honghaiz3, 2016/09/16 01:41:29]

Done.

+      GetUnprunedPortsOnNetworks(failed_networks);

[pthatcher1, 2016/09/15 18:54:38]

I think "GetUnprunedPorts" would be sufficient.

[honghaiz3, 2016/09/16 01:41:29]

Done.

+  if (!port_data_list.empty()) {
+    LOG(LS_INFO) << "Pruned " << port_data_list.size()
+                 << " ports because their networks failed";

[pthatcher1, 2016/09/15 18:54:38]

If you do it *before* the line below, it should be "Prune X", and if you say
"Pruned X", it should go *after* the line below.

I'd prefer before and just change to "Prune"

[honghaiz3, 2016/09/16 01:41:29]

Done.

+    PrunePortsAndRemoveCandidates(port_data_list);
+  }
 
   if (allocation_started_ && network_manager_started_) {
     DoAllocate();
@@ -615,7 +621,13 @@ void BasicPortAllocatorSession::OnNetworksChanged() {
       failed_networks.push_back(sequence->network());
     }
   }
-  RemovePortsAndCandidates(failed_networks);
+  std::vector<PortData*> port_data_list =
+      GetUnprunedPortsOnNetworks(failed_networks);
+  if (!port_data_list.empty()) {
+    LOG(LS_INFO) << "Pruned " << port_data_list.size()
+                 << " ports because their networks were gone";
+    PrunePortsAndRemoveCandidates(port_data_list);

[pthatcher1, 2016/09/15 18:54:38]

Same here.

[honghaiz3, 2016/09/16 01:41:29]

Done.

+  }
 
   if (!network_manager_started_) {
     LOG(LS_INFO) << "Network manager is started";
@@ -757,29 +769,32 @@ bool BasicPortAllocatorSession::PruneTurnPorts(Port* newly_pairable_turn_port) {
   RTC_CHECK(best_turn_port != nullptr);
 
   bool pruned = false;
-  std::vector<PortInterface*> pruned_ports;
+  std::vector<PortData*> pruned_ports;

[pthatcher1, 2016/09/15 18:54:38]

If we don't prune these until the end, should we call this port_to_prune?

[honghaiz3, 2016/09/16 01:41:29]

Done.

   for (PortData& data : ports_) {
     if (data.port()->Network()->name() == network_name &&
         data.port()->Type() == RELAY_PORT_TYPE && !data.pruned() &&
         ComparePort(data.port(), best_turn_port) < 0) {
-      data.set_pruned();
       pruned = true;
-      data.port()->Prune();
       if (data.port() != newly_pairable_turn_port) {
-        pruned_ports.push_back(data.port());
+        // These ports will be pruned in PrunePortsAndRemoveCandidates.
+        pruned_ports.push_back(&data);
+      } else {
+        data.PrunePort();

[Taylor Brandstetter, 2016/09/15 00:43:17]

In this case, we call PrunePort and set "pruned" to true, but don't add it to
pruned_ports. This confused me at first. Maybe "pruned_ports" can be renamed to
"pruned_ports_with_signaled_candidates" or something similar?

[pthatcher1, 2016/09/15 18:54:38]

I think "ports_to_prune" would be a better name.

[honghaiz3, 2016/09/16 01:41:29]

Done.

       }
     }
   }
+
   if (!pruned_ports.empty()) {
-    LOG(LS_INFO) << "Pruned " << pruned_ports.size() << " ports";
-    SignalPortsPruned(this, pruned_ports);
+    LOG(LS_INFO) << "Pruned " << pruned_ports.size()
+                 << " low-priority Turn ports";

[Taylor Brandstetter, 2016/09/15 00:43:17]

nit: capitalize "TURN"

[honghaiz3, 2016/09/16 01:41:29]

Done.

+    PrunePortsAndRemoveCandidates(pruned_ports);
   }
   return pruned;
 }
 
 void BasicPortAllocatorSession::PruneAllPorts() {
   for (PortData& data : ports_) {
-    data.port()->Prune();
+    data.PrunePort();
   }
 }
 
@@ -942,31 +957,40 @@ BasicPortAllocatorSession::PortData* BasicPortAllocatorSession::FindPort(
   return NULL;
 }
 
-// Removes ports and candidates created on a given list of networks.
-void BasicPortAllocatorSession::RemovePortsAndCandidates(
+std::vector<BasicPortAllocatorSession::PortData*>
+BasicPortAllocatorSession::GetUnprunedPortsOnNetworks(
     const std::vector<rtc::Network*>& networks) {
-  std::vector<PortInterface*> ports_to_remove;
-  std::vector<Candidate> candidates_to_remove;
+  std::vector<PortData*> port_data_list;

[pthatcher1, 2016/09/15 18:54:38]

Why not "unpruned_ports"?

[honghaiz3, 2016/09/16 01:41:29]

Done.

   for (PortData& data : ports_) {

[pthatcher1, 2016/09/15 18:54:38]

Why not "port : ports_"?

[honghaiz3, 2016/09/16 01:41:29]

Done.

-    if (std::find(networks.begin(), networks.end(),
-                  data.sequence()->network()) == networks.end()) {
-      continue;
+    if (!data.pruned() &&
+        std::find(networks.begin(), networks.end(),
+                  data.sequence()->network()) != networks.end()) {
+      port_data_list.push_back(&data);
     }
+  }
+  return port_data_list;
+}
+
+void BasicPortAllocatorSession::PrunePortsAndRemoveCandidates(
+    const std::vector<PortData*>& port_data_list) {
+  std::vector<PortInterface*> ports_to_remove;
+  std::vector<Candidate> candidates_to_remove;
+  for (PortData* data : port_data_list) {
     // Prune the port so that it may be destroyed.
-    data.port()->Prune();
-    ports_to_remove.push_back(data.port());
-    if (data.has_pairable_candidate()) {
-      GetCandidatesFromPort(data, &candidates_to_remove);
+    data->PrunePort();
+    ports_to_remove.push_back(data->port());
+    if (data->has_pairable_candidate()) {
+      GetCandidatesFromPort(*data, &candidates_to_remove);
       // Mark the port as having no pairable candidates so that its candidates
       // won't be removed multiple times.
-      data.set_has_pairable_candidate(false);
+      data->set_has_pairable_candidate(false);
     }
   }
   if (!ports_to_remove.empty()) {
-    LOG(LS_INFO) << "Removed " << ports_to_remove.size() << " ports";
     SignalPortsPruned(this, ports_to_remove);
   }
   if (!candidates_to_remove.empty()) {
+    LOG(LS_INFO) << "Removed " << candidates_to_remove.size() << " candidates";
     SignalCandidatesRemoved(this, candidates_to_remove);
   }
 }