[CrOS Tether] Sort Tether network lists.

chromeos/network/network_state_handler.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chromeos/network/network_state_handler.h"
 
 #include <stddef.h>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 520 matching lines @@
   tether_network_state->set_visible(true);
   tether_network_state->set_update_received();
   tether_network_state->set_update_requested(false);
   tether_network_state->set_connectable(true);
   tether_network_state->set_carrier(carrier);
   tether_network_state->set_battery_percentage(battery_percentage);
   tether_network_state->set_tether_has_connected_to_host(has_connected_to_host);
   tether_network_state->set_signal_strength(signal_strength);
 
   tether_network_list_.push_back(std::move(tether_network_state));
+  network_list_sorted_ = false;
+
   NotifyNetworkListChanged();
 }
 
 bool NetworkStateHandler::UpdateTetherNetworkProperties(
     const std::string& guid,
     const std::string& carrier,
     int battery_percentage,
     int signal_strength) {
   if (tether_technology_state_ != TECHNOLOGY_ENABLED) {
     NET_LOG(ERROR) << "UpdateTetherNetworkProperties() called when Tether "
                    << "networks are not enabled. Cannot update.";
     return false;
   }
 
   NetworkState* tether_network_state = GetModifiableNetworkStateFromGuid(guid);
   if (!tether_network_state) {
     NET_LOG(ERROR) << "UpdateTetherNetworkProperties(): No NetworkState for "
                    << "Tether network with GUID \"" << guid << "\".";
     return false;
   }
 
   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;
 }
 
 bool NetworkStateHandler::SetTetherNetworkHasConnectedToHost(
     const std::string& guid) {
   NetworkState* tether_network_state = GetModifiableNetworkStateFromGuid(guid);
   if (!tether_network_state) {
     NET_LOG(ERROR) << "SetTetherNetworkHasConnectedToHost(): No NetworkState "
                    << "for Tether network with GUID \"" << guid << "\".";
     return false;
   }
 
   if (tether_network_state->tether_has_connected_to_host()) {
     return false;
   }
 
   tether_network_state->set_tether_has_connected_to_host(true);
+  network_list_sorted_ = false;
 
   NotifyNetworkPropertiesUpdated(tether_network_state);
   return true;
 }
 
 bool NetworkStateHandler::RemoveTetherNetworkState(const std::string& guid) {
   for (auto iter = tether_network_list_.begin();
        iter != tether_network_list_.end(); ++iter) {
     if (iter->get()->AsNetworkState()->guid() == guid) {
       NetworkState* wifi_network = GetModifiableNetworkStateFromGuid(
           iter->get()->AsNetworkState()->tether_guid());
       if (wifi_network)
         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;
     }
   }
   return false;
 }
 
 bool NetworkStateHandler::DisassociateTetherNetworkStateFromWifiNetwork(
     const std::string& tether_network_guid) {
@@ skipping 18 matching lines @@
     return false;
   }
 
   if (wifi_network_state->tether_guid().empty() &&
       tether_network_state->tether_guid().empty()) {
     return true;
   }
 
   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);
 
   return true;
 }
 
 bool NetworkStateHandler::AssociateTetherNetworkStateWithWifiNetwork(
     const std::string& tether_network_guid,
     const std::string& wifi_network_guid) {
@@ skipping 27 matching lines @@
     return false;
   }
 
   if (wifi_network_state->tether_guid() == tether_network_guid &&
       tether_network_state->tether_guid() == wifi_network_guid) {
     return true;
   }
 
   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);
 
   return true;
 }
 
 void NetworkStateHandler::SetTetherNetworkStateDisconnected(
     const std::string& guid) {
   SetTetherNetworkStateConnectionState(guid, shill::kStateDisconnect);
@@ skipping 38 matching lines @@
     NET_LOG(ERROR) << "SetTetherNetworkStateConnectionState: Tether network "
                    << "not found: " << guid;
     return;
   }
 
   DCHECK(
       NetworkTypePattern::Tether().MatchesType(tether_network_state->type()));
 
   std::string prev_connection_state = tether_network_state->connection_state();
   tether_network_state->set_connection_state(connection_state);
+  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 "
                    << guid << ". Old state: " << prev_connection_state << ", "
                    << "New state: " << connection_state;
 
     OnNetworkConnectionStateChanged(tether_network_state);
     NotifyNetworkPropertiesUpdated(tether_network_state);
   }
 }
 
+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;
 
   for (auto it = device_list_.begin(); it < device_list_.end(); ++it) {
     std::string path = (*it)->path();
     if (path == kTetherDevicePath) {
       // If the Tether DeviceState is in the list and it should not be, remove
       // it and return. If it is in the list and it should be, the list is
       // already valid, so return without removing it.
@@ skipping 435 matching lines @@
         devices += ", ";
       devices += (*iter)->name();
     }
     NET_LOG_EVENT("DeviceList", devices);
     NotifyDeviceListChanged();
   } else {
     NOTREACHED();
   }
 }
 
-// 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).
   ManagedStateList active, non_wifi_visible, wifi_visible, hidden, new_networks;
   for (ManagedStateList::iterator iter = network_list_.begin();
        iter != network_list_.end(); ++iter) {
     NetworkState* network = (*iter)->AsNetworkState();
     if (!network->update_received()) {
       new_networks.push_back(std::move(*iter));
       continue;
@@ skipping 320 matching lines @@
   if (type.MatchesType(shill::kTypeVPN))
     technologies.emplace_back(shill::kTypeVPN);
   if (type.MatchesType(kTypeTether))
     technologies.emplace_back(kTypeTether);
 
   CHECK_GT(technologies.size(), 0ul);
   return technologies;
 }
 
 }  // namespace chromeos