This converts the Shill clients to allow propagation of shill errors

chrome/browser/chromeos/cros/cros_network_functions.cc

Index: chrome/browser/chromeos/cros/cros_network_functions.cc
diff --git a/chrome/browser/chromeos/cros/cros_network_functions.cc b/chrome/browser/chromeos/cros/cros_network_functions.cc
index 10139fabe604f1e3a8a7da6f33358479df0a365c..95f58276102d896abf32615ac19abf7373e90ae7 100644
--- a/chrome/browser/chromeos/cros/cros_network_functions.cc
+++ b/chrome/browser/chromeos/cros/cros_network_functions.cc
@@ -26,56 +26,84 @@ 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.
@@ -180,7 +208,14 @@ class DataPlanUpdateWatcher : public CrosNetworkWatcher {
 };
 
 // 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,
@@ -190,6 +225,25 @@ void RunCallbackWithDictionaryValue(const NetworkPropertiesCallback& callback,
   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,
@@ -316,20 +370,23 @@ void CrosSetNetworkServiceProperty(const std::string& service_path,
                                    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,
@@ -337,19 +394,23 @@ void CrosSetNetworkIPConfigProperty(const std::string& ipconfig_path,
                                     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) {
@@ -423,7 +484,9 @@ void CrosRequestNetworkProfileProperties(
     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(
@@ -433,7 +496,10 @@ void CrosRequestNetworkProfileEntryProperties(
   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));
 }
@@ -459,7 +525,8 @@ void CrosRequestHiddenWifiNetworkProperties(
   // |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(
@@ -489,32 +556,38 @@ void CrosRequestVirtualNetworkProperties(
   // |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));
   }
 }
 
@@ -563,7 +636,7 @@ void CrosRequestChangePin(const std::string& 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,
@@ -579,7 +652,8 @@ void CrosRequestCellularRegister(const std::string& 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;
 }
 
@@ -667,7 +741,7 @@ bool CrosRemoveIPConfig(const std::string& 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) {
@@ -758,7 +832,8 @@ bool CrosGetWifiAccessPoints(WifiAccessPointVector* result) {
 
 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) {