Prepare for ICE-renomination

webrtc/p2p/base/p2ptransportchannel.cc

Index: webrtc/p2p/base/p2ptransportchannel.cc
diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc
index 5238da83ca9302954e00b4a5204fc5121eda6851..3dad97d05dfb904bfe58854582e80de739ab774a 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -1141,11 +1141,10 @@ int P2PTransportChannel::CompareConnections(
   if (ice_role_ == ICEROLE_CONTROLLED) {
     // Compare the connections based on the nomination states and the last data
     // received time if this is on the controlled side.
-    if (a->nominated() && !b->nominated()) {
-      return a_is_better;
-    }
-    if (!a->nominated() && b->nominated()) {
-      return b_is_better;
+    int cmp = a->nominated_value() - b->nominated_value();
+    if (cmp != 0) {
+      // A positive value indicates |a| is better.
+      return cmp;
     }
 
     if (a->last_data_received() > b->last_data_received()) {
@@ -1212,11 +1211,8 @@ void P2PTransportChannel::SortConnectionsAndUpdateState() {
   // The controlled side can prune only if the selected connection has been
   // nominated because otherwise it may prune the connection that will be
   // selected by the controlling side.
-  // TODO(honghaiz): This is not enough to prevent a connection from being
-  // pruned too early because with aggressive nomination, the controlling side
-  // will nominate every connection until it becomes writable.
   if (ice_role_ == ICEROLE_CONTROLLING ||
-      (selected_connection_ && selected_connection_->nominated())) {
+      (selected_connection_ && selected_connection_->nominated_value() > 0)) {
     PruneConnections();
   }
 
@@ -1586,28 +1582,32 @@ void P2PTransportChannel::MarkConnectionPinged(Connection* conn) {
 }
 
 // Apart from sending ping from |conn| this method also updates
-// |use_candidate_attr| flag. The criteria to update this flag is
-// explained below.
-// Set USE-CANDIDATE if doing ICE AND this channel is in CONTROLLING AND
-//    a) Channel is in FULL ICE AND
-//      a.1) |conn| is the selected connection OR
-//      a.2) there is no selected connection OR
-//      a.3) the selected connection is unwritable OR
-//      a.4) |conn| has higher priority than selected_connection.
-//    b) we're doing LITE ICE AND
-//      b.1) |conn| is the selected_connection AND
-//      b.2) |conn| is writable.
+// |use_candidate_attr| and |nominating_value| flags. The criteria to update
+// the flags is explained below.
+// Set the flags to nominate |conn| if this channel is in CONTROLLING AND
+//   1) |conn| is the selected_connection AND
+//   2) |conn| is writable.
+// This is very much like passive-aggressive nomination.
 void P2PTransportChannel::PingConnection(Connection* conn) {
-  bool use_candidate = false;
-  if (remote_ice_mode_ == ICEMODE_FULL && ice_role_ == ICEROLE_CONTROLLING) {
-    use_candidate =
-        (conn == selected_connection_) || (selected_connection_ == NULL) ||
-        (!selected_connection_->writable()) ||
-        (CompareConnectionCandidates(selected_connection_, conn) < 0);
-  } else if (remote_ice_mode_ == ICEMODE_LITE && conn == selected_connection_) {
-    use_candidate = selected_connection_->writable();
-  }
-  conn->set_use_candidate_attr(use_candidate);
+  bool use_candidate_attr = false;
+  int nominating_value = 0;
+  if (ice_role_ == ICEROLE_CONTROLLING) {
+    bool should_nominate =
+        (conn == selected_connection_) && selected_connection_->writable();
+    if (should_nominate) {
+      if (peer_supports_renomination_) {
+        if (last_nominating_connection_ != conn) {
+          ++nominating_value_;
+          last_nominating_connection_ = conn;
+        }
+        nominating_value = nominating_value_;

[Taylor Brandstetter, 2016/07/22 23:48:29]

Instead of using a temporary variable "nominating_value" and setting it outside
the "if" block, you can just set it here.

[honghaiz3, 2016/07/25 22:24:41]

That's not the intended behavior. 
A connection may be the selected one first and become not later.
In that case, we want to set nominating_value to 0 here. 
The same applies to the set_use_candidate_attr. 

[Taylor Brandstetter, 2016/07/27 22:20:55]

I understand. Previously, "set_nominating_value(0)" would have done nothing, but
now it has meaning.

+      } else {
+        use_candidate_attr = true;
+      }
+    }
+  }
+  conn->set_nominating_value(nominating_value);
+  conn->set_use_candidate_attr(use_candidate_attr);
   last_ping_sent_ms_ = rtc::TimeMillis();
   conn->Ping(last_ping_sent_ms_);
 }