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..c7c82ebfd652adf710e9220c15c405c83147622e 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;

[pneubeck (no reviews), 2014/06/16 14:38:15]

did the ordering by Shill change in the meanwhile or is this a rebase artifact?
If it was never related to the ordering but to pending updates, this should
indeed be fine.

[stevenjb, 2014/06/16 19:54:49]

This was an ordering issue that is now addressed in SortNetworkList().

     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();
@@ -566,7 +538,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 +652,12 @@ void NetworkStateHandler::TechnologyListChanged() {
 void NetworkStateHandler::ManagedStateListChanged(
     ManagedState::ManagedType type) {
   if (type == ManagedState::MANAGED_TYPE_NETWORK) {
+    UpdateNetworkList();

[pneubeck (no reviews), 2014/06/16 14:38:15]

Just noting that networks are added at distributed over several loop iterations.
After these several iterations this is finally called.
In between there can already be access to the network list which wouldn't see
the sorting.

If we start sorting at all, it should be consistent over all accesses?
E.g. on each list access, we could ensure that the list is sorted / return it
sorted.
The list accesses have linear time anyways.

[stevenjb, 2014/06/16 19:54:49]

So, I thought about this a bunch, and while in practice I think this is OK, I
agree that it is imperfect and as-is FirstNetwork would at least have to be
changed back.

From a maintainability perspective having an imperfectly sorted list is
problematic though, so I think that the correct thing to do is to sort the list
every time a NetworkState update is received.

(The easiest thing to do would be to fix the sorting in Shill, but I don't like
to impose a sorting there that we mostly use for UI convenience).

     // 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 +673,62 @@ void NetworkStateHandler::ManagedStateListChanged(
   }
 }
 
+void NetworkStateHandler::UpdateNetworkList() {
+  // Sort networks as follows:

[pneubeck (no reviews), 2014/06/16 14:38:15]

Better also mention that it's a stable sort that maintains the order of Shill's
list.

[stevenjb, 2014/06/16 19:54:49]

Done.

+  // * 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;
+  size_t shared = 0, unshared = 0;
+  ManagedStateList active, wifi_visible, wifi_hidden;

[pneubeck (no reviews), 2014/06/16 14:38:15]

is there an expected case, where this reordering actually changes order?

I thought at first that it's needed because of the visible property. But the
moment this is currently called, the visible property should already be
received.

[stevenjb, 2014/06/16 19:54:49]

Shill is sorting by type then by state, type, visibility, so hidden wifi
networks show up before disconnected (but "visible") Cellular networks.

+  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())) {
+          // List visible non-wifi networks immediately after active networks.

[pneubeck (no reviews), 2014/06/16 14:38:15]

is this the difference to Shill's order?
Note that this relies on Shill still listing the active ones before the
inactive, visible-non-wifi networks.
If you already using several target lists, could you add one more to remove that
dependency? (e.g. ManagedStateList visible_non_wifi)

(you could then remove the negation form the if-condition)

[stevenjb, 2014/06/16 19:54:49]

Done.

+          active.push_back(network);
+        } else {
+          wifi_visible.push_back(network);
+        }
+      } else {
+        wifi_hidden.push_back(network);
+      }
+    }
+    if (network->IsInProfile()) {
+      if (network->IsPrivate())
+        ++unshared;
+      else
+        ++shared;
+    }
+  }
+
+  // Update |network_list_|.
+  network_list_.clear();
+  network_list_.insert(
+      network_list_.end(), active.begin(), active.end());
+  network_list_.insert(
+      network_list_.end(), wifi_visible.begin(), wifi_visible.end());
+  network_list_.insert(
+      network_list_.end(), wifi_hidden.begin(), wifi_hidden.end());
+
+  // Update histograms.
+  UMA_HISTOGRAM_COUNTS_100("Networks.Visible",
+                           active.size() + wifi_visible.size());

[pneubeck (no reviews), 2014/06/16 14:38:15]

optional nit: use a visible counter

[stevenjb, 2014/06/16 19:54:49]

Done.

+  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.