Eliminate NetworkManagerChanged

chromeos/network/shill_property_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/shill_property_handler.h"
 
 #include "base/bind.h"
 #include "base/format_macros.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
@@ skipping 206 matching lines @@
         dbus::ObjectPath(path),
         base::Bind(&ShillPropertyHandler::GetPropertiesCallback,
                    AsWeakPtr(), type, path));
   } else {
     NOTREACHED();
   }
 }
 
 void ShillPropertyHandler::OnPropertyChanged(const std::string& key,
                                              const base::Value& value) {
-  if (ManagerPropertyChanged(key, value)) {
-    std::string detail = key;
-    detail += " = " + network_event_log::ValueAsString(value);
-    NET_LOG_DEBUG("ManagerPropertyChanged", detail);
-    listener_->NotifyManagerPropertyChanged();
-  }
+  ManagerPropertyChanged(key, value);
   CheckPendingStateListUpdates(key);
 }
 
 //------------------------------------------------------------------------------
 // Private methods
 
 void ShillPropertyHandler::ManagerPropertiesCallback(
     DBusMethodCallStatus call_status,
     const base::DictionaryValue& properties) {
   if (call_status != DBUS_METHOD_CALL_SUCCESS) {
     NET_LOG_ERROR("ManagerPropertiesCallback",
                   base::StringPrintf("Failed: %d", call_status));
     return;
   }
   NET_LOG_EVENT("ManagerPropertiesCallback", "Success");
-  bool notify = false;
   const base::Value* update_service_value = NULL;
   const base::Value* update_service_complete_value = NULL;
   for (base::DictionaryValue::Iterator iter(properties);
        !iter.IsAtEnd(); iter.Advance()) {
     // Defer updating Services until all other properties have been updated.
     if (iter.key() == flimflam::kServicesProperty)
       update_service_value = &iter.value();
     else if (iter.key() == shill::kServiceCompleteListProperty)
       update_service_complete_value = &iter.value();
     else
-      notify |= ManagerPropertyChanged(iter.key(), iter.value());
+      ManagerPropertyChanged(iter.key(), iter.value());
   }
   // Update Services which can safely assume other properties have been set.
-  if (update_service_value) {
-    notify |= ManagerPropertyChanged(flimflam::kServicesProperty,
-                                     *update_service_value);
-  }
+  if (update_service_value)
+    ManagerPropertyChanged(flimflam::kServicesProperty, *update_service_value);
   // Update ServiceCompleteList which skips entries that have already been
   // requested for Services.
   if (update_service_complete_value) {
-    notify |= ManagerPropertyChanged(shill::kServiceCompleteListProperty,
-                                     *update_service_complete_value);
+    ManagerPropertyChanged(shill::kServiceCompleteListProperty,
+                           *update_service_complete_value);
   }
 
-  if (notify)
-    listener_->NotifyManagerPropertyChanged();
   CheckPendingStateListUpdates("");
 }
 
 void ShillPropertyHandler::CheckPendingStateListUpdates(
     const std::string& key) {
   // Once there are no pending updates, signal the state list changed callbacks.
   if ((key.empty() || key == flimflam::kServicesProperty) &&
       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 == flimflam::kDevicesProperty) &&
       pending_updates_[ManagedState::MANAGED_TYPE_DEVICE].size() == 0) {
     listener_->ManagedStateListChanged(ManagedState::MANAGED_TYPE_DEVICE);
   }
 }
 
