chromeos-dbus-bindings: Expose Register*Handler() methods in the *ProxyInterface.

The generated proxy is split in two: an abstract interface
(class *ProxyInterface) and the actual implementation that sends the
dbus calls (class *Proxy). For signals, we can register a signal
handler using the Register*Handler() method in the Proxy class, but it
is not available in the ProxyInterface.

This patch defines the Register*Handler() methods in the abstract
interface and includes the corresponding method in the ProxyMock class
so the unittests can check that the signals are handled and the code
under test can receive a ProxyInterface instance and register the
handler with it.

BUG=None
TEST=Unittests updated.

Change-Id: Ife2f91591e87e8c8c3edda76db32c6327b145a81
Reviewed-on: https://chromium-review.googlesource.com/289330
Commit-Queue: Alex Deymo <deymo@chromium.org>
Trybot-Ready: Alex Deymo <deymo@chromium.org>
Tested-by: Alex Deymo <deymo@chromium.org>
Reviewed-by: Alex Vakulenko <avakulenko@chromium.org>

7 files changed, 157 insertions, 46 deletions

diff --git a/chromeos-dbus-bindings/indented_text.cc b/chromeos-dbus-bindings/indented_text.cc
index 1feb265..8d42091 100644
--- a/chromeos-dbus-bindings/indented_text.cc
+++ b/chromeos-dbus-bindings/indented_text.cc
@@ -102,13 +102,22 @@ void IndentedText::AddComments(const std::string& doc_string) {
 
 string IndentedText::GetContents() const {
   string output;
+  for (const string& line : GetLines()) {
+    output.append(line);
+    output.append("\n");
+  }
+  return output;
+}
+
+std::vector<std::string> IndentedText::GetLines() const {
+  vector<string> result;
   for (const auto& member : contents_) {
     const string& line = member.first;
     size_t shift = line.empty() ? 0 : member.second;
     string indent(shift, ' ');
-    output.append(indent + line + "\n");
+    result.push_back(indent + line);
   }
-  return output;
+  return result;
 }
 
 void IndentedText::PushOffset(size_t shift) {
diff --git a/chromeos-dbus-bindings/indented_text.h b/chromeos-dbus-bindings/indented_text.h
index 42fd744..6651a58 100644
--- a/chromeos-dbus-bindings/indented_text.h
+++ b/chromeos-dbus-bindings/indented_text.h
@@ -41,6 +41,10 @@ class IndentedText {
   // Return a string representing the indented text.
   std::string GetContents() const;
 
+  // Return a list of lines representing the intended indented text, not
+  // including the \n.
+  std::vector<std::string> GetLines() const;
+
   // Add or remove an offset to the current stack of indentation offsets.
   void PushOffset(size_t shift);
   void PopOffset();
diff --git a/chromeos-dbus-bindings/indented_text_unittest.cc b/chromeos-dbus-bindings/indented_text_unittest.cc
index 1b0d9d1..a75c396 100644
--- a/chromeos-dbus-bindings/indented_text_unittest.cc
+++ b/chromeos-dbus-bindings/indented_text_unittest.cc
@@ -171,4 +171,13 @@ TEST_F(IndentedTextTest, AddComments_Indentation) {
             "// line3\n", text_.GetContents());
 }
 
+TEST_F(IndentedTextTest, GetLines) {
+  text_.AddLine("no indent");
+  text_.PushOffset(2);
+  text_.AddLine("2 spaces");
+  text_.AddLine("");
+
+  EXPECT_EQ((vector<string>{"no indent", "  2 spaces", ""}), text_.GetLines());
+}
+
 }  // namespace chromeos_dbus_bindings
diff --git a/chromeos-dbus-bindings/proxy_generator.cc b/chromeos-dbus-bindings/proxy_generator.cc
index 24b61af..8202ab3 100644
--- a/chromeos-dbus-bindings/proxy_generator.cc
+++ b/chromeos-dbus-bindings/proxy_generator.cc
@@ -196,6 +196,9 @@ void ProxyGenerator::GenerateInterfaceProxyInterface(
     AddMethodProxy(method, interface.name, true, text);
     AddAsyncMethodProxy(method, interface.name, true, text);
   }
+  for (const auto& signal : interface.signals) {
+    AddSignalHandlerRegistration(signal, interface.name, true, text);
+  }
   AddProperties(config, interface, true, text);
 
   text->PopOffset();
@@ -231,7 +234,9 @@ void ProxyGenerator::GenerateInterfaceProxy(const ServiceConfig& config,
   AddPropertySet(config, interface, text);
   AddConstructor(config, interface, proxy_name, text);
   AddDestructor(proxy_name, text);
-  AddSignalHandlerRegistration(interface, text);
+  for (const auto& signal : interface.signals) {
+    AddSignalHandlerRegistration(signal, interface.name, false, text);
+  }
   AddReleaseObjectProxy(text);
   AddGetObjectPath(text);
   AddGetObjectProxy(text);
@@ -313,6 +318,9 @@ void ProxyGenerator::GenerateInterfaceMock(const ServiceConfig& config,
     AddMethodMock(method, interface.name, text);
     AddAsyncMethodMock(method, interface.name, text);
   }
+  for (const auto& signal : interface.signals) {
+    AddSignalHandlerRegistrationMock(signal, text);
+  }
 
   DbusSignature signature;
   for (const auto& prop : interface.properties) {
@@ -444,56 +452,35 @@ void ProxyGenerator::AddOnPropertyChanged(IndentedText* text) {
   text->AddBlankLine();
 }
 
-void ProxyGenerator::AddSignalHandlerRegistration(const Interface& interface,
-                                                  IndentedText* text) {
+void ProxyGenerator::AddSignalHandlerRegistration(
+      const Interface::Signal& signal,
+      const string& interface_name,
+      bool declaration_only,
+      IndentedText* text) {
   IndentedText block;
-  DbusSignature signature;
-  for (const auto& signal : interface.signals) {
-    block.AddLine(
-        StringPrintf("void Register%sSignalHandler(", signal.name.c_str()));
-    block.PushOffset(kLineContinuationOffset);
-    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);
-      }
-      block.AddLine(StringPrintf("%s%s)>& signal_callback,",
-                                 prefix.c_str(),
-                                 last_argument.c_str()));
-      if (prefix.empty()) {
-        block.PopOffset();
-      }
-    }
-    block.AddLine("dbus::ObjectProxy::OnConnectedCallback "
-                  "on_connected_callback) {");
+  block.AddLine(StringPrintf("%svoid Register%sSignalHandler(",
+                             declaration_only ? "virtual " : "",
+                             signal.name.c_str()));
+  block.PushOffset(kLineContinuationOffset);
+  AddSignalCallbackArg(signal, false, &block);
+  block.AddLine(StringPrintf(
+      "dbus::ObjectProxy::OnConnectedCallback on_connected_callback)%s",
+      declaration_only ? " = 0;" : " override {"));
+  if (!declaration_only) {
     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\",", 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.AddBlankLine();
   text->AddBlock(block);
 }
 
@@ -808,6 +795,68 @@ void ProxyGenerator::AddAsyncMethodMock(const Interface::Method& method,
 }
 
 // static
+void ProxyGenerator::AddSignalHandlerRegistrationMock(
+    const Interface::Signal& signal,
+    IndentedText* text) {
+  IndentedText callback_arg_text;
+  AddSignalCallbackArg(signal, true, &callback_arg_text);
+  vector<string> arg_lines = callback_arg_text.GetLines();
+
+  IndentedText block;
+  block.AddLineAndPushOffsetTo(
+      StringPrintf("MOCK_METHOD2(Register%sSignalHandler,",
+                   signal.name.c_str()),
+      1, '(');
+  for (size_t i = 0; i < arg_lines.size(); ++i) {
+    if (i == 0)
+      block.AddLineAndPushOffsetTo("void(" + arg_lines[i], 1, '(');
+    else
+      block.AddLine(arg_lines[i]);
+  }
+  block.AddLine(
+      "dbus::ObjectProxy::OnConnectedCallback /*on_connected_callback*/));");
+  text->AddBlock(block);
+}
+
+// static
+void ProxyGenerator::AddSignalCallbackArg(const Interface::Signal& signal,
+                                          bool comment_arg_name,
+                                          IndentedText* block) {
+  DbusSignature signature;
+  string signal_callback = StringPrintf("%ssignal_callback%s",
+                                        comment_arg_name ? "/*" : "",
+                                        comment_arg_name ? "*/" : "");
+  if (signal.arguments.empty()) {
+    block->AddLine(StringPrintf("const base::Closure& %s,",
+                                signal_callback.c_str()));
+  } 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);
+    }
+    block->AddLine(StringPrintf("%s%s)>& %s,",
+                                prefix.c_str(),
+                                last_argument.c_str(),
+                                signal_callback.c_str()));
+    if (prefix.empty()) {
+      block->PopOffset();
+    }
+  }
+}
+
+// static
 void ProxyGenerator::ObjectManager::GenerateProxy(
     const ServiceConfig& config,
     const std::vector<Interface>& interfaces,
diff --git a/chromeos-dbus-bindings/proxy_generator.h b/chromeos-dbus-bindings/proxy_generator.h
index 5dc04c9..ba9a224 100644
--- a/chromeos-dbus-bindings/proxy_generator.h
+++ b/chromeos-dbus-bindings/proxy_generator.h
@@ -80,7 +80,9 @@ class ProxyGenerator : public HeaderGenerator {
   static void AddOnPropertyChanged(IndentedText* text);
 
   // Generates logic permitting users to register handlers for signals.
-  static void AddSignalHandlerRegistration(const Interface& interface,
+  static void AddSignalHandlerRegistration(const Interface::Signal& signal,
+                                           const std::string& interface_name,
+                                           bool declaration_only,
                                            IndentedText* text);
 
   // Generates the property set class to contain interface properties.
@@ -116,6 +118,16 @@ class ProxyGenerator : public HeaderGenerator {
                                  const std::string& interface_name,
                                  IndentedText* text);
 
+  // Generates a mock for the signal handler registration method.
+  static void AddSignalHandlerRegistrationMock(
+      const Interface::Signal& signal,
+      IndentedText* text);
+
+  // Generate the signal callback argument of a signal handler.
+  static void AddSignalCallbackArg(const Interface::Signal& signal,
+                                   bool comment_arg_name,
+                                   IndentedText* block);
+
   // Generates the Object Manager proxy class.
   struct ObjectManager {
     // Generates the top-level class for Object Manager proxy.
diff --git a/chromeos-dbus-bindings/proxy_generator_mock_unittest.cc b/chromeos-dbus-bindings/proxy_generator_mock_unittest.cc
index 0f1eb2c..a07a8c9 100644
--- a/chromeos-dbus-bindings/proxy_generator_mock_unittest.cc
+++ b/chromeos-dbus-bindings/proxy_generator_mock_unittest.cc
@@ -88,6 +88,13 @@ class TestInterfaceProxyMock final : public TestInterfaceProxyInterface {
                void(const base::Callback<void()>& /*success_callback*/,
                     const base::Callback<void(chromeos::Error*)>& /*error_callback*/,
                     int /*timeout_ms*/));
+  MOCK_METHOD2(RegisterCloserSignalHandler,
+               void(const base::Closure& /*signal_callback*/,
+                    dbus::ObjectProxy::OnConnectedCallback /*on_connected_callback*/));
+  MOCK_METHOD2(RegisterTheCurseOfKaZarSignalHandler,
+               void(const base::Callback<void(const std::vector<std::string>&,
+                                              uint8_t)>& /*signal_callback*/,
+                    dbus::ObjectProxy::OnConnectedCallback /*on_connected_callback*/));
 
  private:
   DISALLOW_COPY_AND_ASSIGN(TestInterfaceProxyMock);
diff --git a/chromeos-dbus-bindings/proxy_generator_unittest.cc b/chromeos-dbus-bindings/proxy_generator_unittest.cc
index 8840f41..7ce1c3e 100644
--- a/chromeos-dbus-bindings/proxy_generator_unittest.cc
+++ b/chromeos-dbus-bindings/proxy_generator_unittest.cc
@@ -107,6 +107,15 @@ class TestInterfaceProxyInterface {
       const base::Callback<void(chromeos::Error*)>& error_callback,
       int timeout_ms = dbus::ObjectProxy::TIMEOUT_USE_DEFAULT) = 0;
 
+  virtual void RegisterCloserSignalHandler(
+      const base::Closure& signal_callback,
+      dbus::ObjectProxy::OnConnectedCallback on_connected_callback) = 0;
+
+  virtual void RegisterTheCurseOfKaZarSignalHandler(
+      const base::Callback<void(const std::vector<std::string>&,
+                                uint8_t)>& signal_callback,
+      dbus::ObjectProxy::OnConnectedCallback on_connected_callback) = 0;
+
  protected:
   virtual ~TestInterfaceProxyInterface() = default;
 };
@@ -134,7 +143,7 @@ class TestInterfaceProxy final : public TestInterfaceProxyInterface {
 
   void RegisterCloserSignalHandler(
       const base::Closure& signal_callback,
-      dbus::ObjectProxy::OnConnectedCallback on_connected_callback) {
+      dbus::ObjectProxy::OnConnectedCallback on_connected_callback) override {
     chromeos::dbus_utils::ConnectToSignal(
         dbus_object_proxy_,
         "org.chromium.TestInterface",
@@ -146,7 +155,7 @@ class TestInterfaceProxy final : public TestInterfaceProxyInterface {
   void RegisterTheCurseOfKaZarSignalHandler(
       const base::Callback<void(const std::vector<std::string>&,
                                 uint8_t)>& signal_callback,
-      dbus::ObjectProxy::OnConnectedCallback on_connected_callback) {
+      dbus::ObjectProxy::OnConnectedCallback on_connected_callback) override {
     chromeos::dbus_utils::ConnectToSignal(
         dbus_object_proxy_,
         "org.chromium.TestInterface",
@@ -422,6 +431,10 @@ namespace chromium {
 // Abstract interface proxy for org::chromium::TestInterface.
 class TestInterfaceProxyInterface {
  public:
+  virtual void RegisterCloserSignalHandler(
+      const base::Closure& signal_callback,
+      dbus::ObjectProxy::OnConnectedCallback on_connected_callback) = 0;
+
  protected:
   virtual ~TestInterfaceProxyInterface() = default;
 };
@@ -446,7 +459,7 @@ class TestInterfaceProxy final : public TestInterfaceProxyInterface {
 
   void RegisterCloserSignalHandler(
       const base::Closure& signal_callback,
-      dbus::ObjectProxy::OnConnectedCallback on_connected_callback) {
+      dbus::ObjectProxy::OnConnectedCallback on_connected_callback) override {
     chromeos::dbus_utils::ConnectToSignal(
         dbus_object_proxy_,
         "org.chromium.TestInterface",
@@ -565,6 +578,10 @@ namespace chromium {
 // Abstract interface proxy for org::chromium::Itf1.
 class Itf1ProxyInterface {
  public:
+  virtual void RegisterCloserSignalHandler(
+      const base::Closure& signal_callback,
+      dbus::ObjectProxy::OnConnectedCallback on_connected_callback) = 0;
+
   static const char* DataName() { return "Data"; }
   virtual const std::string& data() const = 0;
 
@@ -613,7 +630,7 @@ class Itf1Proxy final : public Itf1ProxyInterface {
 
   void RegisterCloserSignalHandler(
       const base::Closure& signal_callback,
-      dbus::ObjectProxy::OnConnectedCallback on_connected_callback) {
+      dbus::ObjectProxy::OnConnectedCallback on_connected_callback) override {
     chromeos::dbus_utils::ConnectToSignal(
         dbus_object_proxy_,
         "org.chromium.Itf1",
@@ -945,6 +962,10 @@ namespace chromium {
 // Abstract interface proxy for org::chromium::Itf1.
 class Itf1ProxyInterface {
  public:
+  virtual void RegisterCloserSignalHandler(
+      const base::Closure& signal_callback,
+      dbus::ObjectProxy::OnConnectedCallback on_connected_callback) = 0;
+
  protected:
   virtual ~Itf1ProxyInterface() = default;
 };
@@ -983,7 +1004,7 @@ class Itf1Proxy final : public Itf1ProxyInterface {
 
   void RegisterCloserSignalHandler(
       const base::Closure& signal_callback,
-      dbus::ObjectProxy::OnConnectedCallback on_connected_callback) {
+      dbus::ObjectProxy::OnConnectedCallback on_connected_callback) override {
     chromeos::dbus_utils::ConnectToSignal(
         dbus_object_proxy_,
         "org.chromium.Itf1",