[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..873b759dd48c906c320bba99474508445f61b8bc 100644
--- a/chromeos/network/network_state_handler.cc
+++ b/chromeos/network/network_state_handler.cc
@@ -428,31 +428,58 @@ void NetworkStateHandler::GetNetworkListByType(const NetworkTypePattern& type,
 
   int count = list->size();
 
-  if (type.Equals(NetworkTypePattern::Tether()) ||
-      (limit != 0 && count >= limit)) {
-    // If only searching for Tether networks, there is no need to continue
-    // searching through other network types; likewise, if the limit has already
-    // been reached, there is no need to continue searching.
-    return;
+  // Only add more networks if the limit has not yet been reached.
+  if (limit == 0 || count < limit) {

[stevenjb, 2017/06/21 20:59:04]

Instead of indenting this, modify the test in lines 456/457 and move it to the
top of the for loop.

[Kyle Horimoto, 2017/06/21 22:17:07]

Done.

+    for (auto iter = network_list_.begin(); iter != network_list_.end();
+         ++iter) {
+      const NetworkState* network = (*iter)->AsNetworkState();
+      DCHECK(network);
+      if (!network->update_received() || !network->Matches(type))
+        continue;
+      if (configured_only && !network->IsInProfile())
+        continue;
+      if (visible_only && !network->visible())
+        continue;
+      if (network->type() == shill::kTypeWifi &&
+          !network->tether_guid().empty()) {
+        // Wi-Fi networks which are actually underlying Wi-Fi hotspots for a
+        // Tether network should not be returned since they should only be shown
+        // to the user as Tether networks.
+        continue;
+      }
+      if (network->type() == shill::kTypeEthernet) {
+        // Ethernet networks should always be in front.
+        list->insert(list->begin(), network);
+      } else {
+        list->push_back(network);
+      }
+      if (limit > 0 && ++count >= limit)
+        break;
+    }
   }
 
-  for (auto iter = network_list_.begin(); iter != network_list_.end(); ++iter) {
-    const NetworkState* network = (*iter)->AsNetworkState();
-    DCHECK(network);
-    if (!network->update_received() || !network->Matches(type))
-      continue;
-    if (configured_only && !network->IsInProfile())
-      continue;
-    if (visible_only && !network->visible())
-      continue;
-    if (network->type() == shill::kTypeEthernet) {
-      // Ethernet networks should always be in front.
-      list->insert(list->begin(), network);
-    } else {
-      list->push_back(network);
+  if (type.MatchesPattern(NetworkTypePattern::Tether())) {
+    // Tether networks were added to |list| above before non-Tether networks.
+    // Thus, if |type| includes Tether networks, it is possible that |list|
+    // contains some non-active Tether networks before some active (i.e.,
+    // connecting or connected) non-Tether networks. Active networks should
+    // always be listed first, so adjust the list accordingly.
+    size_t last_active_network_index = -1;
+    for (size_t i = 0; i < list->size(); ++i) {
+      const NetworkState* network = list->at(i);
+      if (network->IsConnectingState() || network->IsConnectedState()) {
+        // A new active network has been found in the list.
+        last_active_network_index++;
+
+        if (i != last_active_network_index) {
+          // If this is an active network that is not in the correct order
+          // (i.e., it is listed after non-active networks), move it to the
+          // correct spot.
+          list->erase(list->begin() + i);
+          list->insert(list->begin() + last_active_network_index, network);
+        }
+      }
     }

[stevenjb, 2017/06/21 20:59:04]

Can we move this logic to a separate helper?

[Kyle Horimoto, 2017/06/21 22:17:07]

Done.

-    if (limit > 0 && ++count >= limit)
-      break;
   }
 }
 
@@ -538,6 +565,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 +591,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;
 
   NotifyNetworkPropertiesUpdated(tether_network_state);
   return true;
@@ -581,6 +611,7 @@ bool NetworkStateHandler::SetTetherNetworkHasConnectedToHost(
   }
 
   tether_network_state->set_tether_has_connected_to_host(true);
+  network_list_sorted_ = false;
 
   NotifyNetworkPropertiesUpdated(tether_network_state);
   return true;
@@ -634,6 +665,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 +713,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 +772,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;
 
+  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 +786,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 +1252,10 @@ void NetworkStateHandler::ManagedStateListChanged(
   }
 }
 
-// TODO(khorimoto): Add sorting for the Tether network list as well.
 void NetworkStateHandler::SortNetworkList() {
+  if (tether_network_list_sorter_)
+    tether_network_list_sorter_->SortTetherNetworkList(&tether_network_list_);
+
   // 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).