[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 49 matching lines @@
                             state->path().c_str());
 }
 
 std::string ValueAsString(const base::Value& value) {
   std::string vstr;
   base::JSONWriter::WriteWithOptions(
       value, base::JSONWriter::OPTIONS_OMIT_BINARY_VALUES, &vstr);
   return vstr.empty() ? "''" : vstr;
 }
 
+void EnsureActiveNetworksInFrontOfList(
+    NetworkStateHandler::NetworkStateList* list) {
+  size_t last_active_network_index = -1;

[stevenjb, 2017/06/22 00:25:19]

Don't use size_t with negative values, it's generally unsigned.

You can use std::numeric_limits<size_t>::max() (include <limits>), but then will
need to change the logic... see below...

[Kyle Horimoto, 2017/06/22 02:08:01]

Acknowledged.

+  for (size_t i = 0; i < list->size(); ++i) {
+    const NetworkState* network = list->at(i);
+    if (network->IsConnectingState() || network->IsConnectedState()) {

[stevenjb, 2017/06/22 00:25:19]

I started to make some suggestions here, but this is a) confusing and b) not
super efficient (inserting into a vector is slow), see alternate suggestion
below.

[Kyle Horimoto, 2017/06/22 02:08:01]

Acknowledged.

+      // 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);
+      }
+    }
+  }
+}
+
 }  // namespace
 
 const char NetworkStateHandler::kDefaultCheckPortalList[] =
     "ethernet,wifi,cellular";
 
 NetworkStateHandler::NetworkStateHandler() {}
 
 NetworkStateHandler::~NetworkStateHandler() {
   // Normally Shutdown() will get called in ~NetworkHandler, however unit
   // tests do not use that class so this needs to call Shutdown when we
@@ skipping 337 matching lines @@
                                                int limit,
                                                NetworkStateList* list) {
   DCHECK(list);
   list->clear();
 
   // Sort the network list if necessary.
   if (!network_list_sorted_)
     SortNetworkList();
 
   if (type.MatchesPattern(NetworkTypePattern::Tether()))
     GetTetherNetworkList(limit, list);

[stevenjb, 2017/06/22 00:25:19]

So, rather than using the kind of confusing + kind of inefficient function
above, what if we modify GetTetherNetworkList() to take an 'active' boolean that
would return just the active tether networks here, then...

[Kyle Horimoto, 2017/06/22 02:08:01]

Done.

 
   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;
-  }
-
-  for (auto iter = network_list_.begin(); iter != network_list_.end(); ++iter) {
+  // Only add more networks if the limit has not yet been reached.
+  for (auto iter = network_list_.begin();
+       iter != network_list_.end() && (limit == 0 || count < limit); ++iter) {

[stevenjb, 2017/06/22 00:25:19]

Now that we've changed the logic, this is probably more clear without |count|,
just use list->size() (which arguably would have been more clear before also).

[Kyle Horimoto, 2017/06/22 02:08:01]

Done.

     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;
+    }

[stevenjb, 2017/06/22 00:25:19]

Move all of the above logic into helper method (e.g.
IncludeNetworkInList(network, configured_only, visible_only)), then...

[Kyle Horimoto, 2017/06/22 02:08:01]

Done.

     if (network->type() == shill::kTypeEthernet) {
       // Ethernet networks should always be in front.
       list->insert(list->begin(), network);
     } else {
       list->push_back(network);
     }

[stevenjb, 2017/06/22 00:25:19]

break if this is not an active network (note: Ethernet will *always* be active)

[Kyle Horimoto, 2017/06/22 02:08:01]

Done.

-    if (limit > 0 && ++count >= limit)
-      break;
+    count++;
+  }
+
+  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.
+    EnsureActiveNetworksInFrontOfList(list);

[stevenjb, 2017/06/22 00:25:19]

Append GetTetherNetworkList(limit, list, active=false) here

Then continue iterating network_list and append inactive networks.

So the code will look like:

  GetTetherNetworkList(limit, list, true /* active */);

  for ((auto iter = network_list_.begin();
       iter != network_list_.end() && (limit == 0 || list->size() < limit);
       ++iter) {
    const NetworkState* network = (*iter)->AsNetworkState();
    if (!IncludeNetworkInList(network, configured_only, visible_only)
      continue;
    if (!IsActiveNetwork(network))
      break;
    if (network->type() == shill::kTypeEthernet) {
      // Ethernet networks should always be in front.
      list->insert(list->begin(), network);
    } else {
      list->push_back(network);
    }
  }

  GetTetherNetworkList(limit, list, false /* active */);

  for ((auto iter = network_list_.begin();
       iter != network_list_.end() && (limit == 0 || list->size() < limit);
       ++iter) {
    const NetworkState* network = (*iter)->AsNetworkState();
    if (!IncludeNetworkInList(network, configured_only, visible_only)
      continue;
    list->push_back(network);
  }

The advantage to this approach is that it is immediately clear that the order
is:
Active Tether
Active Shill networks
Inactive Tether
Inactive Shill networks

[Kyle Horimoto, 2017/06/22 02:08:01]

Done.

   }
 }
 
 void NetworkStateHandler::GetTetherNetworkList(int limit,
                                                NetworkStateList* list) {
   DCHECK(list);
   list->clear();
 
   if (!IsTechnologyEnabled(NetworkTypePattern::Tether()))
     return;
@@ skipping 65 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;
 
   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(
@@ skipping 33 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;
+
   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);
   }
 }
@@ skipping 446 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_sort_delegate_)
+    tether_sort_delegate_->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).
   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