dbus-binding-generator: Use abstract interfaces instead of proxies

Use abstract proxy interfaces instead of concrete proxy instances.
This allows client to mock out APIs and facilitate testing.

Had to add the ability to obtain an object path from the interface
as well as set properties on interfaces, as these were used by the
current callers.

BUG: 26092352
Change-Id: I853d37f60fbe6d56e33171c82cd74520e38711cf

2 files changed, 94 insertions, 48 deletions

diff --git a/chromeos-dbus-bindings/proxy_generator.cc b/chromeos-dbus-bindings/proxy_generator.cc
index 9ce0117..5a18bc1 100644
--- a/chromeos-dbus-bindings/proxy_generator.cc
+++ b/chromeos-dbus-bindings/proxy_generator.cc
@@ -211,6 +211,8 @@ void ProxyGenerator::GenerateInterfaceProxyInterface(
     AddSignalHandlerRegistration(signal, interface.name, true, text);
   }
   AddProperties(config, interface, true, text);
+  text->AddBlankLine();
+  text->AddLine("virtual const dbus::ObjectPath& GetObjectPath() const = 0;");
 
   text->PopOffset();
   text->AddLine("};");
@@ -277,9 +279,10 @@ void ProxyGenerator::GenerateInterfaceProxy(const ServiceConfig& config,
   }
   if (!config.object_manager.name.empty() && !interface.properties.empty()) {
     text->AddLine("PropertySet* property_set_;");
-    text->AddLine(StringPrintf("base::Callback<void(%s*, const std::string&)> "
-                               "on_property_changed_;",
-                               proxy_name.c_str()));
+    text->AddLine(
+        StringPrintf("base::Callback<void(%sInterface*, const std::string&)> "
+                     "on_property_changed_;",
+                     proxy_name.c_str()));
   }
   text->AddLine("dbus::ObjectProxy* dbus_object_proxy_;");
   text->AddBlankLine();
@@ -416,7 +419,7 @@ void ProxyGenerator::AddReleaseObjectProxy(IndentedText* text) {
 // static
 void ProxyGenerator::AddGetObjectPath(IndentedText* text) {
   text->AddBlankLine();
-  text->AddLine("const dbus::ObjectPath& GetObjectPath() const {");
+  text->AddLine("const dbus::ObjectPath& GetObjectPath() const override {");
   text->AddLineWithOffset("return object_path_;", kBlockOffset);
   text->AddLine("}");
 }
@@ -434,7 +437,7 @@ void ProxyGenerator::AddPropertyPublicMethods(const string& class_name,
   text->AddBlankLine();
   text->AddLine("void SetPropertyChangedCallback(");
   text->AddLineWithOffset(
-      StringPrintf("const base::Callback<void(%s*, "
+      StringPrintf("const base::Callback<void(%sInterface*, "
                    "const std::string&)>& callback) {", class_name.c_str()),
       kLineContinuationOffset);
   text->AddLineWithOffset("on_property_changed_ = callback;", kBlockOffset);
@@ -583,6 +586,24 @@ void ProxyGenerator::AddProperties(const ServiceConfig& config,
           kBlockOffset);
       text->AddLine("}");
     }
+    if (!declaration_only)
+      text->AddBlankLine();
+    text->AddLineAndPushOffsetTo(
+        StringPrintf("%svoid set_%s(%s value,",
+                     declaration_only ? "virtual " : "",
+                     name.c_str(),
+                     type.c_str()),
+        1, '(');
+    text->AddLine(
+        StringPrintf("const base::Callback<void(bool)>& callback)%s",
+                     declaration_only ? " = 0;" : " override {"));
+    text->PopOffset();
+    if (!declaration_only) {
+      text->AddLineWithOffset(
+          StringPrintf("property_set_->%s.Set(value, callback);", name.c_str()),
+          kBlockOffset);
+      text->AddLine("}");
+    }
   }
 }
 
