privetd: Expose dbus API

chromeos/dbus/privet_daemon_manager_client.cc

+// Copyright 2015 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/privet_daemon_manager_client.h"
+
+#include "base/bind.h"
+#include "base/logging.h"
+#include "base/memory/weak_ptr.h"
+#include "base/message_loop/message_loop.h"
+#include "base/observer_list.h"
+#include "dbus/bus.h"
+#include "dbus/message.h"
+#include "dbus/object_manager.h"
+#include "dbus/object_proxy.h"
+#include "dbus/values_util.h"
+
+namespace dbus {

[hashimoto, 2015/03/18 05:52:50]

Generally speaking, you shouldn't add arbitrary stuffs to other namespaces.

If you really want to let dbus::PropertySet handle types under namespace
chromeos, you should change dbus::PropertySet's design.

[dtapuska, 2015/03/18 14:28:54]

I won't use Property template; but use the PropertyBase class; and provide my
own implementation of that. It is a cleaner approach.

+
+template <>
+bool Property<
+    std::map<std::string,
+             chromeos::PrivetDaemonManagerClient::PairingInfoVariant>>::
+    PopValueFromReader(MessageReader* reader);
+
+template <>
+void Property<
+    std::map<std::string,
+             chromeos::PrivetDaemonManagerClient::PairingInfoVariant>>::
+    AppendSetValueToWriter(MessageWriter* writer);
+
+template class Property<
+    std::map<std::string,
+             chromeos::PrivetDaemonManagerClient::PairingInfoVariant>>;
+
+template <>
+bool Property<
+    std::map<std::string,
+             chromeos::PrivetDaemonManagerClient::PairingInfoVariant>>::
+    PopValueFromReader(MessageReader* reader) {
+  MessageReader variant_reader(NULL);
+  MessageReader array_reader(NULL);
+  if (!reader->PopVariant(&variant_reader) ||
+      !variant_reader.PopArray(&array_reader)) {
+    return false;
+  }
+  value_.clear();
+  while (array_reader.HasMoreData()) {
+    MessageReader dict_entry_reader(NULL);
+    if (!array_reader.PopDictEntry(&dict_entry_reader))
+      return false;
+    std::string key;
+    if (!dict_entry_reader.PopString(&key))
+      return false;
+
+    MessageReader field_reader(NULL);
+    if (!dict_entry_reader.PopVariant(&field_reader))
+      return false;
+
+    chromeos::PrivetDaemonManagerClient::PairingInfoVariant value;
+    if (field_reader.GetDataSignature() == "s") {
+      if (!field_reader.PopString(&value.string))
+        return false;
+      value.type =
+          chromeos::PrivetDaemonManagerClient::PairingInfoVariant::Type::STRING;
+    } else if (field_reader.GetDataSignature() == "ay") {
+      const uint8* bytes = NULL;
+      size_t length = 0;
+      if (!field_reader.PopArrayOfBytes(&bytes, &length))
+        return false;
+      value.blob.assign(bytes, bytes + length);
+      value.type =
+          chromeos::PrivetDaemonManagerClient::PairingInfoVariant::Type::BLOB;
+    }
+
+    value_[key] = value;
+  }
+  return true;
+}

[satorux1, 2015/03/17 17:39:11]

This and the next function seem to be complex. how about adding unit tests?

[dtapuska, 2015/03/18 14:28:53]

Done.

+
+template <>
+void Property<
+    std::map<std::string,
+             chromeos::PrivetDaemonManagerClient::PairingInfoVariant>>::
+    AppendSetValueToWriter(MessageWriter* writer) {
+  MessageWriter variant_writer(NULL);
+  MessageWriter dict_writer(NULL);
+  writer->OpenVariant("a{sv}", &variant_writer);
+  variant_writer.OpenArray("{sv}", &dict_writer);
+  for (const auto& pair : set_value_) {
+    dbus::MessageWriter entry_writer(NULL);
+    dict_writer.OpenDictEntry(&entry_writer);
+    entry_writer.AppendString(pair.first);
+    switch (pair.second.type) {
+      case chromeos::PrivetDaemonManagerClient::PairingInfoVariant::Type::
+          STRING:
+        entry_writer.AppendVariantOfString(pair.second.string);
+        break;
+      case chromeos::PrivetDaemonManagerClient::PairingInfoVariant::Type::
+          BLOB: {
+        MessageWriter array_writer(NULL);
+        entry_writer.OpenVariant("ay", &array_writer);
+        array_writer.AppendArrayOfBytes(pair.second.blob.data(),
+                                        pair.second.blob.size());
+        entry_writer.CloseContainer(&array_writer);
+      }
+    }
+    dict_writer.CloseContainer(&entry_writer);
+  }
+  variant_writer.CloseContainer(&dict_writer);
+  writer->CloseContainer(&variant_writer);
+}

[satorux1, 2015/03/17 17:39:11]

nit: blank line here.

[dtapuska, 2015/03/18 14:28:54]

Done.

+}  // namespace dbus
+
+namespace chromeos {
+
+// TODO(benchan): Move these constants to system_api.
+namespace privetd {
+const char kPrivetdServiceName[] = "org.chromium.privetd";

[hashimoto, 2015/03/18 05:52:50]

It's better to put these constants in the anonymous namespace below as long as
they are not used in other files.

[dtapuska, 2015/03/18 14:28:54]

Done.

+const char kPrivetdServicePath[] = "/org/chromium/privetd";
+const char kPrivetdManagerPath[] = "/org/chromium/privetd/Manager";
+const char kManagerInterfaceName[] = "org.chromium.privetd.Manager";
+const char kSetDescriptionMethod[] = "SetDescription";
+const char kGCDBootstrapStateProperty[] = "GCDBootstrapState";
+const char kWiFiBootstrapStateProperty[] = "WiFiBootstrapState";
+const char kPairingInfoProperty[] = "PairingInfo";
+const char kDescriptionProperty[] = "Description";
+const char kNameProperty[] = "Name";
+
+}  // namespace privetd
+
+namespace {
+
+// The PrivetDaemonManagerClient implementation used in production.
+class PrivetDaemonManagerClientImpl : public PrivetDaemonManagerClient,
+                                      public dbus::ObjectManager::Interface {
+ public:
+  PrivetDaemonManagerClientImpl();
+  ~PrivetDaemonManagerClientImpl() override;
+
+  // PrivetDaemonManagerClient overrides.
+  void AddObserver(Observer* observer) override;
+  void RemoveObserver(Observer* observer) override;
+  void SetDescription(const std::string& description,
+                      const VoidDBusMethodCallback& callback) override;
+  const ManagerProperties* GetManagerProperties() override;
+
+  // DBusClient overrides.
+  void Init(dbus::Bus* bus) override;
+
+  // dbus::ObjectManager::Interface overrides.
+  dbus::PropertySet* CreateProperties(
+      dbus::ObjectProxy* object_proxy,
+      const dbus::ObjectPath& object_path,
+      const std::string& interface_name) override;
+  void ObjectAdded(const dbus::ObjectPath& object_path,
+                   const std::string& interface_name) override;
+  void ObjectRemoved(const dbus::ObjectPath& object_path,
+                     const std::string& interface_name) override;
+
+ private:
+  // Called by dbus::PropertySet when a property value is changed,
+  // either by result of a signal or response to a GetAll() or Get()
+  // call. Informs observers.
+  void OnManagerPropertyChanged(const std::string& property_name);
+  void OnVoidDBusMethod(const VoidDBusMethodCallback& callback,
+                        dbus::Response* response);
+
+  // List of observers interested in event notifications from us.
+  ObserverList<Observer> observers_;
+  dbus::ObjectManager* object_manager_;
+  base::WeakPtrFactory<PrivetDaemonManagerClientImpl> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(PrivetDaemonManagerClientImpl);
+};
+
+PrivetDaemonManagerClientImpl::PrivetDaemonManagerClientImpl()
+    : object_manager_(nullptr), weak_ptr_factory_(this) {
+}
+
+PrivetDaemonManagerClientImpl::~PrivetDaemonManagerClientImpl() {
+  if (object_manager_) {
+    object_manager_->UnregisterInterface(privetd::kManagerInterfaceName);
+  }
+}
+
+void PrivetDaemonManagerClientImpl::AddObserver(Observer* observer) {
+  DCHECK(observer);
+  observers_.AddObserver(observer);
+}
+
+void PrivetDaemonManagerClientImpl::RemoveObserver(Observer* observer) {
+  DCHECK(observer);
+  observers_.RemoveObserver(observer);
+}
+
+void PrivetDaemonManagerClientImpl::SetDescription(
+    const std::string& description,
+    const VoidDBusMethodCallback& callback) {
+  dbus::ObjectProxy* object_proxy = object_manager_->GetObjectProxy(
+      dbus::ObjectPath(privetd::kPrivetdManagerPath));
+  if (!object_proxy) {
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(&PrivetDaemonManagerClientImpl::OnVoidDBusMethod,
+                   weak_ptr_factory_.GetWeakPtr(), callback, nullptr));
+    return;
+  }
+
+  dbus::MethodCall method_call(privetd::kManagerInterfaceName,
+                               privetd::kSetDescriptionMethod);
+  dbus::MessageWriter writer(&method_call);
+  writer.AppendString(description);
+  object_proxy->CallMethod(
+      &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+      base::Bind(&PrivetDaemonManagerClientImpl::OnVoidDBusMethod,
+                 weak_ptr_factory_.GetWeakPtr(), callback));
+}
+
+const PrivetDaemonManagerClient::ManagerProperties*
+PrivetDaemonManagerClientImpl::GetManagerProperties() {
+  return static_cast<ManagerProperties*>(object_manager_->GetProperties(
+      dbus::ObjectPath(privetd::kPrivetdManagerPath),
+      privetd::kManagerInterfaceName));
+}
+
+void PrivetDaemonManagerClientImpl::Init(dbus::Bus* bus) {
+  object_manager_ =
+      bus->GetObjectManager(privetd::kPrivetdServiceName,
+                            dbus::ObjectPath(privetd::kPrivetdServicePath));
+  object_manager_->RegisterInterface(privetd::kManagerInterfaceName, this);
+}
+
+dbus::PropertySet* PrivetDaemonManagerClientImpl::CreateProperties(
+    dbus::ObjectProxy* object_proxy,
+    const dbus::ObjectPath& object_path,
+    const std::string& interface_name) {
+  dbus::PropertySet* properties = nullptr;
+  if (interface_name == privetd::kManagerInterfaceName) {
+    properties = new ManagerProperties(
+        object_proxy, interface_name,
+        base::Bind(&PrivetDaemonManagerClientImpl::OnManagerPropertyChanged,
+                   weak_ptr_factory_.GetWeakPtr()));
+  } else {
+    NOTREACHED() << "Unhandled interface name " << interface_name;
+  }
+  return properties;
+}
+
+void PrivetDaemonManagerClientImpl::ObjectAdded(
+    const dbus::ObjectPath& object_path,
+    const std::string& interface_name) {
+  if (interface_name == privetd::kManagerInterfaceName) {
+    FOR_EACH_OBSERVER(Observer, observers_, ManagerAdded());
+  } else {
+    NOTREACHED() << "Unhandled interface name " << interface_name;
+  }
+}
+
+void PrivetDaemonManagerClientImpl::ObjectRemoved(
+    const dbus::ObjectPath& object_path,
+    const std::string& interface_name) {
+  if (interface_name == privetd::kManagerInterfaceName) {
+    FOR_EACH_OBSERVER(Observer, observers_, ManagerRemoved());
+  } else {
+    NOTREACHED() << "Unhandled interface name " << interface_name;
+  }
+}
+
+void PrivetDaemonManagerClientImpl::OnManagerPropertyChanged(
+    const std::string& property_name) {
+  FOR_EACH_OBSERVER(Observer, observers_,
+                    ManagerPropertyChanged(property_name));
+}
+
+void PrivetDaemonManagerClientImpl::OnVoidDBusMethod(
+    const VoidDBusMethodCallback& callback,
+    dbus::Response* response) {
+  callback.Run(response ? DBUS_METHOD_CALL_SUCCESS : DBUS_METHOD_CALL_FAILURE);
+}
+
+}  // namespace
+
+PrivetDaemonManagerClient::ManagerProperties::ManagerProperties(
+    dbus::ObjectProxy* object_proxy,
+    const std::string& interface_name,
+    const PropertyChangedCallback& callback)
+    : dbus::PropertySet(object_proxy, interface_name, callback) {
+  RegisterProperty(privetd::kWiFiBootstrapStateProperty,
+                   &wifi_bootstrap_state_);
+  RegisterProperty(privetd::kGCDBootstrapStateProperty, &gcd_bootstrap_state_);
+  RegisterProperty(privetd::kPairingInfoProperty, &pairing_info_);
+  RegisterProperty(privetd::kDescriptionProperty, &description_);
+  RegisterProperty(privetd::kNameProperty, &name_);
+}
+
+PrivetDaemonManagerClient::ManagerProperties::~ManagerProperties() {
+}
+
+PrivetDaemonManagerClient::Observer::~Observer() {

[hashimoto, 2015/03/18 05:52:50]

If you want to provide empty implementation for interface methods rather than
using "=0", you can do it in the header.
Please consult existing observer classes as examples.
(https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&q=c...)

[dtapuska, 2015/03/18 14:28:54]

Empty implementations should not be done in the header; as per OWNER comments in
the following review...
https://codereview.chromium.org/887083002/diff/80001/chromeos/dbus/leadership...

I'll make them pure virtual and put the dtor in the cc file. This should follow
all guides/comments.

+}
+
+void PrivetDaemonManagerClient::Observer::ManagerAdded() {
+}
+
+void PrivetDaemonManagerClient::Observer::ManagerRemoved() {
+}
+
+void PrivetDaemonManagerClient::Observer::ManagerPropertyChanged(
+    const std::string& property_name) {
+}
+
+PrivetDaemonManagerClient::PrivetDaemonManagerClient() {
+}
+
+PrivetDaemonManagerClient::~PrivetDaemonManagerClient() {
+}
+
+// static
+PrivetDaemonManagerClient* PrivetDaemonManagerClient::Create() {
+  return new PrivetDaemonManagerClientImpl();
+}
+
+}  // namespace chromeos