-bool ShillPropertyHandler::ManagerPropertyChanged(const std::string& key,
+void ShillPropertyHandler::ManagerPropertyChanged(const std::string& key,
                                                   const base::Value& value) {
-  bool notify_manager_changed = false;
   if (key == flimflam::kServicesProperty) {
     const base::ListValue* vlist = GetListValue(key, value);
     if (vlist) {
       listener_->UpdateManagedList(ManagedState::MANAGED_TYPE_NETWORK, *vlist);
       UpdateProperties(ManagedState::MANAGED_TYPE_NETWORK, *vlist);
       // 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
       // (and we limit the max number of observed services to kMaxObserved).
       UpdateObserved(ManagedState::MANAGED_TYPE_NETWORK, *vlist);
     }
   } else if (key == shill::kServiceCompleteListProperty) {
     const ListValue* vlist = GetListValue(key, value);
     if (vlist) {
       listener_->UpdateManagedList(ManagedState::MANAGED_TYPE_FAVORITE, *vlist);
       UpdateProperties(ManagedState::MANAGED_TYPE_FAVORITE, *vlist);
     }
   } else if (key == flimflam::kDevicesProperty) {
     const base::ListValue* vlist = GetListValue(key, value);
     if (vlist) {
       listener_->UpdateManagedList(ManagedState::MANAGED_TYPE_DEVICE, *vlist);
       UpdateProperties(ManagedState::MANAGED_TYPE_DEVICE, *vlist);
       UpdateObserved(ManagedState::MANAGED_TYPE_DEVICE, *vlist);
     }
   } else if (key == flimflam::kAvailableTechnologiesProperty) {
     const base::ListValue* vlist = GetListValue(key, value);
-    if (vlist) {
+    if (vlist)
       UpdateAvailableTechnologies(*vlist);
-      notify_manager_changed = true;
-    }
   } else if (key == flimflam::kEnabledTechnologiesProperty) {
     const base::ListValue* vlist = GetListValue(key, value);
-    if (vlist) {
+    if (vlist)
       UpdateEnabledTechnologies(*vlist);
-      notify_manager_changed = true;
-    }
   } else if (key == shill::kUninitializedTechnologiesProperty) {
     const base::ListValue* vlist = GetListValue(key, value);
-    if (vlist) {
+    if (vlist)
       UpdateUninitializedTechnologies(*vlist);
-      notify_manager_changed = true;
-    }
   } else if (key == flimflam::kProfilesProperty) {
     listener_->ProfileListChanged();
   } else if (key == flimflam::kCheckPortalListProperty) {
     std::string check_portal_list;
-    if (value.GetAsString(&check_portal_list)) {
+    if (value.GetAsString(&check_portal_list))
       listener_->CheckPortalListChanged(check_portal_list);
-      notify_manager_changed = true;
-    }
   } else {
     VLOG(2) << "Ignored Manager Property: " << key;
   }
-  return notify_manager_changed;
 }
 
 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;
   VLOG(2) << "Update Properties: " << type << " Entries: " << entries.GetSize();
   for (base::ListValue::const_iterator iter = entries.begin();
@@ skipping 51 matching lines @@
 void ShillPropertyHandler::UpdateAvailableTechnologies(
     const base::ListValue& technologies) {
   available_technologies_.clear();
   NET_LOG_EVENT("AvailableTechnologiesChanged",
                 base::StringPrintf("Size: %" PRIuS, technologies.GetSize()));
   for (base::ListValue::const_iterator iter = technologies.begin();
        iter != technologies.end(); ++iter) {
     std::string technology;
     (*iter)->GetAsString(&technology);
     DCHECK(!technology.empty());
     available_technologies_.insert(technology);

[pneubeck (no reviews), 2013/09/11 20:13:35]

how about checking the result value of each insert and only triggering a
notification if really something changed?

similarly for the other Update* functions

[stevenjb, 2013/09/11 21:23:46]

We clear the lists then insert the technologies. We'd have to save the original
list and compare + swap the results. I'm not convinced that's worth the added
complexity. The only time that would save a Callback is when we request a full
Manager properties update, which should only happen when we add or remove a new
network.

   }
+  listener_->TechnologyListChanged();
 }
 
 void ShillPropertyHandler::UpdateEnabledTechnologies(
     const base::ListValue& technologies) {
   enabled_technologies_.clear();
   NET_LOG_EVENT("EnabledTechnologiesChanged",
                 base::StringPrintf("Size: %" PRIuS, technologies.GetSize()));
   for (base::ListValue::const_iterator iter = technologies.begin();
        iter != technologies.end(); ++iter) {
     std::string technology;
     (*iter)->GetAsString(&technology);
     DCHECK(!technology.empty());
     enabled_technologies_.insert(technology);
     enabling_technologies_.erase(technology);
   }
+  listener_->TechnologyListChanged();
 }
 
 void ShillPropertyHandler::UpdateUninitializedTechnologies(
     const base::ListValue& technologies) {
   uninitialized_technologies_.clear();
   NET_LOG_EVENT("UninitializedTechnologiesChanged",
                 base::StringPrintf("Size: %" PRIuS, technologies.GetSize()));
   for (base::ListValue::const_iterator iter = technologies.begin();
        iter != technologies.end(); ++iter) {
     std::string technology;
     (*iter)->GetAsString(&technology);
     DCHECK(!technology.empty());
     uninitialized_technologies_.insert(technology);
   }
+  listener_->TechnologyListChanged();
 }
 
 void ShillPropertyHandler::EnableTechnologyFailed(
     const std::string& technology,
     const network_handler::ErrorCallback& error_callback,
     const std::string& dbus_error_name,
     const std::string& dbus_error_message) {
   enabling_technologies_.erase(technology);
   network_handler::ShillErrorCallbackFunction(
       "EnableTechnology Failed",
@@ skipping 119 matching lines @@
 
 void ShillPropertyHandler::NetworkDevicePropertyChangedCallback(
     const std::string& path,
     const std::string& key,
     const base::Value& value) {
   listener_->UpdateDeviceProperty(path, key, value);
 }
 
 }  // namespace internal
 }  // namespace chromeos