Enhance the DBus interface for peerd

chromeos/dbus/peer_daemon_manager_client.cc

Index: chromeos/dbus/peer_daemon_manager_client.cc
diff --git a/chromeos/dbus/peer_daemon_manager_client.cc b/chromeos/dbus/peer_daemon_manager_client.cc
index b2ba214a23630632f274b48bac508c7769677022..78c44542030ce88a957098c0023ae50b743369c0 100644
--- a/chromeos/dbus/peer_daemon_manager_client.cc
+++ b/chromeos/dbus/peer_daemon_manager_client.cc
@@ -8,19 +8,23 @@
 #include "base/callback.h"
 #include "base/logging.h"
 #include "base/memory/weak_ptr.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 chromeos {
-namespace {
 
 // TODO(benchan): Move these constants to system_api.
 namespace peerd {
 const char kPeerdServiceName[] = "org.chromium.peerd";
+const char kPeerdObjectManagerServicePath[] = "/org/chromium/peerd";
 const char kPeerdManagerPath[] = "/org/chromium/peerd/Manager";
 const char kManagerInterface[] = "org.chromium.peerd.Manager";
+const char kPeerInterface[] = "org.chromium.peerd.Peer";
+const char kServiceInterface[] = "org.chromium.peerd.Service";

[hashimoto, 2015/02/05 09:51:11]

nit: As noted in the header, please be consistent whether things are ordered
like Manager->Peer->Service or Manager->Service->Peer.

[dtapuska, 2015/02/05 19:51:16]

Done.

 const char kStartMonitoringMethod[] = "StartMonitoring";
 const char kStopMonitoringMethod[] = "StopMonitoring";
 const char kExposeServiceMethod[] = "ExposeService";
@@ -28,8 +32,11 @@ const char kRemoveExposedServiceMethod[] = "RemoveExposedService";
 const char kPingMethod[] = "Ping";
 }  // namespace peerd
 
+namespace {

[hashimoto, 2015/02/05 09:51:11]

unnamed namespace should start from before string constants above.
No need to give them external linkage.

[dtapuska, 2015/02/05 19:51:16]

Done.

+
 // The PeerDaemonManagerClient implementation used in production.
-class PeerDaemonManagerClientImpl : public PeerDaemonManagerClient {
+class PeerDaemonManagerClientImpl : public PeerDaemonManagerClient,
+                                    public dbus::ObjectManager::Interface {
  public:
   PeerDaemonManagerClientImpl();
   ~PeerDaemonManagerClientImpl() override;
@@ -38,6 +45,14 @@ class PeerDaemonManagerClientImpl : public PeerDaemonManagerClient {
   void Init(dbus::Bus* bus) override;
 
   // PeerDaemonManagerClient overrides:
+  void AddObserver(Observer* observer) override;
+  void RemoveObserver(Observer* observer) override;
+  std::vector<dbus::ObjectPath> GetPeers() override;
+  std::vector<dbus::ObjectPath> GetServices() override;
+  PeerProperties* GetPeerProperties(
+      const dbus::ObjectPath& object_path) override;
+  ServiceProperties* GetServiceProperties(
+      const dbus::ObjectPath& object_path) override;
   void StartMonitoring(
       const std::vector<std::string>& requested_technologies,
       const base::DictionaryValue& options,
@@ -53,55 +68,119 @@ class PeerDaemonManagerClientImpl : public PeerDaemonManagerClient {
                             const VoidDBusMethodCallback& callback) override;
   void Ping(const StringDBusMethodCallback& callback) override;
 
+  // dbus::ObjectManager::Interface override.

[hashimoto, 2015/02/05 09:51:11]

nit: "overrides:" instead of "override." to be consistent with other parts.

[dtapuska, 2015/02/05 19:51:16]

Done.

+  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:
   void OnStringDBusMethod(const StringDBusMethodCallback& callback,
                           dbus::Response* response);
   void OnVoidDBusMethod(const VoidDBusMethodCallback& callback,
                         dbus::Response* response);
+  void OnManagerPropertyChanged(const std::string& property_name);
+  void OnPeerPropertyChanged(const dbus::ObjectPath& object_path,
+                             const std::string& property_name);
+  void OnServicePropertyChanged(const dbus::ObjectPath& object_path,
+                                const std::string& property_name);
+
+  // List of observers interested in event notifications from us.
+  ObserverList<Observer> observers_;
+  dbus::ObjectManager* object_manager_;
 
-  dbus::ObjectProxy* peer_daemon_proxy_;
   base::WeakPtrFactory<PeerDaemonManagerClientImpl> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(PeerDaemonManagerClientImpl);
 };
 
 PeerDaemonManagerClientImpl::PeerDaemonManagerClientImpl()
-    : peer_daemon_proxy_(nullptr), weak_ptr_factory_(this) {
+    : object_manager_(nullptr), weak_ptr_factory_(this) {
 }
 
 PeerDaemonManagerClientImpl::~PeerDaemonManagerClientImpl() {
+  if (object_manager_) {
+    object_manager_->UnregisterInterface("org.chromium.leaderd.Manager");

[hashimoto, 2015/02/05 09:51:11]

This code doesn't match with RegisterInterface() calls below.

Please use constants.

[dtapuska, 2015/02/05 19:51:16]

Done.

+    object_manager_->UnregisterInterface("org.chromium.leaderd.Group");
+  }
+}
+
+void PeerDaemonManagerClientImpl::AddObserver(Observer* observer) {
+  DCHECK(observer);
+  observers_.AddObserver(observer);
+}
+
+void PeerDaemonManagerClientImpl::RemoveObserver(Observer* observer) {
+  DCHECK(observer);
+  observers_.RemoveObserver(observer);
+}
+
+std::vector<dbus::ObjectPath> PeerDaemonManagerClientImpl::GetPeers() {
+  return object_manager_->GetObjectsWithInterface(peerd::kPeerInterface);
+}
+
+std::vector<dbus::ObjectPath> PeerDaemonManagerClientImpl::GetServices() {
+  return object_manager_->GetObjectsWithInterface(peerd::kServiceInterface);
+}
+
+PeerDaemonManagerClient::PeerProperties*
+PeerDaemonManagerClientImpl::GetPeerProperties(
+    const dbus::ObjectPath& object_path) {
+  return static_cast<PeerProperties*>(
+      object_manager_->GetProperties(object_path, peerd::kPeerInterface));
+}
+
+PeerDaemonManagerClient::ServiceProperties*
+PeerDaemonManagerClientImpl::GetServiceProperties(
+    const dbus::ObjectPath& object_path) {
+  return static_cast<ServiceProperties*>(
+      object_manager_->GetProperties(object_path, peerd::kServiceInterface));
 }
 
 void PeerDaemonManagerClientImpl::StartMonitoring(
     const std::vector<std::string>& requested_technologies,
     const base::DictionaryValue& options,
     const StringDBusMethodCallback& callback) {
+  dbus::ObjectProxy* object_proxy = object_manager_->GetObjectProxy(
+      dbus::ObjectPath(peerd::kPeerdManagerPath));
+  if (!object_proxy) {
+    callback.Run(DBUS_METHOD_CALL_FAILURE, "");

[hashimoto, 2015/02/05 09:51:11]

nit: Please use std::string() instead of "" (cf.
https://codereview.chromium.org/13145003).


Please PostTask() instead of running the callback directly.
The same goes for other parts.

In the example below, the caller expects B() to run after A() (and B() runs
after A() in usual cases), but by directly running the callback here, it can be
A() after B() when something goes wrong.
This may result in a bug difficult to find which only happens in extremely rare
use cases.

void f(PeerDaemonManagerClientImpl* p) {
  p->StartMonitoring(base::Bind(&B));
  A();
}

[dtapuska, 2015/02/05 19:51:16]

Done.

+    return;
+  }
+
   dbus::MethodCall method_call(peerd::kManagerInterface,
                                peerd::kStartMonitoringMethod);
   dbus::MessageWriter writer(&method_call);
   writer.AppendArrayOfStrings(requested_technologies);
   dbus::AppendValueData(&writer, options);
-  peer_daemon_proxy_->CallMethod(
-      &method_call,
-      dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+  object_proxy->CallMethod(
+      &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
       base::Bind(&PeerDaemonManagerClientImpl::OnStringDBusMethod,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 callback));
+                 weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
 void PeerDaemonManagerClientImpl::StopMonitoring(
     const std::string& monitoring_token,
     const VoidDBusMethodCallback& callback) {
+  dbus::ObjectProxy* object_proxy = object_manager_->GetObjectProxy(
+      dbus::ObjectPath(peerd::kPeerdManagerPath));
+  if (!object_proxy) {
+    callback.Run(DBUS_METHOD_CALL_FAILURE);
+    return;
+  }
+
   dbus::MethodCall method_call(peerd::kManagerInterface,
                                peerd::kStopMonitoringMethod);
   dbus::MessageWriter writer(&method_call);
   writer.AppendString(monitoring_token);
-  peer_daemon_proxy_->CallMethod(
-      &method_call,
-      dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+  object_proxy->CallMethod(
+      &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
       base::Bind(&PeerDaemonManagerClientImpl::OnVoidDBusMethod,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 callback));
+                 weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
 void PeerDaemonManagerClientImpl::ExposeService(
@@ -109,6 +188,13 @@ void PeerDaemonManagerClientImpl::ExposeService(
     const std::map<std::string, std::string>& service_info,
     const base::DictionaryValue& options,
     const StringDBusMethodCallback& callback) {
+  dbus::ObjectProxy* object_proxy = object_manager_->GetObjectProxy(
+      dbus::ObjectPath(peerd::kPeerdManagerPath));
+  if (!object_proxy) {
+    callback.Run(DBUS_METHOD_CALL_FAILURE, "");
+    return;
+  }
+
   dbus::MethodCall method_call(peerd::kManagerInterface,
                                peerd::kExposeServiceMethod);
   dbus::MessageWriter writer(&method_call);
@@ -126,12 +212,10 @@ void PeerDaemonManagerClientImpl::ExposeService(
   writer.CloseContainer(&array_writer);
 
   dbus::AppendValueData(&writer, options);
-  peer_daemon_proxy_->CallMethod(
-      &method_call,
-      dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+  object_proxy->CallMethod(
+      &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
       base::Bind(&PeerDaemonManagerClientImpl::OnStringDBusMethod,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 callback));
+                 weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
 void PeerDaemonManagerClientImpl::RemoveExposedService(
@@ -139,27 +223,83 @@ void PeerDaemonManagerClientImpl::RemoveExposedService(
     const VoidDBusMethodCallback& callback) {
   dbus::MethodCall method_call(peerd::kManagerInterface,
                                peerd::kRemoveExposedServiceMethod);
+  dbus::ObjectProxy* object_proxy = object_manager_->GetObjectProxy(
+      dbus::ObjectPath(peerd::kPeerdManagerPath));
+  if (!object_proxy) {
+    callback.Run(DBUS_METHOD_CALL_FAILURE);
+    return;
+  }
   dbus::MessageWriter writer(&method_call);
   writer.AppendString(service_token);
-  peer_daemon_proxy_->CallMethod(
-      &method_call,
-      dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+  object_proxy->CallMethod(
+      &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
       base::Bind(&PeerDaemonManagerClientImpl::OnVoidDBusMethod,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 callback));
+                 weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
 void PeerDaemonManagerClientImpl::Ping(
     const StringDBusMethodCallback& callback) {
   dbus::MethodCall method_call(peerd::kManagerInterface,
                                peerd::kPingMethod);
+  dbus::ObjectProxy* object_proxy = object_manager_->GetObjectProxy(
+      dbus::ObjectPath(peerd::kPeerdManagerPath));
+  if (!object_proxy) {
+    callback.Run(DBUS_METHOD_CALL_FAILURE, "");
+    return;
+  }
   dbus::MessageWriter writer(&method_call);
-  peer_daemon_proxy_->CallMethod(
-      &method_call,
-      dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+  object_proxy->CallMethod(
+      &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
       base::Bind(&PeerDaemonManagerClientImpl::OnStringDBusMethod,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 callback));
+                 weak_ptr_factory_.GetWeakPtr(), callback));
+}
+
+dbus::PropertySet* PeerDaemonManagerClientImpl::CreateProperties(
+    dbus::ObjectProxy* object_proxy,
+    const dbus::ObjectPath& object_path,
+    const std::string& interface_name) {
+  dbus::PropertySet* properties = nullptr;
+  if (interface_name == peerd::kManagerInterface) {
+    properties = new ManagerProperties(
+        object_proxy,
+        base::Bind(&PeerDaemonManagerClientImpl::OnManagerPropertyChanged,
+                   weak_ptr_factory_.GetWeakPtr()));
+  } else if (interface_name == peerd::kServiceInterface) {
+    properties = new ServiceProperties(
+        object_proxy,
+        base::Bind(&PeerDaemonManagerClientImpl::OnServicePropertyChanged,
+                   weak_ptr_factory_.GetWeakPtr(), object_path));
+  } else if (interface_name == peerd::kPeerInterface) {
+    properties = new PeerProperties(
+        object_proxy,
+        base::Bind(&PeerDaemonManagerClientImpl::OnPeerPropertyChanged,
+                   weak_ptr_factory_.GetWeakPtr(), object_path));
+  }

[hashimoto, 2015/02/05 09:51:11]

How about leaving a log when interface_name is an unexpected value?
The same goes for ObjectAdded() and ObjectRemoved().

[dtapuska, 2015/02/05 19:51:16]

Done.

+  return properties;
+}
+
+void PeerDaemonManagerClientImpl::ObjectAdded(
+    const dbus::ObjectPath& object_path,
+    const std::string& interface_name) {
+  if (interface_name == peerd::kManagerInterface) {
+    FOR_EACH_OBSERVER(Observer, observers_, ManagerAdded());
+  } else if (interface_name == peerd::kServiceInterface) {
+    FOR_EACH_OBSERVER(Observer, observers_, ServiceAdded(object_path));
+  } else if (interface_name == peerd::kPeerInterface) {
+    FOR_EACH_OBSERVER(Observer, observers_, PeerAdded(object_path));
+  }
+}
+
+void PeerDaemonManagerClientImpl::ObjectRemoved(
+    const dbus::ObjectPath& object_path,
+    const std::string& interface_name) {
+  if (interface_name == peerd::kManagerInterface) {
+    FOR_EACH_OBSERVER(Observer, observers_, ManagerRemoved());
+  } else if (interface_name == peerd::kServiceInterface) {
+    FOR_EACH_OBSERVER(Observer, observers_, ServiceRemoved(object_path));
+  } else if (interface_name == peerd::kPeerInterface) {
+    FOR_EACH_OBSERVER(Observer, observers_, PeerRemoved(object_path));
+  }
 }
 
 void PeerDaemonManagerClientImpl::OnStringDBusMethod(
@@ -187,13 +327,68 @@ void PeerDaemonManagerClientImpl::OnVoidDBusMethod(
 }
 
 void PeerDaemonManagerClientImpl::Init(dbus::Bus* bus) {
-  peer_daemon_proxy_ =
-      bus->GetObjectProxy(peerd::kPeerdServiceName,
-                          dbus::ObjectPath(peerd::kPeerdManagerPath));
+  object_manager_ = bus->GetObjectManager(
+      peerd::kPeerdServiceName,
+      dbus::ObjectPath(peerd::kPeerdObjectManagerServicePath));
+  object_manager_->RegisterInterface(peerd::kManagerInterface, this);
+  object_manager_->RegisterInterface(peerd::kPeerInterface, this);
+  object_manager_->RegisterInterface(peerd::kServiceInterface, this);
+}
+
+void PeerDaemonManagerClientImpl::OnManagerPropertyChanged(
+    const std::string& property_name) {
+  FOR_EACH_OBSERVER(Observer, observers_,
+                    ManagerPropertyChanged(property_name));
+}
+
+void PeerDaemonManagerClientImpl::OnPeerPropertyChanged(
+    const dbus::ObjectPath& object_path,
+    const std::string& property_name) {
+  FOR_EACH_OBSERVER(Observer, observers_,
+                    PeerPropertyChanged(object_path, property_name));
+}
+
+void PeerDaemonManagerClientImpl::OnServicePropertyChanged(
+    const dbus::ObjectPath& object_path,
+    const std::string& property_name) {
+  FOR_EACH_OBSERVER(Observer, observers_,
+                    ServicePropertyChanged(object_path, property_name));
 }
 
 }  // namespace
 
+PeerDaemonManagerClient::ManagerProperties::ManagerProperties(
+    dbus::ObjectProxy* object_proxy,
+    const PropertyChangedCallback& callback)
+    : dbus::PropertySet{object_proxy, peerd::kManagerInterface, callback} {
+  RegisterProperty("MonitoredTechnologies", &monitored_technologies);
+}
+
+PeerDaemonManagerClient::ManagerProperties::~ManagerProperties() {
+}
+
+PeerDaemonManagerClient::PeerProperties::PeerProperties(
+    dbus::ObjectProxy* object_proxy,
+    const PropertyChangedCallback& callback)
+    : dbus::PropertySet{object_proxy, peerd::kPeerInterface, callback} {
+  RegisterProperty("UUID", &u_u_i_d);
+  RegisterProperty("LastSeen", &last_seen);
+}
+
+PeerDaemonManagerClient::PeerProperties::~PeerProperties() {
+}
+
+PeerDaemonManagerClient::ServiceProperties::ServiceProperties(
+    dbus::ObjectProxy* object_proxy,
+    const PropertyChangedCallback& callback)
+    : dbus::PropertySet{object_proxy, peerd::kServiceInterface, callback} {
+  RegisterProperty("ServiceId", &service_id);
+  RegisterProperty("ServiceInfo", &service_info);
+  RegisterProperty("IpInfos", &ip_infos);
+}
+
+PeerDaemonManagerClient::ServiceProperties::~ServiceProperties() {
+}
 
 PeerDaemonManagerClient::PeerDaemonManagerClient() {
 }