Rely on Service.Visible instead of Manager.Services

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 "base/bind.h"
 #include "base/format_macros.h"
 #include "base/guid.h"
 #include "base/location.h"
@@ skipping 38 matching lines @@
     return "None";
   return base::StringPrintf("%s (%s)", state->name().c_str(),
                             state->path().c_str());
 }
 
 }  // namespace
 
 const char NetworkStateHandler::kDefaultCheckPortalList[] =
     "ethernet,wifi,cellular";
 
-NetworkStateHandler::NetworkStateHandler() {
+NetworkStateHandler::NetworkStateHandler()
+    : network_list_sorted_(false) {
 }
 
 NetworkStateHandler::~NetworkStateHandler() {
   FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_, IsShuttingDown());
   STLDeleteContainerPointers(network_list_.begin(), network_list_.end());
   STLDeleteContainerPointers(device_list_.begin(), device_list_.end());
 }
 
 void NetworkStateHandler::InitShillPropertyHandler() {
   shill_property_handler_.reset(new internal::ShillPropertyHandler(this));
@@ skipping 109 matching lines @@
 }
 
 const NetworkState* NetworkStateHandler::DefaultNetwork() const {
   if (default_network_path_.empty())
     return NULL;
   return GetNetworkState(default_network_path_);
 }
 
 const NetworkState* NetworkStateHandler::ConnectedNetworkByType(
     const NetworkTypePattern& type) const {
+  // Active networks are always listed first by Shill so no need to sort.
   for (ManagedStateList::const_iterator iter = network_list_.begin();
        iter != network_list_.end(); ++iter) {
     const NetworkState* network = (*iter)->AsNetworkState();
     DCHECK(network);
     if (!network->update_received())
       continue;
     if (!network->IsConnectedState())
       break;  // Connected networks are listed first.
     if (network->Matches(type))
       return network;
   }
   return NULL;
 }
 
 const NetworkState* NetworkStateHandler::ConnectingNetworkByType(
     const NetworkTypePattern& type) const {
+  // Active networks are always listed first by Shill so no need to sort.
   for (ManagedStateList::const_iterator iter = network_list_.begin();
        iter != network_list_.end(); ++iter) {
     const NetworkState* network = (*iter)->AsNetworkState();
     DCHECK(network);
     if (!network->update_received() || network->IsConnectedState())
       continue;
     if (!network->IsConnectingState())
       break;  // Connected and connecting networks are listed first.
     if (network->Matches(type))
       return network;
   }
   return NULL;
 }
 
 const NetworkState* NetworkStateHandler::FirstNetworkByType(
-    const NetworkTypePattern& type) const {
+    const NetworkTypePattern& type) {
+  if (!network_list_sorted_)

[pneubeck (no reviews), 2014/06/18 09:45:48]

optional nit: 
you could move network_list_sorted_ into SortNetworkList.

(If you find the function name unfitting, could be renamed then to
EnsureNetworkListSorted)

[stevenjb, 2014/06/18 22:33:26]

Yeah, I considered that, but I kind of want to force a sort after a complete
update just in case I missed an edge case, and don't want to add a parameter
'force'. Since we only do this in a couple of places I thought this was more
readable.

[pneubeck (no reviews), 2014/06/18 22:35:29]

I see. that's fine.

+    SortNetworkList();  // Sort to ensure visible networks are listed fist.

[pneubeck (no reviews), 2014/06/18 09:45:49]

fist -> first

[stevenjb, 2014/06/18 22:33:26]

Done.

   for (ManagedStateList::const_iterator iter = network_list_.begin();
        iter != network_list_.end(); ++iter) {
     const NetworkState* network = (*iter)->AsNetworkState();
     DCHECK(network);
     if (!network->update_received())
       continue;
     if (!network->visible())
-      continue;
+      break;
     if (network->Matches(type))
       return network;
   }
   return NULL;
 }
 
 std::string NetworkStateHandler::FormattedHardwareAddressForType(
     const NetworkTypePattern& type) const {
   const DeviceState* device = NULL;
   const NetworkState* network = ConnectedNetworkByType(type);
   if (network)
     device = GetDeviceState(network->device_path());
   else
     device = GetDeviceStateByType(type);
   if (!device)
     return std::string();
   return network_util::FormattedMacAddress(device->mac_address());
 }
 
 void NetworkStateHandler::GetVisibleNetworkListByType(
     const NetworkTypePattern& type,
-    NetworkStateList* list) const {
+    NetworkStateList* list) {
   GetNetworkListByType(type,
                        false /* configured_only */,
                        true /* visible_only */,
                        0 /* no limit */,
                        list);
 }
 
