Merge FavoriteState into NetworkState

chromeos/network/shill_property_handler.cc

Index: chromeos/network/shill_property_handler.cc
diff --git a/chromeos/network/shill_property_handler.cc b/chromeos/network/shill_property_handler.cc
index c833106ce02759cf989d786fb670478130304af1..9bf9d6cd5db3a0a954466966eb705bd10fc95173 100644
--- a/chromeos/network/shill_property_handler.cc
+++ b/chromeos/network/shill_property_handler.cc
@@ -104,7 +104,8 @@ class ShillPropertyObserver : public ShillPropertyChangedObserver {
 
 ShillPropertyHandler::ShillPropertyHandler(Listener* listener)
     : listener_(listener),
-      shill_manager_(DBusThreadManager::Get()->GetShillManagerClient()) {
+      shill_manager_(DBusThreadManager::Get()->GetShillManagerClient()),
+      new_observed_networks_(0) {
 }
 
 ShillPropertyHandler::~ShillPropertyHandler() {
@@ -124,6 +125,7 @@ void ShillPropertyHandler::Init() {
 
 void ShillPropertyHandler::UpdateManagerProperties() {
   NET_LOG_EVENT("UpdateManagerProperties", "");
+  new_observed_networks_ = 0;
   shill_manager_->GetProperties(
       base::Bind(&ShillPropertyHandler::ManagerPropertiesCallback,
                  AsWeakPtr()));
@@ -211,8 +213,7 @@ void ShillPropertyHandler::RequestProperties(ManagedState::ManagedType type,
   NET_LOG_DEBUG("Request Properties: " + ManagedState::TypeToString(type),
                 path);
   pending_updates_[type].insert(path);
-  if (type == ManagedState::MANAGED_TYPE_NETWORK ||
-      type == ManagedState::MANAGED_TYPE_FAVORITE) {
+  if (type == ManagedState::MANAGED_TYPE_NETWORK) {
     DBusThreadManager::Get()->GetShillServiceClient()->GetProperties(
         dbus::ObjectPath(path),
         base::Bind(&ShillPropertyHandler::GetPropertiesCallback,
@@ -257,15 +258,14 @@ void ShillPropertyHandler::ManagerPropertiesCallback(
     else
       ManagerPropertyChanged(iter.key(), iter.value());
   }
-  // Update Services which can safely assume other properties have been set.
-  if (update_service_value)
-    ManagerPropertyChanged(shill::kServicesProperty, *update_service_value);
-  // Update ServiceCompleteList which skips entries that have already been
-  // requested for Services.
+  // Update Service lists after other Manager properties. Update
+  // ServiceCompleteList first so that Services (visible) entries already exist.
   if (update_service_complete_value) {
     ManagerPropertyChanged(shill::kServiceCompleteListProperty,
                            *update_service_complete_value);
   }
+  if (update_service_value)
+    ManagerPropertyChanged(shill::kServicesProperty, *update_service_value);
 
   CheckPendingStateListUpdates("");
 }
@@ -273,17 +273,10 @@ void ShillPropertyHandler::ManagerPropertiesCallback(
 void ShillPropertyHandler::CheckPendingStateListUpdates(
     const std::string& key) {
   // Once there are no pending updates, signal the state list changed callbacks.
-  if ((key.empty() || key == shill::kServicesProperty) &&
+  if ((key.empty() || key == shill::kServiceCompleteListProperty) &&

[pneubeck (no reviews), 2014/06/10 10:51:37]

I'm very uncertain about this working. It seems to rely on the order in which
Shill sends the kServiceCompleteListProperty and kServices update. If that's the
case, IMO that's too fragile.

Do we have a test checking the following?
A service X is not visible but 'configured' (i.e. X is in ServiceCompleteList
but not in kServicesProperty).
Observer sees it as not visible.
Service X becomes visible, i.e. X is now also in kServicesProperty.
Ideally, we would test this for both update orders (Services vs.
ServicesCompleteList).

From reading the code, I'm a bit unsure whether the updating of the visible
property and sending the notification is always in the correct order (the
notification in question is send in line 234).

[stevenjb, 2014/06/10 16:17:28]

Yes it relies on the order. Yes it's fragile, but it's going away.
I'll add a comment.

[stevenjb, 2014/06/10 23:42:27]

Actually, this part doesn't rely on order, it relies on all updates being
received, which will always occur after both lists have been updated. The call
to UpdateVisibleNetworks() does rely on the order, so I added a comment and TODO
there.

[pneubeck (no reviews), 2014/06/11 12:44:55]

Again, I think you describe only the case where any updates were requested.

[stevenjb, 2014/06/11 23:31:40]

Yes, addressed now.

       pending_updates_[ManagedState::MANAGED_TYPE_NETWORK].size() == 0) {
     listener_->ManagedStateListChanged(ManagedState::MANAGED_TYPE_NETWORK);
   }
-  // Both Network update requests and Favorite update requests will affect
-  // the list of favorites, so wait for both to complete.
-  if ((key.empty() || key == shill::kServiceCompleteListProperty) &&
-      pending_updates_[ManagedState::MANAGED_TYPE_NETWORK].size() == 0 &&
-      pending_updates_[ManagedState::MANAGED_TYPE_FAVORITE].size() == 0) {
-    listener_->ManagedStateListChanged(ManagedState::MANAGED_TYPE_FAVORITE);
-  }
   if ((key.empty() || key == shill::kDevicesProperty) &&
       pending_updates_[ManagedState::MANAGED_TYPE_DEVICE].size() == 0) {
     listener_->ManagedStateListChanged(ManagedState::MANAGED_TYPE_DEVICE);
@@ -300,8 +293,8 @@ void ShillPropertyHandler::ManagerPropertyChanged(const std::string& key,
   } else if (key == shill::kServicesProperty) {
     const base::ListValue* vlist = GetListValue(key, value);
     if (vlist) {
-      listener_->UpdateManagedList(ManagedState::MANAGED_TYPE_NETWORK, *vlist);
-      UpdateProperties(ManagedState::MANAGED_TYPE_NETWORK, *vlist);
+      listener_->UpdateVisibleNetworks(*vlist);

[pneubeck (no reviews), 2014/06/10 10:51:37]

Note, that Shill is emitting a 'Visible' property for each service since a while
(https://chromium-review.googlesource.com/202305, and 
https://chromium-review.googlesource.com/202136).

That property could be parsed by NetworkState in the same way as other
properties. Maybe simpler than relying on the Services list.

This would also make my previous comment moot.

[stevenjb, 2014/06/10 16:17:28]

That is the intention, but this CL has already taken two weeks to complete and I
really need a reasonably stable intermediate point. I've tested this pretty
carefully, and the update order in manager.cc is pretty clear, so would prefer
to move forward with this (commented).
https://chromium-review.googlesource.com/#/c/202223/2/manager.cc

[stevenjb, 2014/06/10 23:42:27]

Also, to be clear, I've tested this pretty thoroughly and am highly confident
that it will work. I am slightly less confident of the robustness of Shill
emitting the Visible property changes (although I don't expect there to be any
problems). Thus, I would rather commit this as-is, then follow up with the
Visible property change after, so that we can revert just that change if it
turns out not to be reliable.

[pneubeck (no reviews), 2014/06/11 12:44:55]

Assuming that Shill does emit the signals in the required order, this
intermediate step sounds good to me.

You might want to put a second thought into whether pure visibility changes also
currently work (where no network state updates were requested).

[stevenjb, 2014/06/11 23:31:40]

Addressed.

+
       // UpdateObserved used to use kServiceWatchListProperty for TYPE_NETWORK,
       // however that prevents us from receiving Strength updates from inactive
       // networks. The overhead for observing all services is not unreasonable
@@ -311,9 +304,11 @@ void ShillPropertyHandler::ManagerPropertyChanged(const std::string& key,
   } else if (key == shill::kServiceCompleteListProperty) {
     const base::ListValue* vlist = GetListValue(key, value);
     if (vlist) {
-      listener_->UpdateManagedList(ManagedState::MANAGED_TYPE_FAVORITE, *vlist);
-      UpdateProperties(ManagedState::MANAGED_TYPE_FAVORITE, *vlist);
+      listener_->UpdateManagedList(ManagedState::MANAGED_TYPE_NETWORK, *vlist);
+      UpdateProperties(ManagedState::MANAGED_TYPE_NETWORK, *vlist);
     }
+  } else if (key == shill::kServiceWatchListProperty) {
+    // Currently we ignore the watch list.
   } else if (key == shill::kDevicesProperty) {
     const base::ListValue* vlist = GetListValue(key, value);
     if (vlist) {
@@ -347,8 +342,6 @@ void ShillPropertyHandler::ManagerPropertyChanged(const std::string& key,
 void ShillPropertyHandler::UpdateProperties(ManagedState::ManagedType type,
                                             const base::ListValue& entries) {
   std::set<std::string>& requested_updates = requested_updates_[type];
-  std::set<std::string>& requested_service_updates =
-      requested_updates_[ManagedState::MANAGED_TYPE_NETWORK];  // For favorites
   std::set<std::string> new_requested_updates;
   NET_LOG_DEBUG("UpdateProperties: " + ManagedState::TypeToString(type),
                 base::StringPrintf("%" PRIuS, entries.GetSize()));
@@ -358,22 +351,14 @@ void ShillPropertyHandler::UpdateProperties(ManagedState::ManagedType type,
     (*iter)->GetAsString(&path);
     if (path.empty())
       continue;
-    // Only request properties once. Favorites that are visible will be updated
-    // when the Network entry is updated. Since 'Services' is always processed
-    // before ServiceCompleteList, only Favorites that are not visible will be
-    // requested here, and GetPropertiesCallback() will only get called with
-    // type == FAVORITE for non-visible Favorites.
-    if (type == ManagedState::MANAGED_TYPE_FAVORITE &&
-        requested_service_updates.count(path) > 0) {
-      continue;
-    }
 
     // We add a special case for devices here to work around an issue in shill
     // that prevents it from sending property changed signals for cellular
     // devices (see crbug.com/321854).
     if (type == ManagedState::MANAGED_TYPE_DEVICE ||
-        requested_updates.find(path) == requested_updates.end())
+        requested_updates.find(path) == requested_updates.end()) {
       RequestProperties(type, path);
+    }
     new_requested_updates.insert(path);
   }
   requested_updates.swap(new_requested_updates);
@@ -381,8 +366,6 @@ void ShillPropertyHandler::UpdateProperties(ManagedState::ManagedType type,
 
 void ShillPropertyHandler::UpdateObserved(ManagedState::ManagedType type,
                                           const base::ListValue& entries) {
-  DCHECK(type == ManagedState::MANAGED_TYPE_NETWORK ||
-         type == ManagedState::MANAGED_TYPE_DEVICE);
   ShillPropertyObserverMap& observer_map =
       (type == ManagedState::MANAGED_TYPE_NETWORK)
       ? observed_networks_ : observed_devices_;
@@ -401,6 +384,8 @@ void ShillPropertyHandler::UpdateObserved(ManagedState::ManagedType type,
       new_observed[path] = new ShillPropertyObserver(
           type, path, base::Bind(
               &ShillPropertyHandler::PropertyChangedCallback, AsWeakPtr()));
+      if (type == ManagedState::MANAGED_TYPE_NETWORK)
+        ++new_observed_networks_;
     }
     observer_map.erase(path);
     // Limit the number of observed services.
@@ -491,14 +476,6 @@ void ShillPropertyHandler::GetPropertiesCallback(
                   base::StringPrintf("%s: %d", path.c_str(), call_status));
     return;
   }
-  // Update Favorite properties for networks in the Services list. Call this
-  // for all networks, regardless of whether or not Profile is set, because
-  // we track all networks in the Favorites list (even if they aren't saved
-  // in a Profile). See notes in UpdateProperties() and favorite_state.h.
-  if (type == ManagedState::MANAGED_TYPE_NETWORK) {
-    listener_->UpdateManagedStateProperties(
-        ManagedState::MANAGED_TYPE_FAVORITE, path, properties);
-  }
   listener_->UpdateManagedStateProperties(type, path, properties);
 
   if (type == ManagedState::MANAGED_TYPE_NETWORK) {
@@ -514,15 +491,8 @@ void ShillPropertyHandler::GetPropertiesCallback(
   }
 
   // Notify the listener only when all updates for that type have completed.
-  if (pending_updates_[type].size() == 0) {
+  if (pending_updates_[type].size() == 0)
     listener_->ManagedStateListChanged(type);
-    // Notify that Favorites have changed when notifying for Networks if there
-    // are no additional Favorite updates pending.
-    if (type == ManagedState::MANAGED_TYPE_NETWORK &&
-        pending_updates_[ManagedState::MANAGED_TYPE_FAVORITE].size() == 0) {
-      listener_->ManagedStateListChanged(ManagedState::MANAGED_TYPE_FAVORITE);
-    }
-  }
 }
 
 void ShillPropertyHandler::PropertyChangedCallback(