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;

[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;
   }
   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 57 matching lines @@
   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) {
+    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();
          iter != device_list_.end(); ++iter) {
       if (iter != device_list_.begin())
         devices += ", ";
       devices += (*iter)->name();
     }
     NET_LOG_EVENT("DeviceList", devices);
     NotifyDeviceListChanged();
   } else {
     NOTREACHED();
   }
 }
 
+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.
   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