-void NetworkStateHandler::GetVisibleNetworkList(NetworkStateList* list) const {
+void NetworkStateHandler::GetVisibleNetworkList(NetworkStateList* list) {
   GetVisibleNetworkListByType(NetworkTypePattern::Default(), list);
 }
 
 void NetworkStateHandler::GetNetworkListByType(const NetworkTypePattern& type,
                                                bool configured_only,
                                                bool visible_only,
                                                int limit,
-                                               NetworkStateList* list) const {
+                                               NetworkStateList* list) {
   DCHECK(list);
   list->clear();
   int count = 0;
+  // Sort the network list if necessary.
+  if (!network_list_sorted_)
+    SortNetworkList();
   for (ManagedStateList::const_iterator 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;
@@ skipping 84 matching lines @@
     network->clear_last_error();
 }
 
 void NetworkStateHandler::SetCheckPortalList(
     const std::string& check_portal_list) {
   NET_LOG_EVENT("SetCheckPortalList", check_portal_list);
   shill_property_handler_->SetCheckPortalList(check_portal_list);
 }
 
 const NetworkState* NetworkStateHandler::GetEAPForEthernet(
-    const std::string& service_path) const {
+    const std::string& service_path) {
   const NetworkState* network = GetNetworkState(service_path);
   if (!network) {
     NET_LOG_ERROR("GetEAPForEthernet", "Unknown service path " + service_path);
     return NULL;
   }
   if (network->type() != shill::kTypeEthernet) {
     NET_LOG_ERROR("GetEAPForEthernet", "Not of type Ethernet: " + service_path);
     return NULL;
   }
   if (!network->IsConnectedState())
@@ skipping 72 matching lines @@
     } else {
       managed_list->push_back(found->second);
       managed_map.erase(found);
     }
     list_entries.insert(path);
   }
   // Delete any remaining entries in managed_map.
   STLDeleteContainerPairSecondPointers(managed_map.begin(), managed_map.end());
 }
 
-void NetworkStateHandler::UpdateVisibleNetworks(
-    const base::ListValue& entries) {
-  NET_LOG_DEBUG(base::StringPrintf("UpdateVisibleNetworks"),
-                base::StringPrintf("%" PRIuS, entries.GetSize()));
-  // Create a map of all networks and clear the visible state.
-  ManagedStateList* network_list =
-      GetManagedList(ManagedState::MANAGED_TYPE_NETWORK);
-  typedef std::map<std::string, NetworkState*> NetworkMap;
-  NetworkMap network_map;
-  for (ManagedStateList::iterator iter = network_list->begin();
-       iter != network_list->end(); ++iter) {
-    NetworkState* network = (*iter)->AsNetworkState();
-    network_map[network->path()] = network;
-    network->set_visible(false);
-  }
-  // Look up each entry and set the associated network to visible.
-  for (base::ListValue::const_iterator iter = entries.begin();
-       iter != entries.end(); ++iter) {
-    std::string path;
-    (*iter)->GetAsString(&path);
-    NetworkMap::iterator found = network_map.find(path);
-    if (found != network_map.end())
-      found->second->set_visible(true);
-    else
-      NET_LOG_DEBUG("Visible network not in list", path);
-  }
-}
-
 void NetworkStateHandler::ProfileListChanged() {
   NET_LOG_EVENT("ProfileListChanged", "Re-Requesting Network Properties");
   for (ManagedStateList::iterator iter = network_list_.begin();
        iter != network_list_.end(); ++iter) {
     NetworkState* network = (*iter)->AsNetworkState();
     DCHECK(network);
     shill_property_handler_->RequestProperties(
         ManagedState::MANAGED_TYPE_NETWORK, network->path());
   }
 }
