Add FlimflamIPConfigClient

chrome/browser/chromeos/dbus/flimflam_ipconfig_client.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/dbus/flimflam_ipconfig_client.h"
+
+#include "base/bind.h"
+#include "base/chromeos/chromeos_version.h"
+#include "dbus/bus.h"
+#include "dbus/message.h"
+#include "dbus/object_path.h"
+#include "dbus/object_proxy.h"
+#include "dbus/values_util.h"
+#include "third_party/cros_system_api/dbus/service_constants.h"
+
+namespace chromeos {
+
+namespace {
+
+// The FlimflamIPConfigClient implementation.
+class FlimflamIPConfigClientImpl : public FlimflamIPConfigClient {

[stevenjb, 2012/03/29 00:28:14]

For complex classes like these, it is easier to read when the class definition
is separated from the implementation. I know we inline Impl classes a lot, but
it is a trend that we should reverse.

[hashimoto, 2012/03/29 03:04:46]

Done.

[satorux1, 2012/03/29 17:58:44]

I personally prefer not to separate the implementation from the class
definition, mostly because I hate to type more and touch two places when I
change function signatures. Besides, we inline functions inside classes in every
other language, so I'm used to it.

I think things like the are up to the author's taste. Since Ryo agreed to make
them separate, we should go with separation in this file. :)

+ public:
+  explicit FlimflamIPConfigClientImpl(dbus::Bus* bus)
+      : proxy_(bus->GetObjectProxy(
+          flimflam::kFlimflamServiceName,
+          dbus::ObjectPath(flimflam::kFlimflamServicePath))),
+        weak_ptr_factory_(this) {
+    // We are not using dbus::PropertySet to monitor PropertyChanged signal
+    // because the interface is not "org.freedesktop.DBus.Properties".
+    proxy_->ConnectToSignal(
+        flimflam::kFlimflamIPConfigInterface,
+        flimflam::kMonitorPropertyChanged,
+        base::Bind(&FlimflamIPConfigClientImpl::OnPropertyChanged,
+                   weak_ptr_factory_.GetWeakPtr()),
+        base::Bind(&FlimflamIPConfigClientImpl::OnSignalConnected,
+                   weak_ptr_factory_.GetWeakPtr()));
+  }
+
+  // FlimflamIPConfigClient override.
+  virtual void SetPropertyChangedHandler(
+      const PropertyChangedHandler& handler) OVERRIDE {
+    property_changed_handler_ = handler;
+  }
+
+  // FlimflamIPConfigClient override.
+  virtual void ResetPropertyChangedHandler() OVERRIDE {
+    property_changed_handler_.Reset();
+  }
+
+  // FlimflamIPConfigClient override.
+  virtual void GetProperties(const DictionaryValueCallback& callback) OVERRIDE {
+    dbus::MethodCall method_call(flimflam::kFlimflamIPConfigInterface,
+                                 flimflam::kGetPropertiesFunction);
+    proxy_->CallMethod(
+        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        base::Bind(&FlimflamIPConfigClientImpl::OnDictionaryValueMethod,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   callback));
+  }
+
+  // FlimflamIPConfigClient override.
+  virtual void SetProperty(const std::string& name,
+                           const base::Value& value,
+                           const VoidCallback& callback) OVERRIDE {
+    dbus::MethodCall method_call(flimflam::kFlimflamIPConfigInterface,
+                                 flimflam::kSetPropertyFunction);
+    dbus::MessageWriter writer(&method_call);
+    writer.AppendString(name);
+    // IPConfig supports writing basic type and string array properties.
+    if (value.GetType() == base::Value::TYPE_LIST) {

[stevenjb, 2012/03/29 00:28:14]

I think this would be better as a switch(value.GetType()), with the supported
types spelled out. e.g. How does AppendBasicTypeValueData handle DictionaryValue
or BinaryValue?

[hashimoto, 2012/03/29 03:04:46]

Done.

+      const base::ListValue* list_value = NULL;
+      value.GetAsList(&list_value);
+      dbus::MessageWriter sub_writer(NULL);
+      writer.OpenArray("s", &sub_writer);
+      for (base::ListValue::const_iterator it = list_value->begin();
+           it != list_value->end();
+           ++it) {
+        std::string str;
+        (*it)->GetAsString(&str);
+        sub_writer.AppendString(str);
+      }
+      writer.CloseContainer(&sub_writer);
+    } else {
+      dbus::AppendBasicTypeValueData(&writer, value);
+    }
+    proxy_->CallMethod(&method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+                       base::Bind(&FlimflamIPConfigClientImpl::OnVoidMethod,
+                       weak_ptr_factory_.GetWeakPtr(),
+                       callback));
+  }
+
+  // FlimflamIPConfigClient override.
+  virtual void ClearProperty(const std::string& name,
+                             const VoidCallback& callback) OVERRIDE {
+    dbus::MethodCall method_call(flimflam::kFlimflamIPConfigInterface,
+                                 flimflam::kClearPropertyFunction);
+    dbus::MessageWriter writer(&method_call);
+    writer.AppendString(name);
+    proxy_->CallMethod(&method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+                       base::Bind(&FlimflamIPConfigClientImpl::OnVoidMethod,
+                       weak_ptr_factory_.GetWeakPtr(),
+                       callback));
+  }
+
+  // FlimflamIPConfigClient override.
+  virtual void Remove(const VoidCallback& callback) OVERRIDE {

[stevenjb, 2012/03/29 00:28:14]

I think this should be named RemoveConfig.

[hashimoto, 2012/03/29 03:04:46]

The actual method name is "Remove".
(http://git.chromium.org/gitweb/?p=chromiumos/platform/system_api.git;a=blob;f...)
This constant is named "kRemoveConfigFunction" because there are two "Remove" in
the same namespace.  (I think there should be only one constant named
kRemoveFunction, though.)

[stevenjb, 2012/03/29 18:56:16]

Well, I think flimflam::kRemoveConfigFunction and RemoveConfig is more clear
(the other should probably also be more specific - functions like 'Add' and
'Remove' are harder to search for, etc), but this is fine.

+    dbus::MethodCall method_call(flimflam::kFlimflamIPConfigInterface,
+                                 flimflam::kRemoveConfigFunction);
+    proxy_->CallMethod(&method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+                       base::Bind(&FlimflamIPConfigClientImpl::OnVoidMethod,
+                       weak_ptr_factory_.GetWeakPtr(),
+                       callback));
+  }
+
+ private:
+  // Handles the result of signal connection setup.
+  void OnSignalConnected(const std::string& interface,
+                         const std::string& signal,
+                         bool successed) {

[stevenjb, 2012/03/29 00:28:14]

'success' or 'succeeded'

[hashimoto, 2012/03/29 03:04:46]

Done.

+    LOG_IF(ERROR, !successed) << "Connect to " << interface << " " <<
+        signal << " failed.";

[stevenjb, 2012/03/29 00:28:14]

nit: start second line with <<

[hashimoto, 2012/03/29 03:04:46]

Done.

+  }
+
+  // Handles PropertyChanged signal.
+  virtual void OnPropertyChanged(dbus::Signal* signal) {
+    dbus::MessageReader reader(signal);
+    std::string name;
+    const bool name_popped = reader.PopString(&name);

[stevenjb, 2012/03/29 00:28:14]

nit: I think I prefer
if (!reader.PopString(&name))
  return;

[hashimoto, 2012/03/29 03:04:46]

Done.

+    scoped_ptr<base::Value> value(dbus::PopDataAsValue(&reader));
+    if (!name_popped || !value.get())
+      return;
+
+    if (!property_changed_handler_.is_null())
+      property_changed_handler_.Run(name, *value);
+  }
+
+  // Handles responses for methods without results.
+  virtual void OnVoidMethod(const VoidCallback& callback,
+                            dbus::Response* response) {
+    if (!response) {
+      callback.Run(FAILURE);
+      return;
+    }
+    callback.Run(SUCCESS);
+  }
+
+  // Handles responses for methods with DictionaryValue results.
+  virtual void OnDictionaryValueMethod(const DictionaryValueCallback& callback,
+                                       dbus::Response* response) {
+    if (!response) {
+      base::DictionaryValue result;
+      callback.Run(FAILURE, result);

[stevenjb, 2012/03/29 00:28:14]

callback.Run(FAILURE, base::DictionaryValue())

[hashimoto, 2012/03/29 03:04:46]

I tried it before, but clang said "error: C++98 requires an accessible copy
constructor for class 'base::DictionaryValue' when binding a reference to a
temporary; was private"

+      return;
+    }
+    dbus::MessageReader reader(response);
+    scoped_ptr<base::Value> value(dbus::PopDataAsValue(&reader));
+    base::DictionaryValue* result = NULL;
+    if (!value.get() || !value->GetAsDictionary(&result)) {
+      base::DictionaryValue result;
+      callback.Run(FAILURE, result);

[stevenjb, 2012/03/29 00:28:14]

callback.Run(FAILURE, base::DictionaryValue())

[hashimoto, 2012/03/29 03:04:46]

ditto.

+      return;
+    }
+    callback.Run(SUCCESS, *result);
+  }
+
+  dbus::ObjectProxy* proxy_;
+  base::WeakPtrFactory<FlimflamIPConfigClientImpl> weak_ptr_factory_;
+  PropertyChangedHandler property_changed_handler_;
+
+  DISALLOW_COPY_AND_ASSIGN(FlimflamIPConfigClientImpl);
+};
+
+// A stub implementation of FlimflamIPConfigClient.
+class FlimflamIPConfigClientStubImpl : public FlimflamIPConfigClient {
+ public:
+  FlimflamIPConfigClientStubImpl() {}
+
+  virtual ~FlimflamIPConfigClientStubImpl() {}
+
+  // FlimflamIPConfigClient override.
+  virtual void SetPropertyChangedHandler(
+      const PropertyChangedHandler& handler) OVERRIDE {}
+
+  // FlimflamIPConfigClient override.
+  virtual void ResetPropertyChangedHandler() OVERRIDE {}
+
+  // FlimflamIPConfigClient override.
+  virtual void GetProperties(const DictionaryValueCallback& callback) OVERRIDE {

[stevenjb, 2012/03/29 00:28:14]

Stub should invoke callback here and elsewhere.

[hashimoto, 2012/03/29 03:04:46]

Done.

+  }
+
+  // FlimflamIPConfigClient override.
+  virtual void SetProperty(const std::string& name,
+                           const base::Value& value,
+                           const VoidCallback& callback) OVERRIDE {}
+
+  // FlimflamIPConfigClient override.
+  virtual void ClearProperty(const std::string& name,
+                             const VoidCallback& callback) OVERRIDE {}
+
+  // FlimflamIPConfigClient override.
+  virtual void Remove(const VoidCallback& callback) OVERRIDE {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(FlimflamIPConfigClientStubImpl);
+};
+
+}  // namespace
+
+FlimflamIPConfigClient::FlimflamIPConfigClient() {}
+
+FlimflamIPConfigClient::~FlimflamIPConfigClient() {}
+
+// static
+FlimflamIPConfigClient* FlimflamIPConfigClient::Create(dbus::Bus* bus) {
+  if (base::chromeos::IsRunningOnChromeOS())
+    return new FlimflamIPConfigClientImpl(bus);
+  else
+    return new FlimflamIPConfigClientStubImpl();
+}
+
+}  // namespace chromeos