@@ -1012,7 +1033,7 @@ void ProxyGenerator::ObjectManager::AddInterfaceAccessors(
   // GetProxy().
   if (interface.path.empty()) {
     // We have no fixed path, so there could be multiple instances of this itf.
-    text->AddLine(StringPrintf("%s* Get%s(",
+    text->AddLine(StringPrintf("%sInterface* Get%s(",
                                 itf_name.MakeProxyName(true).c_str(),
                                 itf_name.MakeProxyName(false).c_str()));
     text->PushOffset(kLineContinuationOffset);
@@ -1031,7 +1052,7 @@ void ProxyGenerator::ObjectManager::AddInterfaceAccessors(
   } else {
     // We have a fixed path, so the object could be considered a "singleton".
     // Skip the object_path parameter and return the first available instance.
-    text->AddLine(StringPrintf("%s* Get%s() {",
+    text->AddLine(StringPrintf("%sInterface* Get%s() {",
                                 itf_name.MakeProxyName(true).c_str(),
                                 itf_name.MakeProxyName(false).c_str()));
     text->PushOffset(kBlockOffset);
@@ -1044,11 +1065,12 @@ void ProxyGenerator::ObjectManager::AddInterfaceAccessors(
   }
 
   // GetInstances().
-  text->AddLine(StringPrintf("std::vector<%s*> Get%sInstances() const {",
-                              itf_name.MakeProxyName(true).c_str(),
-                              itf_name.type_name.c_str()));
+  text->AddLine(
+      StringPrintf("std::vector<%sInterface*> Get%sInstances() const {",
+                   itf_name.MakeProxyName(true).c_str(),
+                   itf_name.type_name.c_str()));
   text->PushOffset(kBlockOffset);
-  text->AddLine(StringPrintf("std::vector<%s*> values;",
+  text->AddLine(StringPrintf("std::vector<%sInterface*> values;",
                              itf_name.MakeProxyName(true).c_str()));
   text->AddLine(StringPrintf("values.reserve(%s.size());", map_name.c_str()));
   text->AddLine(StringPrintf("for (const auto& pair : %s)", map_name.c_str()));
@@ -1062,18 +1084,18 @@ void ProxyGenerator::ObjectManager::AddInterfaceAccessors(
                               itf_name.type_name.c_str()));
   text->PushOffset(kLineContinuationOffset);
   text->AddLine(
-      StringPrintf("const base::Callback<void(%s*)>& callback) {",
-                    itf_name.MakeProxyName(true).c_str()));
+      StringPrintf("const base::Callback<void(%sInterface*)>& callback) {",
+                   itf_name.MakeProxyName(true).c_str()));
   text->PopOffset();
   text->PushOffset(kBlockOffset);
   text->AddLine(StringPrintf("on_%s_added_ = callback;",
-                              itf_name.MakeVariableName().c_str()));
+                             itf_name.MakeVariableName().c_str()));
   text->PopOffset();
   text->AddLine("}");
 
   // SetRemovedCallback().
   text->AddLine(StringPrintf("void Set%sRemovedCallback(",
-                              itf_name.type_name.c_str()));
+                             itf_name.type_name.c_str()));
   text->PushOffset(kLineContinuationOffset);
   text->AddLine("const base::Callback<void(const dbus::ObjectPath&)>& "
                 "callback) {");
@@ -1284,9 +1306,10 @@ void ProxyGenerator::ObjectManager::AddDataMembers(
                                itf_name.MakeProxyName(true).c_str(),
                                var_name.c_str()));
     text->PopOffset();
-    text->AddLine(StringPrintf("base::Callback<void(%s*)> on_%s_added_;",
-                               itf_name.MakeProxyName(true).c_str(),
-                               var_name.c_str()));
+    text->AddLine(
+        StringPrintf("base::Callback<void(%sInterface*)> on_%s_added_;",
+                     itf_name.MakeProxyName(true).c_str(),
+                     var_name.c_str()));
     text->AddLine(StringPrintf("base::Callback<void(const dbus::ObjectPath&)> "
                                "on_%s_removed_;",
                                var_name.c_str()));
diff --git a/chromeos-dbus-bindings/proxy_generator_unittest.cc b/chromeos-dbus-bindings/proxy_generator_unittest.cc
index ccbd1fe..5648bc9 100644
--- a/chromeos-dbus-bindings/proxy_generator_unittest.cc
+++ b/chromeos-dbus-bindings/proxy_generator_unittest.cc
@@ -117,6 +117,8 @@ class TestInterfaceProxyInterface {
       const base::Callback<void(const std::vector<std::string>&,
                                 uint8_t)>& signal_callback,
       dbus::ObjectProxy::OnConnectedCallback on_connected_callback) = 0;
+
+  virtual const dbus::ObjectPath& GetObjectPath() const = 0;
 };
 
 }  // namespace chromium
@@ -167,7 +169,7 @@ class TestInterfaceProxy final : public TestInterfaceProxyInterface {
     bus_->RemoveObjectProxy(service_name_, object_path_, callback);
   }
 