@@ skipping 32 matching lines @@
     const base::DictionaryValue& properties) {
   DCHECK(network);
   bool network_property_updated = false;
   std::string prev_connection_state = network->connection_state();
   for (base::DictionaryValue::Iterator iter(properties);
        !iter.IsAtEnd(); iter.Advance()) {
     if (network->PropertyChanged(iter.key(), iter.value()))
       network_property_updated = true;
   }
   network_property_updated |= network->InitialPropertiesReceived(properties);
+  UpdateGuid(network);
+  network_list_sorted_ = false;
+
   // Notify observers of NetworkState changes.
   if (network_property_updated || network->update_requested()) {
     // Signal connection state changed after all properties have been updated.
     if (ConnectionStateChanged(network, prev_connection_state))
       OnNetworkConnectionStateChanged(network);
     NET_LOG_EVENT("NetworkPropertiesUpdated", GetLogName(network));
     NotifyNetworkPropertiesUpdated(network);
   }
-  UpdateGuid(network);
 }
 
 void NetworkStateHandler::UpdateNetworkServiceProperty(
     const std::string& service_path,
     const std::string& key,
     const base::Value& value) {
   bool changed = false;
   NetworkState* network = GetModifiableNetworkState(service_path);
   if (!network)
     return;
   std::string prev_connection_state = network->connection_state();
   std::string prev_profile_path = network->profile_path();
   changed |= network->PropertyChanged(key, value);
   if (!changed)
     return;
 
-  if (key == shill::kStateProperty) {
+  if (key == shill::kStateProperty || key == shill::kVisibleProperty) {
+    network_list_sorted_ = false;
     if (ConnectionStateChanged(network, prev_connection_state)) {
       OnNetworkConnectionStateChanged(network);
       // If the connection state changes, other properties such as IPConfig
       // may have changed, so request a full update.
       RequestUpdateForNetwork(service_path);
     }
   } else {
     std::string value_str;
     value.GetAsString(&value_str);
     // Some property changes are noisy and not interesting:
@@ skipping 101 matching lines @@
 
 void NetworkStateHandler::TechnologyListChanged() {
   // Eventually we would like to replace Technology state with Device state.
   // For now, treat technology state changes as device list changes.
   NotifyDeviceListChanged();
 }
 
 void NetworkStateHandler::ManagedStateListChanged(
     ManagedState::ManagedType type) {
   if (type == ManagedState::MANAGED_TYPE_NETWORK) {
+    SortNetworkList();
+    UpdateNetworkStats();
     // Notify observers that the list of networks has changed.
     NET_LOG_EVENT("NOTIFY:NetworkListChanged",
                   base::StringPrintf("Size:%" PRIuS, network_list_.size()));
     FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                       NetworkListChanged());
-    // Update UMA stats.
-    size_t shared = 0, unshared = 0, visible = 0;
-    for (ManagedStateList::iterator iter = network_list_.begin();
-         iter != network_list_.end(); ++iter) {
-      NetworkState* network = (*iter)->AsNetworkState();
-      if (network->visible())
-        ++visible;
-      if (network->IsInProfile()) {
-        if (network->IsPrivate())
-          ++unshared;
-        else
-          ++shared;
-      }
-    }
-    UMA_HISTOGRAM_COUNTS_100("Networks.Visible", visible);
-    UMA_HISTOGRAM_COUNTS_100("Networks.RememberedShared", shared);
-    UMA_HISTOGRAM_COUNTS_100("Networks.RememberedUnshared", unshared);
   } else if (type == ManagedState::MANAGED_TYPE_DEVICE) {
     std::string devices;
     for (ManagedStateList::const_iterator iter = device_list_.begin();
          iter != device_list_.end(); ++iter) {
       if (iter != device_list_.begin())
         devices += ", ";
       devices += (*iter)->name();
     }
     NET_LOG_EVENT("DeviceList", devices);
     NotifyDeviceListChanged();
   } else {
     NOTREACHED();
   }
 }
 
+void NetworkStateHandler::SortNetworkList() {
+  bool listing_active_networks = true;
+  ManagedStateList active, non_wifi_visible, wifi_visible, hidden;
+  for (ManagedStateList::iterator iter = network_list_.begin();
+       iter != network_list_.end(); ++iter) {
+    NetworkState* network = (*iter)->AsNetworkState();
+    if (network->IsConnectedState() || network->IsConnectingState()) {
+      active.push_back(network);
+      if (!listing_active_networks) {
+        NET_LOG_ERROR("Active network follows inactive network",
+                      GetLogName(network));
+      }
+    } else {
+      listing_active_networks = false;
+      if (network->visible()) {
+        if (NetworkTypePattern::WiFi().MatchesType(network->type()))
+          wifi_visible.push_back(network);
+        else
+          non_wifi_visible.push_back(network);
+      } else {
+        hidden.push_back(network);

[pneubeck (no reviews), 2014/06/18 09:45:48]

do networks with !update_received() always end up in these list?
since sort is now called not only after all updates are received, you should
handle this case explicitly (or at least comment it)

[stevenjb, 2014/06/18 22:33:26]

They would be treated as "hidden", which would be fine, but I went ahead and
handled them explicitly for clarity.

+      }
+    }
+  }
+  network_list_.clear();
+  network_list_.insert(network_list_.end(), active.begin(), active.end());
+  network_list_.insert(
+      network_list_.end(), non_wifi_visible.begin(), non_wifi_visible.end());
+  network_list_.insert(
+      network_list_.end(), wifi_visible.begin(), wifi_visible.end());
+  network_list_.insert(network_list_.end(), hidden.begin(), hidden.end());
+  network_list_sorted_ = true;
+}
+
+void NetworkStateHandler::UpdateNetworkStats() {
+  size_t shared = 0, unshared = 0, visible = 0;
+  for (ManagedStateList::iterator iter = network_list_.begin();
+       iter != network_list_.end(); ++iter) {
+    NetworkState* network = (*iter)->AsNetworkState();
+    if (network->visible())
+      ++visible;
+    if (network->IsInProfile()) {
+      if (network->IsPrivate())
+        ++unshared;
+      else
+        ++shared;
+    }
+  }
+  UMA_HISTOGRAM_COUNTS_100("Networks.Visible", visible);
+  UMA_HISTOGRAM_COUNTS_100("Networks.RememberedShared", shared);
+  UMA_HISTOGRAM_COUNTS_100("Networks.RememberedUnshared", unshared);
+}
+
 void NetworkStateHandler::DefaultNetworkServiceChanged(
     const std::string& service_path) {
   // Shill uses '/' for empty service path values; check explicitly for that.
   const char* kEmptyServicePath = "/";
   std::string new_service_path =
       (service_path != kEmptyServicePath) ? service_path : "";
   if (new_service_path == default_network_path_)
     return;
 
   default_network_path_ = service_path;
@@ skipping 180 matching lines @@
   if (type.MatchesType(shill::kTypeBluetooth))
     technologies.push_back(new std::string(shill::kTypeBluetooth));
   if (type.MatchesType(shill::kTypeVPN))
     technologies.push_back(new std::string(shill::kTypeVPN));
 
   CHECK_GT(technologies.size(), 0ul);
   return technologies.Pass();
 }
 
 }  // namespace chromeos