This converts the Shill clients to allow propagation of shill errors

chrome/browser/chromeos/cros/cros_network_functions.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 "chrome/browser/chromeos/cros/cros_network_functions.h"
 
 #include "base/bind.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/string_tokenizer.h"
 #include "base/stringprintf.h"
 #include "base/values.h"
 #include "chrome/browser/chromeos/cros/sms_watcher.h"
 #include "chromeos/dbus/cashew_client.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/shill_device_client.h"
 #include "chromeos/dbus/shill_ipconfig_client.h"
 #include "chromeos/dbus/shill_manager_client.h"
 #include "chromeos/dbus/shill_network_client.h"
 #include "chromeos/dbus/shill_profile_client.h"
 #include "chromeos/dbus/shill_service_client.h"
 #include "dbus/object_path.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 namespace chromeos {
 
 namespace {
 
 // Class to watch network manager's properties without Libcros.
-class NetworkManagerPropertiesWatcher : public CrosNetworkWatcher {
+class NetworkManagerPropertiesWatcher
+    : public CrosNetworkWatcher,
+      public ShillManagerClient::PropertyChangedObserver {
  public:
   NetworkManagerPropertiesWatcher(
-      const NetworkPropertiesWatcherCallback& callback) {
+      const NetworkPropertiesWatcherCallback& callback)
+    : callback_(callback) {
     DBusThreadManager::Get()->GetShillManagerClient()->
-        SetPropertyChangedHandler(base::Bind(callback,
-                                             flimflam::kFlimflamServicePath));
+        AddPropertyChangedObserver(this);
   }
+
   virtual ~NetworkManagerPropertiesWatcher() {
     DBusThreadManager::Get()->GetShillManagerClient()->
-        ResetPropertyChangedHandler();
+        RemovePropertyChangedObserver(this);
   }
+
+  virtual void OnPropertyChanged(const std::string& name,
+                                 const base::Value& value) {
+    callback_.Run(flimflam::kFlimflamServicePath, name, value);
+  }
+ private:
+  NetworkPropertiesWatcherCallback callback_;
 };
 
 // Class to watch network service's properties without Libcros.
-class NetworkServicePropertiesWatcher : public CrosNetworkWatcher {
+class NetworkServicePropertiesWatcher
+    : public CrosNetworkWatcher,
+      ShillServiceClient::PropertyChangedObserver

[hashimoto, 2012/09/21 11:52:01]

Add public, no new line before '{'

[Greg Spencer (Chromium), 2012/09/21 22:03:47]

Done.

+{
  public:
   NetworkServicePropertiesWatcher(
       const NetworkPropertiesWatcherCallback& callback,
-      const std::string& service_path) : service_path_(service_path) {
+      const std::string& service_path) : service_path_(service_path),
+                                         callback_(callback) {
     DBusThreadManager::Get()->GetShillServiceClient()->
-        SetPropertyChangedHandler(dbus::ObjectPath(service_path),
-                                  base::Bind(callback, service_path));
+        AddPropertyChangedObserver(dbus::ObjectPath(service_path), this);
   }
+
   virtual ~NetworkServicePropertiesWatcher() {
     DBusThreadManager::Get()->GetShillServiceClient()->
-        ResetPropertyChangedHandler(dbus::ObjectPath(service_path_));
+        RemovePropertyChangedObserver(dbus::ObjectPath(service_path_), this);
   }
 
+  virtual void OnPropertyChanged(const std::string& name,
+                                 const base::Value& value) {
+    callback_.Run(service_path_, name, value);
+  }
  private:

[hashimoto, 2012/09/21 11:52:01]

nit: Add new line before 'private'

[Greg Spencer (Chromium), 2012/09/21 22:03:47]

Done.

   std::string service_path_;
+  NetworkPropertiesWatcherCallback callback_;
 };
 
 // Class to watch network device's properties without Libcros.
-class NetworkDevicePropertiesWatcher : public CrosNetworkWatcher {
+class NetworkDevicePropertiesWatcher
+    : public CrosNetworkWatcher,
+      public ShillDeviceClient::PropertyChangedObserver {
  public:
   NetworkDevicePropertiesWatcher(
       const NetworkPropertiesWatcherCallback& callback,
-      const std::string& device_path) : device_path_(device_path) {
+      const std::string& device_path) : device_path_(device_path),
+                                        callback_(callback) {
     DBusThreadManager::Get()->GetShillDeviceClient()->
-        SetPropertyChangedHandler(dbus::ObjectPath(device_path),
-                                  base::Bind(callback, device_path));
+        AddPropertyChangedObserver(dbus::ObjectPath(device_path), this);
   }
+
   virtual ~NetworkDevicePropertiesWatcher() {
     DBusThreadManager::Get()->GetShillDeviceClient()->
-        ResetPropertyChangedHandler(dbus::ObjectPath(device_path_));
+        RemovePropertyChangedObserver(dbus::ObjectPath(device_path_), this);
+  }
+
+  virtual void OnPropertyChanged(const std::string& name,
+                                 const base::Value& value) {
+    callback_.Run(device_path_, name, value);
   }
 
  private:
   std::string device_path_;
+  NetworkPropertiesWatcherCallback callback_;
 };
 
 // Converts a string to a CellularDataPlanType.
 CellularDataPlanType ParseCellularDataPlanType(const std::string& type) {
   if (type == cashew::kCellularDataPlanUnlimited)
     return CELLULAR_DATA_PLAN_UNLIMITED;
   if (type == cashew::kCellularDataPlanMeteredPaid)
     return CELLULAR_DATA_PLAN_METERED_PAID;
   if (type == cashew::kCellularDataPlanMeteredBase)
     return CELLULAR_DATA_PLAN_METERED_BASE;
@@ skipping 84 matching lines @@
                        &plan->data_bytes_used);
       data_plan_vector->push_back(plan);
     }
     callback_.Run(service, data_plan_vector);
   }
 
   DataPlanUpdateWatcherCallback callback_;
 };
 
 // Does nothing. Used as a callback.
-void DoNothing(DBusMethodCallStatus call_status) {}
+void DoNothing() {}

[hashimoto, 2012/09/21 11:52:01]

You can use base::DoNothing() in base/bind_helpers.h

+
+// Does nothing. Used as a callback.
+void DoNothingWithCallStatus(DBusMethodCallStatus call_status) {}
+
+// Ignores errors.
+void IgnoreErrors(const std::string& error_name,
+                  const std::string& error_message) {}
 
 // A callback used to implement CrosRequest*Properties functions.
 void RunCallbackWithDictionaryValue(const NetworkPropertiesCallback& callback,
                                     const std::string& path,
                                     DBusMethodCallStatus call_status,
                                     const base::DictionaryValue& value) {
   callback.Run(path, call_status == DBUS_METHOD_CALL_SUCCESS ? &value : NULL);
 }
 
+// A callback used to implement CrosRequest*Properties functions.
+void RunCallbackWithDictionaryValueNoStatus(
+    const NetworkPropertiesCallback& callback,
+    const std::string& path,
+    const base::DictionaryValue& value) {
+  callback.Run(path, &value);
+}
+
+// A callback used to implement the error callback for CrosRequest*Properties
+// functions.
+void RunCallbackWithDictionaryValueError(
+    const NetworkPropertiesCallback& callback,
+    const std::string& path,
+    const std::string& error_name,
+    const std::string& error_message) {
+  callback.Run(path, NULL);
+}
+

[hashimoto, 2012/09/21 11:52:01]

nit: Remove this blank line.

+
 // Used as a callback for ShillManagerClient::GetService
 void OnGetService(const NetworkPropertiesCallback& callback,
                   DBusMethodCallStatus call_status,
                   const dbus::ObjectPath& service_path) {
   if (call_status == DBUS_METHOD_CALL_SUCCESS) {
     VLOG(1) << "OnGetServiceService: " << service_path.value();
     DBusThreadManager::Get()->GetShillServiceClient()->GetProperties(
         service_path, base::Bind(&RunCallbackWithDictionaryValue,
                                  callback,
                                  service_path.value()));
@@ skipping 106 matching lines @@
   return DBusThreadManager::Get()->GetShillServiceClient()->
       CallActivateCellularModemAndBlock(dbus::ObjectPath(service_path),
                                         carrier);
 }
 
 void CrosSetNetworkServiceProperty(const std::string& service_path,
                                    const std::string& property,
                                    const base::Value& value) {
   DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(
       dbus::ObjectPath(service_path), property, value,
-      base::Bind(&DoNothing));
+      base::Bind(DoNothing),
+      base::Bind(&IgnoreErrors));
 }
 
 void CrosClearNetworkServiceProperty(const std::string& service_path,
                                      const std::string& property) {
   DBusThreadManager::Get()->GetShillServiceClient()->ClearProperty(
-      dbus::ObjectPath(service_path), property, base::Bind(&DoNothing));
+      dbus::ObjectPath(service_path), property, base::Bind(DoNothing),
+        base::Bind(&IgnoreErrors));

[hashimoto, 2012/09/21 11:52:01]

nit: align.

 }
 
 void CrosSetNetworkDeviceProperty(const std::string& device_path,
                                   const std::string& property,
                                   const base::Value& value) {
   DBusThreadManager::Get()->GetShillDeviceClient()->SetProperty(
-      dbus::ObjectPath(device_path), property, value, base::Bind(&DoNothing));
+      dbus::ObjectPath(device_path), property, value, base::Bind(&DoNothing),
+      base::Bind(&IgnoreErrors));
 }
 
 void CrosSetNetworkIPConfigProperty(const std::string& ipconfig_path,
                                     const std::string& property,
                                     const base::Value& value) {
   DBusThreadManager::Get()->GetShillIPConfigClient()->SetProperty(
       dbus::ObjectPath(ipconfig_path), property, value,
-      base::Bind(&DoNothing));
+      base::Bind(&DoNothingWithCallStatus));
 }
 
 void CrosSetNetworkManagerProperty(const std::string& property,
                                    const base::Value& value) {
   DBusThreadManager::Get()->GetShillManagerClient()->SetProperty(
-      property, value, base::Bind(&DoNothing));
+      property, value, base::Bind(&DoNothing),
+      base::Bind(&IgnoreErrors));
 }
 
 void CrosDeleteServiceFromProfile(const std::string& profile_path,
                                   const std::string& service_path) {
   DBusThreadManager::Get()->GetShillProfileClient()->DeleteEntry(
-      dbus::ObjectPath(profile_path), service_path, base::Bind(&DoNothing));
+      dbus::ObjectPath(profile_path),
+      service_path,
+      base::Bind(&DoNothing),
+      base::Bind(&IgnoreErrors));
 }
 
 void CrosRequestCellularDataPlanUpdate(const std::string& modem_service_path) {
   DBusThreadManager::Get()->GetCashewClient()->RequestDataPlansUpdate(
       modem_service_path);
 }
 
 CrosNetworkWatcher* CrosMonitorNetworkManagerProperties(
     const NetworkPropertiesWatcherCallback& callback) {
   return new NetworkManagerPropertiesWatcher(callback);
@@ skipping 53 matching lines @@
   DBusThreadManager::Get()->GetShillDeviceClient()->GetProperties(
       dbus::ObjectPath(device_path),
       base::Bind(&RunCallbackWithDictionaryValue, callback, device_path));
 }
 
 void CrosRequestNetworkProfileProperties(
     const std::string& profile_path,
     const NetworkPropertiesCallback& callback) {
   DBusThreadManager::Get()->GetShillProfileClient()->GetProperties(
       dbus::ObjectPath(profile_path),
-      base::Bind(&RunCallbackWithDictionaryValue, callback, profile_path));
+      base::Bind(&RunCallbackWithDictionaryValueNoStatus,
+                 callback, profile_path),
+      base::Bind(RunCallbackWithDictionaryValueError, callback, profile_path));
 }
 
 void CrosRequestNetworkProfileEntryProperties(
     const std::string& profile_path,
     const std::string& profile_entry_path,
     const NetworkPropertiesCallback& callback) {
   DBusThreadManager::Get()->GetShillProfileClient()->GetEntry(
       dbus::ObjectPath(profile_path),
       profile_entry_path,
-      base::Bind(&RunCallbackWithDictionaryValue,
+      base::Bind(&RunCallbackWithDictionaryValueNoStatus,
+                 callback,
+                 profile_entry_path),
+      base::Bind(RunCallbackWithDictionaryValueError,
                  callback,
                  profile_entry_path));
 }
 
 void CrosRequestHiddenWifiNetworkProperties(
     const std::string& ssid,
     const std::string& security,
     const NetworkPropertiesCallback& callback) {
   base::DictionaryValue properties;
   properties.SetWithoutPathExpansion(
       flimflam::kModeProperty,
       base::Value::CreateStringValue(flimflam::kModeManaged));
   properties.SetWithoutPathExpansion(
       flimflam::kTypeProperty,
       base::Value::CreateStringValue(flimflam::kTypeWifi));
   properties.SetWithoutPathExpansion(
       flimflam::kSSIDProperty,
       base::Value::CreateStringValue(ssid));
   properties.SetWithoutPathExpansion(
       flimflam::kSecurityProperty,
       base::Value::CreateStringValue(security));
   // shill.Manger.GetService() will apply the property changes in
   // |properties| and return a new or existing service to OnGetService().
   // OnGetService will then call GetProperties which will then call callback.
   DBusThreadManager::Get()->GetShillManagerClient()->GetService(
-      properties, base::Bind(&OnGetService, callback));
+      properties, base::Bind(&OnGetService, callback),
+      base::Bind(&IgnoreErrors));
 }
 
 void CrosRequestVirtualNetworkProperties(
     const std::string& service_name,
     const std::string& server_hostname,
     const std::string& provider_type,
     const NetworkPropertiesCallback& callback) {
   base::DictionaryValue properties;
   properties.SetWithoutPathExpansion(
       flimflam::kTypeProperty,
       base::Value::CreateStringValue(flimflam::kTypeVPN));
   properties.SetWithoutPathExpansion(
       flimflam::kProviderNameProperty,
       base::Value::CreateStringValue(service_name));
   properties.SetWithoutPathExpansion(
       flimflam::kProviderHostProperty,
       base::Value::CreateStringValue(server_hostname));
   properties.SetWithoutPathExpansion(
       flimflam::kProviderTypeProperty,
       base::Value::CreateStringValue(provider_type));
   // The actual value of Domain does not matter, so just use service_name.
   properties.SetWithoutPathExpansion(
       flimflam::kVPNDomainProperty,
       base::Value::CreateStringValue(service_name));
 
   // shill.Manger.GetService() will apply the property changes in
   // |properties| and pass a new or existing service to OnGetService().
   // OnGetService will then call GetProperties which will then call callback.
   DBusThreadManager::Get()->GetShillManagerClient()->GetService(
-      properties, base::Bind(&OnGetService, callback));
+      properties, base::Bind(&OnGetService, callback),
+      base::Bind(&IgnoreErrors));
 }
 
 void CrosRequestNetworkServiceDisconnect(const std::string& service_path) {
   DBusThreadManager::Get()->GetShillServiceClient()->Disconnect(
-      dbus::ObjectPath(service_path), base::Bind(&DoNothing));
+      dbus::ObjectPath(service_path), base::Bind(DoNothing),

[hashimoto, 2012/09/21 11:52:01]

nit: Add '&' before DoNothing for consistency.

+      base::Bind(&IgnoreErrors));
 }
 
 void CrosRequestRemoveNetworkService(const std::string& service_path) {
   DBusThreadManager::Get()->GetShillServiceClient()->Remove(
-      dbus::ObjectPath(service_path), base::Bind(&DoNothing));
+      dbus::ObjectPath(service_path), base::Bind(DoNothing),

[hashimoto, 2012/09/21 11:52:01]

ditto.

+      base::Bind(&IgnoreErrors));
 }
 
 void CrosRequestNetworkScan(const std::string& network_type) {
   DBusThreadManager::Get()->GetShillManagerClient()->RequestScan(
-      network_type, base::Bind(&DoNothing));
+      network_type, base::Bind(&DoNothing),
+      base::Bind(&IgnoreErrors));
 }
 
 void CrosRequestNetworkDeviceEnable(const std::string& network_type,
                                     bool enable) {
   if (enable) {
     DBusThreadManager::Get()->GetShillManagerClient()->EnableTechnology(
-        network_type, base::Bind(&DoNothing));
+        network_type, base::Bind(&DoNothing),
+        base::Bind(&IgnoreErrors));
   } else {
     DBusThreadManager::Get()->GetShillManagerClient()->DisableTechnology(
-        network_type, base::Bind(&DoNothing));
+        network_type, base::Bind(&DoNothing),
+        base::Bind(&IgnoreErrors));
   }
 }
 
 void CrosRequestRequirePin(const std::string& device_path,
                            const std::string& pin,
                            bool enable,
                            const NetworkOperationCallback& callback) {
   DBusThreadManager::Get()->GetShillDeviceClient()->RequirePin(
       dbus::ObjectPath(device_path), pin, enable,
       base::Bind(callback, device_path, NETWORK_METHOD_ERROR_NONE,
@@ skipping 28 matching lines @@
                           const NetworkOperationCallback& callback) {
   DBusThreadManager::Get()->GetShillDeviceClient()->ChangePin(
       dbus::ObjectPath(device_path), old_pin, new_pin,
       base::Bind(callback, device_path, NETWORK_METHOD_ERROR_NONE,
                  std::string()),
       base::Bind(&OnNetworkActionError, callback, device_path));
 }
 
 void CrosProposeScan(const std::string& device_path) {
   DBusThreadManager::Get()->GetShillDeviceClient()->ProposeScan(
-      dbus::ObjectPath(device_path), base::Bind(&DoNothing));
+      dbus::ObjectPath(device_path), base::Bind(&DoNothingWithCallStatus));
 }
 
 void CrosRequestCellularRegister(const std::string& device_path,
                                  const std::string& network_id,
                                  const NetworkOperationCallback& callback) {
   DBusThreadManager::Get()->GetShillDeviceClient()->Register(
       dbus::ObjectPath(device_path), network_id,
       base::Bind(callback, device_path, NETWORK_METHOD_ERROR_NONE,
                  std::string()),
       base::Bind(&OnNetworkActionError, callback, device_path));
 }
 
 bool CrosSetOfflineMode(bool offline) {
   base::FundamentalValue value(offline);
   DBusThreadManager::Get()->GetShillManagerClient()->SetProperty(
-      flimflam::kOfflineModeProperty, value, base::Bind(&DoNothing));
+      flimflam::kOfflineModeProperty, value, base::Bind(&DoNothing),
+      base::Bind(&IgnoreErrors));
   return true;
 }
 
 bool CrosListIPConfigs(const std::string& device_path,
                        NetworkIPConfigVector* ipconfig_vector,
                        std::vector<std::string>* ipconfig_paths,
                        std::string* hardware_address) {
   if (hardware_address)
     hardware_address->clear();
   const dbus::ObjectPath device_object_path(device_path);
@@ skipping 67 matching lines @@
 }
 
 bool CrosRemoveIPConfig(const std::string& ipconfig_path) {
   return DBusThreadManager::Get()->GetShillIPConfigClient()->
       CallRemoveAndBlock(dbus::ObjectPath(ipconfig_path));
 }
 
 void CrosRequestIPConfigRefresh(const std::string& ipconfig_path) {
   DBusThreadManager::Get()->GetShillIPConfigClient()->Refresh(
       dbus::ObjectPath(ipconfig_path),
-      base::Bind(&DoNothing));
+      base::Bind(&DoNothingWithCallStatus));
 }
 
 bool CrosGetWifiAccessPoints(WifiAccessPointVector* result) {
   scoped_ptr<base::DictionaryValue> manager_properties(
       DBusThreadManager::Get()->GetShillManagerClient()->
       CallGetPropertiesAndBlock());
   if (!manager_properties.get()) {
     LOG(WARNING) << "Couldn't read managers's properties";
     return false;
   }
@@ skipping 70 matching lines @@
       result->push_back(ap);
     }
   }
   if (!found_at_least_one_device)
     return false;  // No powered device found that has a 'Networks' array.
   return true;
 }
 
 void CrosConfigureService(const base::DictionaryValue& properties) {
   DBusThreadManager::Get()->GetShillManagerClient()->ConfigureService(
-      properties, base::Bind(&DoNothing));
+      properties, base::Bind(&DoNothing),
+      base::Bind(&IgnoreErrors));
 }
 
 std::string CrosPrefixLengthToNetmask(int32 prefix_length) {
   std::string netmask;
   // Return the empty string for invalid inputs.
   if (prefix_length < 0 || prefix_length > 32)
     return netmask;
   for (int i = 0; i < 4; i++) {
     int remainder = 8;
     if (prefix_length >= 8) {
@@ skipping 49 matching lines @@
       return -1;
     }
     count++;
   }
   if (count < 4)
     return -1;
   return prefix_length;
 }
 
 }  // namespace chromeos