-  const dbus::ObjectPath& GetObjectPath() const {
+  const dbus::ObjectPath& GetObjectPath() const override {
     return object_path_;
   }
 
@@ -325,6 +327,8 @@ class TestInterface2ProxyInterface {
       const base::Callback<void(const std::string& /*name*/, int32_t /*age*/)>& success_callback,
       const base::Callback<void(brillo::Error*)>& error_callback,
       int timeout_ms = dbus::ObjectProxy::TIMEOUT_USE_DEFAULT) = 0;
+
+  virtual const dbus::ObjectPath& GetObjectPath() const = 0;
 };
 
 }  // namespace chromium
@@ -354,7 +358,7 @@ class TestInterface2Proxy final : public TestInterface2ProxyInterface {
     bus_->RemoveObjectProxy(service_name_, object_path_, callback);
   }
 
-  const dbus::ObjectPath& GetObjectPath() const {
+  const dbus::ObjectPath& GetObjectPath() const override {
     return object_path_;
   }
 
@@ -434,6 +438,8 @@ class TestInterfaceProxyInterface {
   virtual void RegisterCloserSignalHandler(
       const base::Closure& signal_callback,
       dbus::ObjectProxy::OnConnectedCallback on_connected_callback) = 0;
+
+  virtual const dbus::ObjectPath& GetObjectPath() const = 0;
 };
 
 }  // namespace chromium
@@ -469,7 +475,7 @@ class TestInterfaceProxy final : public TestInterfaceProxyInterface {
     bus_->RemoveObjectProxy(service_name_, object_path_, callback);
   }
 
