chromeos-dbus-bindings: Use per signal handlers

When we use a delegate, we're forced into a situation where:

1) Some object must manage the lifetime of both the delegate and the
   proxy
2) We can continue to receive callbacks on the signal handler until
   we detach the proxy from the Bus

However, there is not a great mechanism to coordinate the lifetime
of the managing object with the required lifetime of the delegate
object.  If we ever reach the destructor of the managing object,
it is too late to try and tear down the proxy and its signal delegate.

BUG=chromium:438728
TEST=unittests and generated code compiles.

Change-Id: Ia6f8080b069f4aa6d1b92b1ca612e3d69eee88a7
Reviewed-on: https://chromium-review.googlesource.com/233009
Reviewed-by: Christopher Wiley <wiley@chromium.org>
Tested-by: Christopher Wiley <wiley@chromium.org>
Commit-Queue: Christopher Wiley <wiley@chromium.org>

4 files changed, 87 insertions, 244 deletions

diff --git a/chromeos-dbus-bindings/generate_chromeos_dbus_bindings.cc b/chromeos-dbus-bindings/generate_chromeos_dbus_bindings.cc
index 4189bbf..62ba036 100644
--- a/chromeos-dbus-bindings/generate_chromeos_dbus_bindings.cc
+++ b/chromeos-dbus-bindings/generate_chromeos_dbus_bindings.cc
@@ -156,7 +156,7 @@ int main(int argc, char** argv) {
       return 1;
     }
     if (!parser.ParseXmlInterfaceFile(contents, config.ignore_interfaces)) {
-      LOG(ERROR) << "Failed to parse interface file.";
+      LOG(ERROR) << "Failed to parse interface file " << input;
       return 1;
     }
   }
