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 204 matching lines @@
 
 const NetworkState* NetworkStateHandler::FirstNetworkByType(
     const NetworkTypePattern& type) const {
   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);
@@ skipping 218 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);
+
+  // Ensure that sorting is correct. Observers will be notified that the list
+  // changed one time only when ManagedStateListChanged() is called (after
+  // all requested updates have been received).
+  SortNetworkList();

[pneubeck (no reviews), 2014/06/17 15:30:33]

Couldn't this lead to a to huge performance impact?
This will be called on every network update. E.g. if the user logs in, we call
this up to a 100 times. Similarly for other triggers like after a network scan
(if several networks disappeared and reappear; doesn't shill regularly forget
wifi services, on purpose?).

Every of these calls, takes linear time in the length of the list (the list can
be longer than 100; but should be safe to assume <1000 in the typical case). And
that on the UI thread.



I have the strong impression, that this can't be solved with a simple solution
"correctly" if we don't have an idea on the frequency of function calls.


Can we chat/VC about this later?

[stevenjb, 2014/06/17 17:14:08]

Yeah, you're right, technically after a scan this could be an N^2 operation.
Even though N is likely to be small in practice, it could lead to noticeable lag
and should be avoided. This is why I didn't do this initially, I'm not sure how
I talked myself into doing it, except that I was distracted by other things
yesterday.

We could sort on demand. I'm not a huge fan of that either, mostly because it
adds complexity from the calling side of things, but if we use a dirty flag we
can at least ensure that sorting doesn't happen often in that case.

Or we could go back to only sorting when all updates have been received and fix
FirstNetworkByType to scan the entire list. In practice there shouldn't be any
visible artifacts so long as the UI only refreshes the list in response to a
NetworkListChanged event.

Feel free to ping me when you're available if you want to chat about this over
VC.

+
   // 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) {
     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 93 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() {
+  // Sort networks as follows, maintaining the existing relative ordering:
+  // * Connected or connecting networks (should be listed first by Shill)
+  // * Visible non-wifi networks
+  // * Visible wifi networks
+  // * Hidden (wifi) networks
+  // Also update UMA stats.
+  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);
+      }
+    }
+  }
+  // Update |network_list_|.
+  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());
+}
+
+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