-  const dbus::ObjectPath& GetObjectPath() const {
+  const dbus::ObjectPath& GetObjectPath() const override {
     return object_path_;
   }
 
@@ -494,6 +500,8 @@ namespace chromium {
 class TestInterface2ProxyInterface {
  public:
   virtual ~TestInterface2ProxyInterface() = default;
+
+  virtual const dbus::ObjectPath& GetObjectPath() const = 0;
 };
 
 }  // namespace chromium
@@ -521,7 +529,7 @@ class TestInterface2Proxy final : public TestInterface2ProxyInterface {
     bus_->RemoveObjectProxy(service_name_, object_path_, callback);
   }
 
-  const dbus::ObjectPath& GetObjectPath() const {
+  const dbus::ObjectPath& GetObjectPath() const override {
     return object_path_;
   }
 
@@ -582,6 +590,10 @@ class Itf1ProxyInterface {
 
   static const char* DataName() { return "Data"; }
   virtual const std::string& data() const = 0;
+  virtual void set_data(const std::string& value,
+                        const base::Callback<void(bool)>& callback) = 0;
+
+  virtual const dbus::ObjectPath& GetObjectPath() const = 0;
 };
 
 }  // namespace chromium
@@ -638,14 +650,14 @@ class Itf1Proxy final : public Itf1ProxyInterface {
     bus_->RemoveObjectProxy(service_name_, object_path_, callback);
   }
 
-  const dbus::ObjectPath& GetObjectPath() const {
+  const dbus::ObjectPath& GetObjectPath() const override {
     return object_path_;
   }
 
   dbus::ObjectProxy* GetObjectProxy() const { return dbus_object_proxy_; }
 
   void SetPropertyChangedCallback(
-      const base::Callback<void(Itf1Proxy*, const std::string&)>& callback) {
+      const base::Callback<void(Itf1ProxyInterface*, const std::string&)>& callback) {
     on_property_changed_ = callback;
   }
 
@@ -656,6 +668,11 @@ class Itf1Proxy final : public Itf1ProxyInterface {
     return property_set_->data.value();
   }
 
+  void set_data(const std::string& value,
+                const base::Callback<void(bool)>& callback) override {
+    property_set_->data.Set(value, callback);
+  }
+
  private:
   void OnPropertyChanged(const std::string& property_name) {
     if (!on_property_changed_.is_null())
@@ -666,7 +683,7 @@ class Itf1Proxy final : public Itf1ProxyInterface {
   std::string service_name_;
   const dbus::ObjectPath object_path_{"/org/chromium/Test/Object"};
   PropertySet* property_set_;
-  base::Callback<void(Itf1Proxy*, const std::string&)> on_property_changed_;
+  base::Callback<void(Itf1ProxyInterface*, const std::string&)> on_property_changed_;
   dbus::ObjectProxy* dbus_object_proxy_;
 
   friend class org::chromium::ObjectManagerProxy;
@@ -683,6 +700,8 @@ namespace chromium {
 class Itf2ProxyInterface {
  public:
   virtual ~Itf2ProxyInterface() = default;
+
+  virtual const dbus::ObjectPath& GetObjectPath() const = 0;
 };
 
 }  // namespace chromium
@@ -726,7 +745,7 @@ class Itf2Proxy final : public Itf2ProxyInterface {
     bus_->RemoveObjectProxy(service_name_, object_path_, callback);
   }
 
-  const dbus::ObjectPath& GetObjectPath() const {
+  const dbus::ObjectPath& GetObjectPath() const override {
     return object_path_;
   }
 
@@ -769,20 +788,20 @@ class ObjectManagerProxy : public dbus::ObjectManager::Interface {
     return dbus_object_manager_;
   }
 
-  org::chromium::Itf1Proxy* GetItf1Proxy() {
+  org::chromium::Itf1ProxyInterface* GetItf1Proxy() {
     if (itf1_instances_.empty())
       return nullptr;
     return itf1_instances_.begin()->second.get();
   }
-  std::vector<org::chromium::Itf1Proxy*> GetItf1Instances() const {
-    std::vector<org::chromium::Itf1Proxy*> values;
+  std::vector<org::chromium::Itf1ProxyInterface*> GetItf1Instances() const {
+    std::vector<org::chromium::Itf1ProxyInterface*> values;
     values.reserve(itf1_instances_.size());
     for (const auto& pair : itf1_instances_)
       values.push_back(pair.second.get());
     return values;
   }
   void SetItf1AddedCallback(
-      const base::Callback<void(org::chromium::Itf1Proxy*)>& callback) {
+      const base::Callback<void(org::chromium::Itf1ProxyInterface*)>& callback) {
     on_itf1_added_ = callback;
   }
   void SetItf1RemovedCallback(
@@ -790,22 +809,22 @@ class ObjectManagerProxy : public dbus::ObjectManager::Interface {
     on_itf1_removed_ = callback;
   }
 
-  org::chromium::Itf2Proxy* GetItf2Proxy(
+  org::chromium::Itf2ProxyInterface* GetItf2Proxy(
       const dbus::ObjectPath& object_path) {
     auto p = itf2_instances_.find(object_path);
     if (p != itf2_instances_.end())
       return p->second.get();
     return nullptr;
   }
-  std::vector<org::chromium::Itf2Proxy*> GetItf2Instances() const {
-    std::vector<org::chromium::Itf2Proxy*> values;
+  std::vector<org::chromium::Itf2ProxyInterface*> GetItf2Instances() const {
+    std::vector<org::chromium::Itf2ProxyInterface*> values;
     values.reserve(itf2_instances_.size());
     for (const auto& pair : itf2_instances_)
       values.push_back(pair.second.get());
     return values;
   }
   void SetItf2AddedCallback(
-      const base::Callback<void(org::chromium::Itf2Proxy*)>& callback) {
+      const base::Callback<void(org::chromium::Itf2ProxyInterface*)>& callback) {
     on_itf2_added_ = callback;
   }
   void SetItf2RemovedCallback(
@@ -907,11 +926,11 @@ class ObjectManagerProxy : public dbus::ObjectManager::Interface {
   dbus::ObjectManager* dbus_object_manager_;
   std::map<dbus::ObjectPath,
            std::unique_ptr<org::chromium::Itf1Proxy>> itf1_instances_;
-  base::Callback<void(org::chromium::Itf1Proxy*)> on_itf1_added_;
+  base::Callback<void(org::chromium::Itf1ProxyInterface*)> on_itf1_added_;
   base::Callback<void(const dbus::ObjectPath&)> on_itf1_removed_;
   std::map<dbus::ObjectPath,
            std::unique_ptr<org::chromium::Itf2Proxy>> itf2_instances_;
-  base::Callback<void(org::chromium::Itf2Proxy*)> on_itf2_added_;
+  base::Callback<void(org::chromium::Itf2ProxyInterface*)> on_itf2_added_;
   base::Callback<void(const dbus::ObjectPath&)> on_itf2_removed_;
   base::WeakPtrFactory<ObjectManagerProxy> weak_ptr_factory_{this};
 
@@ -961,6 +980,8 @@ class Itf1ProxyInterface {
   virtual void RegisterCloserSignalHandler(
       const base::Closure& signal_callback,
       dbus::ObjectProxy::OnConnectedCallback on_connected_callback) = 0;
+
+  virtual const dbus::ObjectPath& GetObjectPath() const = 0;
 };
 
 }  // namespace chromium
@@ -1010,7 +1031,7 @@ class Itf1Proxy final : public Itf1ProxyInterface {
     bus_->RemoveObjectProxy(service_name_, object_path_, callback);
   }
 
-  const dbus::ObjectPath& GetObjectPath() const {
+  const dbus::ObjectPath& GetObjectPath() const override {
     return object_path_;
   }
 
@@ -1035,6 +1056,8 @@ namespace chromium {
 class Itf2ProxyInterface {
  public:
   virtual ~Itf2ProxyInterface() = default;
+
+  virtual const dbus::ObjectPath& GetObjectPath() const = 0;
 };
 
 }  // namespace chromium
@@ -1076,7 +1099,7 @@ class Itf2Proxy final : public Itf2ProxyInterface {
     bus_->RemoveObjectProxy(service_name_, object_path_, callback);
   }
 
-  const dbus::ObjectPath& GetObjectPath() const {
+  const dbus::ObjectPath& GetObjectPath() const override {
     return object_path_;
   }
 
@@ -1117,20 +1140,20 @@ class ObjectManagerProxy : public dbus::ObjectManager::Interface {
     return dbus_object_manager_;
   }
 
-  org::chromium::Itf1Proxy* GetItf1Proxy() {
+  org::chromium::Itf1ProxyInterface* GetItf1Proxy() {
     if (itf1_instances_.empty())
       return nullptr;
     return itf1_instances_.begin()->second.get();
   }
-  std::vector<org::chromium::Itf1Proxy*> GetItf1Instances() const {
-    std::vector<org::chromium::Itf1Proxy*> values;
+  std::vector<org::chromium::Itf1ProxyInterface*> GetItf1Instances() const {
+    std::vector<org::chromium::Itf1ProxyInterface*> values;
     values.reserve(itf1_instances_.size());
     for (const auto& pair : itf1_instances_)
       values.push_back(pair.second.get());
     return values;
   }
   void SetItf1AddedCallback(
-      const base::Callback<void(org::chromium::Itf1Proxy*)>& callback) {
+      const base::Callback<void(org::chromium::Itf1ProxyInterface*)>& callback) {
     on_itf1_added_ = callback;
   }
   void SetItf1RemovedCallback(
@@ -1138,22 +1161,22 @@ class ObjectManagerProxy : public dbus::ObjectManager::Interface {
     on_itf1_removed_ = callback;
   }
 
-  org::chromium::Itf2Proxy* GetItf2Proxy(
+  org::chromium::Itf2ProxyInterface* GetItf2Proxy(
       const dbus::ObjectPath& object_path) {
     auto p = itf2_instances_.find(object_path);
     if (p != itf2_instances_.end())
       return p->second.get();
     return nullptr;
   }
-  std::vector<org::chromium::Itf2Proxy*> GetItf2Instances() const {
-    std::vector<org::chromium::Itf2Proxy*> values;
+  std::vector<org::chromium::Itf2ProxyInterface*> GetItf2Instances() const {
+    std::vector<org::chromium::Itf2ProxyInterface*> values;
     values.reserve(itf2_instances_.size());
     for (const auto& pair : itf2_instances_)
       values.push_back(pair.second.get());
     return values;
   }
   void SetItf2AddedCallback(
-      const base::Callback<void(org::chromium::Itf2Proxy*)>& callback) {
+      const base::Callback<void(org::chromium::Itf2ProxyInterface*)>& callback) {
     on_itf2_added_ = callback;
   }
   void SetItf2RemovedCallback(
@@ -1244,11 +1267,11 @@ class ObjectManagerProxy : public dbus::ObjectManager::Interface {
   dbus::ObjectManager* dbus_object_manager_;
   std::map<dbus::ObjectPath,
            std::unique_ptr<org::chromium::Itf1Proxy>> itf1_instances_;
-  base::Callback<void(org::chromium::Itf1Proxy*)> on_itf1_added_;
+  base::Callback<void(org::chromium::Itf1ProxyInterface*)> on_itf1_added_;
   base::Callback<void(const dbus::ObjectPath&)> on_itf1_removed_;
   std::map<dbus::ObjectPath,
            std::unique_ptr<org::chromium::Itf2Proxy>> itf2_instances_;
-  base::Callback<void(org::chromium::Itf2Proxy*)> on_itf2_added_;
+  base::Callback<void(org::chromium::Itf2ProxyInterface*)> on_itf2_added_;
   base::Callback<void(const dbus::ObjectPath&)> on_itf2_removed_;
   base::WeakPtrFactory<ObjectManagerProxy> weak_ptr_factory_{this};