Add network cost as part of the connection comparison.

webrtc/p2p/base/port.cc

Index: webrtc/p2p/base/port.cc
diff --git a/webrtc/p2p/base/port.cc b/webrtc/p2p/base/port.cc
index 7993cc02bc082819340ca1e80b974dd91716f995..0f6a750d2efb8fd19e99e2eacb9224cbcf095981 100644
--- a/webrtc/p2p/base/port.cc
+++ b/webrtc/p2p/base/port.cc
@@ -195,6 +195,9 @@ void Port::Construct() {
     ice_username_fragment_ = rtc::CreateRandomString(ICE_UFRAG_LENGTH);
     password_ = rtc::CreateRandomString(ICE_PWD_LENGTH);
   }
+  // TODO(honghaiz): Make it configurable.
+  network_cost_ = (network_->type() == rtc::ADAPTER_TYPE_CELLULAR) ? 99 : 0;

[pthatcher1, 2016/02/04 23:16:20]

Yeah, I definitely would prefer an enum, something like:

enum CandidateCost {
  CANDIDATE_COST_NONE = 0,  // use "none" in SDP
  CANDIDATE_COST_HIGH = 1, // use "high" in SDP
  CANDIDATE_COST_LAST_RESTORT = 2 // use "last-restort" in SDP
}

[juberti2, 2016/02/04 23:31:10]

I don't really like the enum since it's not easily forward-extensible.

[pthatcher1, 2016/02/04 23:51:18]

The algorithm we have designed so far only works for certain know values: A/B/C,
or whatever we call them.  It's not designed for arbitrary cost values.  If we
choose A = 0 and C = 99 right now, we effectively have an enum, whether we call
it that or not.  If our algorithm works with specific values, I think it should
be an enum.  If it works with arbitrary values in a range, then we should make
it an integer.  But the later is not what we've designed so far.

[honghaiz3, 2016/02/05 01:36:46]

Perhaps, we can use enum to define a few integer values which can be used. 
like enum CandidateCost {
  CANDIDATE_COST_NONE = 0,  
  CANDIDATE_COST_LOW = 2, 
  CANDIDATE_COST_MEDIUM = 4, 
  CANDIDATE_COST_HIGH = 10, 
  CANDIDATE_COST_MAX = 99 
}
Will leave this open depending on further discussion. 

[pthatcher1, 2016/02/05 01:42:35]

Thinking about this some more: I do now see merit in using the same thing in the
SDP and the STUN attr (an integer or a string in both), since the mapping is
much easier.  And an integer is clearly better for the STUN packets, so you're
right about that.  Let's make it an int.

+
   LOG_J(LS_INFO, this) << "Port created";
 }
 
@@ -250,6 +253,7 @@ void Port::AddAddress(const rtc::SocketAddress& address,
   c.set_password(password_);
   c.set_network_name(network_->name());
   c.set_network_type(network_->type());
+  c.set_network_cost(network_cost_);
   c.set_generation(generation_);
   c.set_related_address(related_address);
   c.set_foundation(
@@ -692,6 +696,11 @@ class ConnectionRequest : public StunRequest {
           static_cast<uint32_t>(connection_->pings_since_last_response_.size() -
                                 1)));
     }
+    uint32_t network_cost = connection_->port()->network_cost();
+    if (network_cost > 0) {
+      request->AddAttribute(
+          new StunUInt32Attribute(STUN_ATTR_NETWORK_COST, network_cost));
+    }
 
     // Adding ICE_CONTROLLED or ICE_CONTROLLING attribute based on the role.
     if (connection_->port()->GetIceRole() == ICEROLE_CONTROLLING) {
@@ -1151,6 +1160,11 @@ std::string Connection::ToDebugId() const {
   return ss.str();
 }
 
+int Connection::ComputeNetworkCost() const {

[pthatcher1, 2016/02/04 23:16:20]

Everwhere you have "networkcost", please just make it "cost".  

[juberti2, 2016/02/04 23:31:10]

Not sure about this. Network cost seems clearer than just 'cost', which seems
like an antonym for 'priority'. Maybe this should be 'network_priority'.

[pthatcher1, 2016/02/04 23:51:18]

I don't feel strongly one way or the other, I just want it consistent.  If it's
"network cost" then the SDP should have "network-cost", not just "cost".

[honghaiz3, 2016/02/05 01:36:46]

When I put "cost" in the sdp, I intended to save a few bytes. Not sure if that
is super-critical though. 

[pthatcher1, 2016/02/12 00:07:12]

Saving bytes in SDP is of minor importance.  If someone wants to save bytes,
they shouldn't use SDP to start with.

Can you please just make it network-cost in the SDP to be consistent?

[honghaiz3, 2016/02/12 19:28:43]

Done.

+  // TODO(honghaiz): Will add rtt as part of the network cost.
+  return local_candidate().network_cost() + remote_candidate_.network_cost();
+}
+
 std::string Connection::ToString() const {
   const char CONNECT_STATE_ABBREV[2] = {
     '-',  // not connected (false)
@@ -1389,6 +1403,7 @@ void Connection::MaybeAddPrflxCandidate(ConnectionRequest* request,
   new_local_candidate.set_password(local_candidate().password());
   new_local_candidate.set_network_name(local_candidate().network_name());
   new_local_candidate.set_network_type(local_candidate().network_type());
+  new_local_candidate.set_network_cost(local_candidate().network_cost());
   new_local_candidate.set_related_address(local_candidate().address());
   new_local_candidate.set_foundation(ComputeFoundation(
       PRFLX_PORT_TYPE, local_candidate().protocol(),