Rely on Service.Visible instead of Manager.Services

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 aa91883ab1f970acc6a663a61eb14ce3c50e329d..1d1e6878a7597ad85e45c493b02ac66d231de631 100644
--- a/chromeos/network/network_state_handler.cc
+++ b/chromeos/network/network_state_handler.cc
@@ -222,7 +222,7 @@ const NetworkState* NetworkStateHandler::FirstNetworkByType(
     if (!network->update_received())
       continue;
     if (!network->visible())
-      continue;
+      break;
     if (network->Matches(type))
       return network;
   }
@@ -461,34 +461,6 @@ void NetworkStateHandler::UpdateManagedList(ManagedState::ManagedType type,
   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();
@@ -541,6 +513,13 @@ void NetworkStateHandler::UpdateNetworkStateProperties(
       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.
@@ -549,7 +528,6 @@ void NetworkStateHandler::UpdateNetworkStateProperties(
     NET_LOG_EVENT("NetworkPropertiesUpdated", GetLogName(network));
     NotifyNetworkPropertiesUpdated(network);
   }
-  UpdateGuid(network);
 }
 
 void NetworkStateHandler::UpdateNetworkServiceProperty(
@@ -566,7 +544,7 @@ void NetworkStateHandler::UpdateNetworkServiceProperty(
   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
@@ -680,28 +658,13 @@ void NetworkStateHandler::TechnologyListChanged() {
 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();
@@ -717,6 +680,65 @@ void NetworkStateHandler::ManagedStateListChanged(
   }
 }
 
+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.