[CrOS Tether] Sort Tether network lists.

chromeos/network/network_state_handler.cc

Index: chromeos/network/network_state_handler.cc
diff --git a/chromeos/network/network_state_handler.cc b/chromeos/network/network_state_handler.cc
index e95ba4f933e7f194fb4f7a195450912dac57e161..b23f0354d2ec2acd27513f28f16c24b7c9ed861a 100644
--- a/chromeos/network/network_state_handler.cc
+++ b/chromeos/network/network_state_handler.cc
@@ -538,6 +538,8 @@ void NetworkStateHandler::AddTetherNetworkState(const std::string& guid,
   tether_network_state->set_signal_strength(signal_strength);
 
   tether_network_list_.push_back(std::move(tether_network_state));
+  network_list_sorted_ = false;
+
   NotifyNetworkListChanged();
 }
 
@@ -562,6 +564,7 @@ bool NetworkStateHandler::UpdateTetherNetworkProperties(
   tether_network_state->set_carrier(carrier);
   tether_network_state->set_battery_percentage(battery_percentage);
   tether_network_state->set_signal_strength(signal_strength);
+  network_list_sorted_ = false;

[Ryan Hansberry, 2017/06/19 19:17:48]

I thought sorting was only determined by most recent TetherAvail and
ConnectTethering responses -- why would this affect sorting?

[Kyle Horimoto, 2017/06/20 23:44:54]

The algorithm used to sort is an implementation detail of the sorting delegate.
Embedding that logic here violates encapsulation. Imagine, for example, that a
future update to HostScanDevicePrioritizer sorts according to connection
strength. Sorting would be broken in this case.

TL;DR: We shouldn't assume the sorting algorithm used from this class.

 
   NotifyNetworkPropertiesUpdated(tether_network_state);
   return true;
@@ -581,6 +584,7 @@ bool NetworkStateHandler::SetTetherNetworkHasConnectedToHost(
   }
 
   tether_network_state->set_tether_has_connected_to_host(true);
+  network_list_sorted_ = false;
 
   NotifyNetworkPropertiesUpdated(tether_network_state);
   return true;
@@ -596,6 +600,8 @@ bool NetworkStateHandler::RemoveTetherNetworkState(const std::string& guid) {
         wifi_network->set_tether_guid(std::string());
 
       tether_network_list_.erase(iter);
+      network_list_sorted_ = false;

[Ryan Hansberry, 2017/06/19 19:17:49]

Is this true? Once an element is removed, I would assume the list remains
sorted.

[Kyle Horimoto, 2017/06/20 23:44:54]

You're right - removed.

+
       NotifyNetworkListChanged();
 
       return true;
@@ -634,6 +640,7 @@ bool NetworkStateHandler::DisassociateTetherNetworkStateFromWifiNetwork(
 
   wifi_network_state->set_tether_guid(std::string());
   tether_network_state->set_tether_guid(std::string());
+  network_list_sorted_ = false;
 
   NotifyNetworkPropertiesUpdated(wifi_network_state);
   NotifyNetworkPropertiesUpdated(tether_network_state);
@@ -681,6 +688,7 @@ bool NetworkStateHandler::AssociateTetherNetworkStateWithWifiNetwork(
 
   tether_network_state->set_tether_guid(wifi_network_guid);
   wifi_network_state->set_tether_guid(tether_network_guid);
+  network_list_sorted_ = false;
 
   NotifyNetworkPropertiesUpdated(wifi_network_state);
   NotifyNetworkPropertiesUpdated(tether_network_state);
@@ -739,8 +747,9 @@ void NetworkStateHandler::SetTetherNetworkStateConnectionState(
 
   std::string prev_connection_state = tether_network_state->connection_state();
   tether_network_state->set_connection_state(connection_state);
-  DCHECK(!tether_network_state->is_captive_portal());
+  network_list_sorted_ = false;

[Ryan Hansberry, 2017/06/19 19:17:48]

Again, I'm not sure how this relates to how HostScanDevicePrioritizer sorts
Tether networks.

[Kyle Horimoto, 2017/06/20 23:44:54]

Connecting/connected networks should be prioritized over other networks. When a
network is connected, the list should need to be re-sorted.

 
+  DCHECK(!tether_network_state->is_captive_portal());
   if (ConnectionStateChanged(tether_network_state, prev_connection_state,
                              false /* prev_is_captive_portal */)) {
     NET_LOG(EVENT) << "Changing connection state for Tether network with GUID "
@@ -752,6 +761,17 @@ void NetworkStateHandler::SetTetherNetworkStateConnectionState(
   }
 }
 
+void NetworkStateHandler::SetTetherNetworkListSorter(
+    const TetherNetworkListSorter* tether_network_list_sorter) {
+  if (tether_technology_state_ != TECHNOLOGY_ENABLED) {
+    NET_LOG(ERROR) << "SetTetherNetworkListSorter() called when Tether "
+                   << "networks are not enabled.";
+    return;
+  }
+
+  tether_network_list_sorter_ = tether_network_list_sorter;
+}
+
 void NetworkStateHandler::EnsureTetherDeviceState() {
   bool should_be_present =
       tether_technology_state_ != TechnologyState::TECHNOLOGY_UNAVAILABLE;
@@ -1207,8 +1227,10 @@ void NetworkStateHandler::ManagedStateListChanged(
   }
 }
 
-// TODO(khorimoto): Add sorting for the Tether network list as well.
 void NetworkStateHandler::SortNetworkList() {
+  if (tether_network_list_sorter_)

[Ryan Hansberry, 2017/06/19 19:17:49]

Check if network_list_sorted_ is false?

[Kyle Horimoto, 2017/06/20 23:44:54]

That's already done before SortNetworkList() is called.

+    tether_network_list_sorter_->SortTetherNetworkList(&tether_network_list_);

[Ryan Hansberry, 2017/06/19 19:17:49]

set network_list_sorted_ = true

[Kyle Horimoto, 2017/06/20 23:44:54]

That's already done at the end of this function.

+
   // Note: usually active networks will precede inactive networks, however
   // this may briefly be untrue during state transitions (e.g. a network may
   // transition to idle before the list is updated).