This converts the Shill clients to allow propagation of shill errors

chromeos/dbus/shill_client_helper.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/dbus/shill_client_helper.h"
 
 #include "base/bind.h"
 #include "base/values.h"
 #include "dbus/message.h"
 #include "dbus/object_proxy.h"
 #include "dbus/values_util.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 namespace chromeos {
 
 ShillClientHelper::ShillClientHelper(dbus::Bus* bus,
-                                           dbus::ObjectProxy* proxy)
+                                     dbus::ObjectProxy* proxy)
     : blocking_method_caller_(bus, proxy),
       proxy_(proxy),
       weak_ptr_factory_(this) {
 }
 
 ShillClientHelper::~ShillClientHelper() {
 }
 
-void ShillClientHelper::SetPropertyChangedHandler(
-    const PropertyChangedHandler& handler) {
-  property_changed_handler_ = handler;
+void ShillClientHelper::AddPropertyChangedObserver(
+    PropertyChangedObserver* obs) {

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

Rename this to |observer|

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

Done.

+  observer_list_.AddObserver(obs);
 }
 
-void ShillClientHelper::ResetPropertyChangedHandler() {
-  property_changed_handler_.Reset();
+void ShillClientHelper::RemovePropertyChangedObserver(
+    PropertyChangedObserver* obs) {

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

ditto.

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

Done.

+  observer_list_.RemoveObserver(obs);
 }
 
 void ShillClientHelper::MonitorPropertyChanged(
     const std::string& interface_name) {
   // We are not using dbus::PropertySet to monitor PropertyChanged signal
   // because the interface is not "org.freedesktop.DBus.Properties".
   proxy_->ConnectToSignal(interface_name,
                           flimflam::kMonitorPropertyChanged,
                           base::Bind(&ShillClientHelper::OnPropertyChanged,
                                      weak_ptr_factory_.GetWeakPtr()),
@@ skipping 12 matching lines @@
 
 void ShillClientHelper::CallObjectPathMethod(
     dbus::MethodCall* method_call,
     const ObjectPathDBusMethodCallback& callback) {
   proxy_->CallMethod(method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
                      base::Bind(&ShillClientHelper::OnObjectPathMethod,
                                 weak_ptr_factory_.GetWeakPtr(),
                                 callback));
 }
 
+void ShillClientHelper::CallObjectPathMethodWithErrorCallback(
+    dbus::MethodCall* method_call,
+    const ObjectPathDBusMethodCallback& callback,
+    const ErrorCallback& error_callback) {
+  proxy_->CallMethodWithErrorCallback(
+      method_call,
+      dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+      base::Bind(&ShillClientHelper::OnObjectPathMethod,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 callback),
+      base::Bind(&ShillClientHelper::OnError,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 error_callback));
+}
+
 void ShillClientHelper::CallDictionaryValueMethod(
     dbus::MethodCall* method_call,
     const DictionaryValueCallback& callback) {
   proxy_->CallMethod(method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
                      base::Bind(&ShillClientHelper::OnDictionaryValueMethod,
                                 weak_ptr_factory_.GetWeakPtr(),
                                 callback));
 }
 
 void ShillClientHelper::CallVoidMethodWithErrorCallback(
@@ skipping 62 matching lines @@
   base::DictionaryValue* result = NULL;
   if (!value || !value->GetAsDictionary(&result)) {
     delete value;
     return NULL;
   }
   return result;
 }
 
 // static
 void ShillClientHelper::AppendValueDataAsVariant(dbus::MessageWriter* writer,
-                                                    const base::Value& value) {
+                                                 const base::Value& value) {
   // Support basic types and string-to-string dictionary.
   switch (value.GetType()) {
     case base::Value::TYPE_DICTIONARY: {
       const base::DictionaryValue* dictionary = NULL;
       value.GetAsDictionary(&dictionary);
       dbus::MessageWriter variant_writer(NULL);
       writer->OpenVariant("a{ss}", &variant_writer);
       dbus::MessageWriter array_writer(NULL);
       variant_writer.OpenArray("{ss}", &array_writer);
       for (base::DictionaryValue::Iterator it(*dictionary);
@@ skipping 20 matching lines @@
     case base::Value::TYPE_STRING:
       dbus::AppendBasicTypeValueDataAsVariant(writer, value);
       break;
     default:
       DLOG(ERROR) << "Unexpected type " << value.GetType();
   }
 
 }
 
 void ShillClientHelper::OnSignalConnected(const std::string& interface,
-                                             const std::string& signal,
-                                             bool success) {
+                                          const std::string& signal,
+                                          bool success) {
   LOG_IF(ERROR, !success) << "Connect to " << interface << " " << signal
                           << " failed.";
 }
 
 void ShillClientHelper::OnPropertyChanged(dbus::Signal* signal) {
-  if (property_changed_handler_.is_null())
+  if (!observer_list_.might_have_observers())

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

This early exit was necessary since calling Run() against NULL callback results
in SIGSEGV.
With observer list, we no longer have to check this.

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

Sure, but why not early exit anyhow, to avoid having to create the reader and
pop the values?

     return;
 
   dbus::MessageReader reader(signal);
   std::string name;
   if (!reader.PopString(&name))
     return;
   scoped_ptr<base::Value> value(dbus::PopDataAsValue(&reader));
   if (!value.get())
     return;
-  property_changed_handler_.Run(name, *value);
+
+  FOR_EACH_OBSERVER(PropertyChangedObserver, observer_list_,
+                    OnPropertyChanged(name, *value));
+
 }
 
 void ShillClientHelper::OnVoidMethod(const VoidDBusMethodCallback& callback,
-                                        dbus::Response* response) {
+                                     dbus::Response* response) {
   if (!response) {
     callback.Run(DBUS_METHOD_CALL_FAILURE);
     return;
   }
   callback.Run(DBUS_METHOD_CALL_SUCCESS);
 }
 
 void ShillClientHelper::OnObjectPathMethod(
     const ObjectPathDBusMethodCallback& callback,
     dbus::Response* response) {
@@ skipping 45 matching lines @@
   if (!value.get() || !value->GetAsDictionary(&result)) {
     const std::string error_name;  // No error name.
     const std::string error_message = "Invalid response.";
     error_callback.Run(error_name, error_message);
     return;
   }
   callback.Run(*result);
 }
 
 void ShillClientHelper::OnError(const ErrorCallback& error_callback,
-                                   dbus::ErrorResponse* response) {
+                                dbus::ErrorResponse* response) {
   std::string error_name;
   std::string error_message;
   if (response) {
     // Error message may contain the error message as string.
     dbus::MessageReader reader(response);
     error_name = response->GetErrorName();
     reader.PopString(&error_message);
   }
   error_callback.Run(error_name, error_message);
 }
 
 }  // namespace chromeos