diff --git a/chromeos-dbus-bindings/proxy_generator.cc b/chromeos-dbus-bindings/proxy_generator.cc
index 96f2bc8..a379bca 100644
--- a/chromeos-dbus-bindings/proxy_generator.cc
+++ b/chromeos-dbus-bindings/proxy_generator.cc
@@ -111,17 +111,15 @@ void ProxyGenerator::GenerateInterfaceProxy(const ServiceConfig& config,
   text->AddLine(StringPrintf("class %s final {", proxy_name.c_str()));
   text->AddLineWithOffset("public:", kScopeOffset);
   text->PushOffset(kBlockOffset);
-  AddSignalReceiver(interface, text);
   AddPropertySet(config, interface, text);
   AddConstructor(config, interface, proxy_name, text);
   AddDestructor(proxy_name, text);
+  AddSignalHandlerRegistration(interface, text);
   AddReleaseObjectProxy(text);
   AddGetObjectPath(text);
   AddGetObjectProxy(text);
   if (!config.object_manager.name.empty() && !interface.properties.empty())
     AddPropertyPublicMethods(proxy_name, text);
-  if (!interface.signals.empty())
-    AddSignalConnectedCallback(text);
   for (const auto& method : interface.methods) {
     AddMethodProxy(method, interface.name, text);
   }
@@ -210,47 +208,6 @@ void ProxyGenerator::AddConstructor(const ServiceConfig& config,
   if (args.size() > 1)
     block.PopOffset();
   block.AddLine("}");
-  if (!interface.signals.empty()) {
-    block.AddBlankLine();
-    block.AddLine(StringPrintf("%s(", class_name.c_str()));
-    block.PushOffset(kLineContinuationOffset);
-    vector<string> param_names;
-    for (const auto& arg : args) {
-      block.AddLine(StringPrintf("%s,", GetParamString(arg).c_str()));
-      param_names.push_back(arg.name);
-    }
-    block.AddLine("SignalReceiver* signal_receiver) :");
-    string param_list = chromeos::string_utils::Join(", ", param_names);
-    block.PushOffset(kLineContinuationOffset);
-    block.AddLine(StringPrintf("%s(%s) {", class_name.c_str(),
-                               param_list.c_str()));
-    block.PopOffset();
-    block.PopOffset();
-    block.PushOffset(kBlockOffset);
-    for (const auto& signal : interface.signals) {
-      block.AddLine("chromeos::dbus_utils::ConnectToSignal(");
-      block.PushOffset(kLineContinuationOffset);
-      block.AddLine("dbus_object_proxy_,");
-      block.AddLine(StringPrintf("\"%s\",", interface.name.c_str()));
-      block.AddLine(StringPrintf("\"%s\",", signal.name.c_str()));
-      block.AddLine("base::Bind(");
-      block.PushOffset(kLineContinuationOffset);
-      block.AddLine(StringPrintf(
-        "&SignalReceiver::%s,",
-        GetHandlerNameForSignal(signal.name).c_str()));
-      block.AddLine("base::Unretained(signal_receiver)),");
-      block.PopOffset();
-      block.AddLine("base::Bind(");
-      block.PushOffset(kLineContinuationOffset);
-      block.AddLine(StringPrintf(
-        "&%s::OnDBusSignalConnected,", class_name.c_str()));
-      block.AddLine("base::Unretained(this)));");
-      block.PopOffset();
-      block.PopOffset();
-    }
-    block.PopOffset();
-    block.AddLine("}");
-  }
   block.AddBlankLine();
   text->AddBlock(block);
 }
@@ -321,75 +278,56 @@ void ProxyGenerator::AddOnPropertyChanged(IndentedText* text) {
   text->AddBlankLine();
 }
 
-// static
-void ProxyGenerator::AddSignalConnectedCallback(IndentedText* text) {
-  IndentedText block;
-  block.AddLine("void OnDBusSignalConnected(");
-  block.PushOffset(kLineContinuationOffset);
-  block.AddLine("const std::string& interface,");
-  block.AddLine("const std::string& signal,");
-  block.AddLine("bool success) {");
-  block.PopOffset();
-  block.PushOffset(kBlockOffset);
-  block.AddLine("if (!success) {");
-  block.PushOffset(kBlockOffset);
-  block.AddLine("LOG(ERROR)");
-  block.PushOffset(kLineContinuationOffset);
-  block.AddLine("<< \"Failed to connect to \" << interface << \".\" << signal");
-  block.AddLine("<< \" for \" << service_name_ << \" at \"");
-  block.AddLine("<< object_path_.value();");
-  block.PopOffset();
-  block.PopOffset();
-  block.AddLine("}");
-  block.PopOffset();
-  block.AddLine("}");
-  block.AddBlankLine();
-  text->AddBlock(block);
-}
-
-// static
-void ProxyGenerator::AddSignalReceiver(const Interface& interface,
-                                       IndentedText* text) {
-  if (interface.signals.empty())
-    return;
-
+void ProxyGenerator::AddSignalHandlerRegistration(const Interface& interface,
+                                                  IndentedText* text) {
   IndentedText block;
-  block.AddLine("class SignalReceiver {");
-  block.AddLineWithOffset("public:", kScopeOffset);
-  block.PushOffset(kBlockOffset);
   DbusSignature signature;
   for (const auto& signal : interface.signals) {
-    block.AddComments(signal.doc_string);
-    string signal_begin = StringPrintf(
-        "virtual void %s(", GetHandlerNameForSignal(signal.name).c_str());
-    string signal_end = ") {}";
-
-    if (signal.arguments.empty()) {
-      block.AddLine(signal_begin + signal_end);
-      continue;
-    }
-    block.AddLine(signal_begin);
+    block.AddLine(
+        StringPrintf("void Register%sSignalHandler(", signal.name.c_str()));
     block.PushOffset(kLineContinuationOffset);
-    string last_argument;
-    vector<string> argument_types;
-    for (const auto& argument : signal.arguments) {
-      if (!last_argument.empty()) {
-          block.AddLine(StringPrintf("%s,", last_argument.c_str()));
+    if (signal.arguments.empty()) {
+      block.AddLine("const base::Closure& signal_callback,");
+    } else {
+      string last_argument;
+      string prefix{"const base::Callback<void("};
+      for (const auto argument : signal.arguments) {
+        if (!last_argument.empty()) {
+          if (!prefix.empty()) {
+            block.AddLineAndPushOffsetTo(
+                StringPrintf("%s%s,", prefix.c_str(), last_argument.c_str()),
+                1, '(');
+            prefix.clear();
+          } else {
+            block.AddLine(StringPrintf("%s,", last_argument.c_str()));
+          }
+        }
+        CHECK(signature.Parse(argument.type, &last_argument));
+        MakeConstReferenceIfNeeded(&last_argument);
       }
-      CHECK(signature.Parse(argument.type, &last_argument));
-      MakeConstReferenceIfNeeded(&last_argument);
-      if (!argument.name.empty()) {
-        last_argument += ' ';
-        last_argument += argument.name;
+      block.AddLine(StringPrintf("%s%s)>& signal_callback,",
+                                 prefix.c_str(),
+                                 last_argument.c_str()));
+      if (prefix.empty()) {
+        block.PopOffset();
       }
     }
-    block.AddLine(last_argument + signal_end);
-    block.PopOffset();
+    block.AddLine("dbus::ObjectProxy::OnConnectedCallback "
+                  "on_connected_callback) {");
+    block.PopOffset();  // Method signature arguments
+    block.PushOffset(kBlockOffset);
+    block.AddLine("chromeos::dbus_utils::ConnectToSignal(");
+    block.PushOffset(kLineContinuationOffset);
+    block.AddLine("dbus_object_proxy_,");
+    block.AddLine(StringPrintf("\"%s\",", interface.name.c_str()));
+    block.AddLine(StringPrintf("\"%s\",", signal.name.c_str()));
+    block.AddLine("signal_callback,");
+    block.AddLine("on_connected_callback);");
+    block.PopOffset();  // Function call line continuation
+    block.PopOffset();  // Method body
+    block.AddLine("}");
+    block.AddBlankLine();
   }
-  block.PopOffset();
-  block.AddLine("};");
-  block.AddBlankLine();
-
   text->AddBlock(block);
 }
 
diff --git a/chromeos-dbus-bindings/proxy_generator.h b/chromeos-dbus-bindings/proxy_generator.h
index 7d4549b..f561714 100644
--- a/chromeos-dbus-bindings/proxy_generator.h
+++ b/chromeos-dbus-bindings/proxy_generator.h
@@ -47,9 +47,6 @@ class ProxyGenerator : public HeaderGenerator {
   static void AddDestructor(const std::string& class_name,
                             IndentedText* text);
 
-  // Generates a callback for signal receiver registration completion.
-  static void AddSignalConnectedCallback(IndentedText* text);
-
   // Generates ReleaseObjectProxy() method to release ownership
   // of the object proxy.
   static void AddReleaseObjectProxy(IndentedText* text);
@@ -67,9 +64,9 @@ class ProxyGenerator : public HeaderGenerator {
   // Generates OnPropertyChanged() method.
   static void AddOnPropertyChanged(IndentedText* text);
 
-  // Generates the method signatures for signal receivers.
-  static void AddSignalReceiver(const Interface& interface,
-                                IndentedText* text);
+  // Generates logic permitting users to register handlers for signals.
+  static void AddSignalHandlerRegistration(const Interface& interface,
+                                           IndentedText* text);
 
   // Generates the property set class to contain interface properties.
   static void AddPropertySet(const ServiceConfig& config,
diff --git a/chromeos-dbus-bindings/proxy_generator_unittest.cc b/chromeos-dbus-bindings/proxy_generator_unittest.cc
index d3ebaad..9afc9ab 100644
--- a/chromeos-dbus-bindings/proxy_generator_unittest.cc
+++ b/chromeos-dbus-bindings/proxy_generator_unittest.cc
@@ -72,14 +72,6 @@ namespace chromium {
 // Interface proxy for org::chromium::TestInterface.
 class TestInterfaceProxy final {
  public:
-  class SignalReceiver {
-   public:
-    virtual void OnCloserSignal() {}
-    virtual void OnTheCurseOfKaZarSignal(
-        const std::vector<std::string>&,
-        uint8_t) {}
-  };
-
   TestInterfaceProxy(
       const scoped_refptr<dbus::Bus>& bus,
       const std::string& service_name) :
@@ -89,34 +81,30 @@ class TestInterfaceProxy final {
               bus_->GetObjectProxy(service_name_, object_path_)} {
   }
 
-  TestInterfaceProxy(
-      const scoped_refptr<dbus::Bus>& bus,
-      const std::string& service_name,
-      SignalReceiver* signal_receiver) :
-          TestInterfaceProxy(bus, service_name) {
+  ~TestInterfaceProxy() {
+  }
+
+  void RegisterCloserSignalHandler(
+      const base::Closure& signal_callback,
+      dbus::ObjectProxy::OnConnectedCallback on_connected_callback) {
     chromeos::dbus_utils::ConnectToSignal(
         dbus_object_proxy_,
         "org.chromium.TestInterface",
         "Closer",
-        base::Bind(
-            &SignalReceiver::OnCloserSignal,
-            base::Unretained(signal_receiver)),
-        base::Bind(
-            &TestInterfaceProxy::OnDBusSignalConnected,
-            base::Unretained(this)));
+        signal_callback,
+        on_connected_callback);
+  }
+
+  void RegisterTheCurseOfKaZarSignalHandler(
+      const base::Callback<void(const std::vector<std::string>&,
+                                uint8_t)>& signal_callback,
+      dbus::ObjectProxy::OnConnectedCallback on_connected_callback) {
     chromeos::dbus_utils::ConnectToSignal(
         dbus_object_proxy_,
         "org.chromium.TestInterface",
         "TheCurseOfKaZar",
-        base::Bind(
-            &SignalReceiver::OnTheCurseOfKaZarSignal,
-            base::Unretained(signal_receiver)),
-        base::Bind(
-            &TestInterfaceProxy::OnDBusSignalConnected,
-            base::Unretained(this)));
-  }
-
-  ~TestInterfaceProxy() {
+        signal_callback,
+        on_connected_callback);
   }
 
   void ReleaseObjectProxy(const base::Closure& callback) {
@@ -129,18 +117,6 @@ class TestInterfaceProxy final {
 
   dbus::ObjectProxy* GetObjectProxy() const { return dbus_object_proxy_; }
 
-  void OnDBusSignalConnected(
-      const std::string& interface,
-      const std::string& signal,
-      bool success) {
-    if (!success) {
-      LOG(ERROR)
-          << "Failed to connect to " << interface << "." << signal
-          << " for " << service_name_ << " at "
-          << object_path_.value();
-    }
-  }
-
   bool Elements(
       const std::string& in_space_walk,
       const std::vector<dbus::ObjectPath>& in_ramblin_man,
@@ -291,34 +267,24 @@ namespace chromium {
 // Interface proxy for org::chromium::TestInterface.
 class TestInterfaceProxy final {
  public:
-  class SignalReceiver {
-   public:
-    virtual void OnCloserSignal() {}
-  };
-
   TestInterfaceProxy(const scoped_refptr<dbus::Bus>& bus) :
       bus_{bus},
       dbus_object_proxy_{
           bus_->GetObjectProxy(service_name_, object_path_)} {
   }
 
-  TestInterfaceProxy(
-      const scoped_refptr<dbus::Bus>& bus,
-      SignalReceiver* signal_receiver) :
-          TestInterfaceProxy(bus) {
+  ~TestInterfaceProxy() {
+  }
+
+  void RegisterCloserSignalHandler(
+      const base::Closure& signal_callback,
+      dbus::ObjectProxy::OnConnectedCallback on_connected_callback) {
     chromeos::dbus_utils::ConnectToSignal(
         dbus_object_proxy_,
         "org.chromium.TestInterface",
         "Closer",
-        base::Bind(
-            &SignalReceiver::OnCloserSignal,
-            base::Unretained(signal_receiver)),
-        base::Bind(
-            &TestInterfaceProxy::OnDBusSignalConnected,
-            base::Unretained(this)));
-  }
-
-  ~TestInterfaceProxy() {
+        signal_callback,
+        on_connected_callback);
   }
 
   void ReleaseObjectProxy(const base::Closure& callback) {
@@ -331,18 +297,6 @@ class TestInterfaceProxy final {
 
   dbus::ObjectProxy* GetObjectProxy() const { return dbus_object_proxy_; }
 
-  void OnDBusSignalConnected(
-      const std::string& interface,
-      const std::string& signal,
-      bool success) {
-    if (!success) {
-      LOG(ERROR)
-          << "Failed to connect to " << interface << "." << signal
-          << " for " << service_name_ << " at "
-          << object_path_.value();
-    }
-  }
-
  private:
   scoped_refptr<dbus::Bus> bus_;
   const std::string service_name_{"org.chromium.Test"};
@@ -430,11 +384,6 @@ namespace chromium {
 // Interface proxy for org::chromium::Itf1.
 class Itf1Proxy final {
  public:
-  class SignalReceiver {
-   public:
-    virtual void OnCloserSignal() {}
-  };
-
   class PropertySet : public dbus::PropertySet {
    public:
     PropertySet(dbus::ObjectProxy* object_proxy,
@@ -462,25 +411,18 @@ class Itf1Proxy final {
               bus_->GetObjectProxy(service_name_, object_path_)} {
   }
 
-  Itf1Proxy(
-      const scoped_refptr<dbus::Bus>& bus,
-      const std::string& service_name,
-      PropertySet* property_set,
-      SignalReceiver* signal_receiver) :
-          Itf1Proxy(bus, service_name, property_set) {
+  ~Itf1Proxy() {
+  }
+
+  void RegisterCloserSignalHandler(
+      const base::Closure& signal_callback,
+      dbus::ObjectProxy::OnConnectedCallback on_connected_callback) {
     chromeos::dbus_utils::ConnectToSignal(
         dbus_object_proxy_,
         "org.chromium.Itf1",
         "Closer",
-        base::Bind(
-            &SignalReceiver::OnCloserSignal,
-            base::Unretained(signal_receiver)),
-        base::Bind(
-            &Itf1Proxy::OnDBusSignalConnected,
-            base::Unretained(this)));
-  }
-
-  ~Itf1Proxy() {
+        signal_callback,
+        on_connected_callback);
   }
 
   void ReleaseObjectProxy(const base::Closure& callback) {
@@ -501,18 +443,6 @@ class Itf1Proxy final {
   const PropertySet* GetProperties() const { return property_set_; }
   PropertySet* GetProperties() { return property_set_; }
 
-  void OnDBusSignalConnected(
-      const std::string& interface,
-      const std::string& signal,
-      bool success) {
-    if (!success) {
-      LOG(ERROR)
-          << "Failed to connect to " << interface << "." << signal
-          << " for " << service_name_ << " at "
-          << object_path_.value();
-    }
-  }
-
   const std::string& data() const {
     return property_set_->data.value();
   }
@@ -800,11 +730,6 @@ namespace chromium {
 // Interface proxy for org::chromium::Itf1.
 class Itf1Proxy final {
  public:
-  class SignalReceiver {
-   public:
-    virtual void OnCloserSignal() {}
-  };
-
   class PropertySet : public dbus::PropertySet {
    public:
     PropertySet(dbus::ObjectProxy* object_proxy,
@@ -825,23 +750,18 @@ class Itf1Proxy final {
           bus_->GetObjectProxy(service_name_, object_path_)} {
   }
 
-  Itf1Proxy(
-      const scoped_refptr<dbus::Bus>& bus,
-      SignalReceiver* signal_receiver) :
-          Itf1Proxy(bus) {
+  ~Itf1Proxy() {
+  }
+
+  void RegisterCloserSignalHandler(
+      const base::Closure& signal_callback,
+      dbus::ObjectProxy::OnConnectedCallback on_connected_callback) {
     chromeos::dbus_utils::ConnectToSignal(
         dbus_object_proxy_,
         "org.chromium.Itf1",
         "Closer",
-        base::Bind(
-            &SignalReceiver::OnCloserSignal,
-            base::Unretained(signal_receiver)),
-        base::Bind(
-            &Itf1Proxy::OnDBusSignalConnected,
-            base::Unretained(this)));
-  }
-
-  ~Itf1Proxy() {
+        signal_callback,
+        on_connected_callback);
   }
 
   void ReleaseObjectProxy(const base::Closure& callback) {
@@ -854,18 +774,6 @@ class Itf1Proxy final {
 
   dbus::ObjectProxy* GetObjectProxy() const { return dbus_object_proxy_; }
 
-  void OnDBusSignalConnected(
-      const std::string& interface,
-      const std::string& signal,
-      bool success) {
-    if (!success) {
-      LOG(ERROR)
-          << "Failed to connect to " << interface << "." << signal
-          << " for " << service_name_ << " at "
-          << object_path_.value();
-    }
-  }
-
  private:
   scoped_refptr<dbus::Bus> bus_;
   const std::string service_name_{"org.chromium.Test"};