chromeos-dbus-bindings: Ignore multi-return methods

Multiple return arguments are legal in D-Bus.  Instead of throwing
a fatal error, ignore methods of this sort in the adaptor
generator, since libchromeos currently cannot automatically
generate returns for such methods.

While here, provide a protected getter to the DBus interface
pointer, so subclasses can register their own methods.

BUG=chromium:419271
TEST=Modified unit tests

Change-Id: Idd4ed43ee4cb8fa4249d666d85cadcb465c39cc1
Reviewed-on: https://chromium-review.googlesource.com/220750
Reviewed-by: Alex Deymo <deymo@chromium.org>
Reviewed-by: Alex Vakulenko <avakulenko@chromium.org>
Commit-Queue: Paul Stewart <pstew@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>

4 files changed, 71 insertions, 23 deletions

diff --git a/chromeos-dbus-bindings/adaptor_generator.cc b/chromeos-dbus-bindings/adaptor_generator.cc
index 7057bc9..66b7c1a 100644
--- a/chromeos-dbus-bindings/adaptor_generator.cc
+++ b/chromeos-dbus-bindings/adaptor_generator.cc
@@ -69,12 +69,25 @@ bool AdaptorGenerator::GenerateAdaptor(
   text.AddLine("virtual void OnRegisterComplete(bool success) {}");
   text.PopOffset();
 
+  text.AddBlankLine();
+  text.AddLineWithOffset("protected:", kScopeOffset);
+  text.PushOffset(kBlockOffset);
+  text.AddLine("chromeos::dbus_utils::DBusInterface* dbus_interface() {");
+  text.PushOffset(kBlockOffset);
+  text.AddLine("return dbus_interface_;");
+  text.PopOffset();
+  text.AddLine("}");
+  text.PopOffset();
+
+  text.AddBlankLine();
   text.AddLineWithOffset("private:", kScopeOffset);
 
   text.PushOffset(kBlockOffset);
   text.AddLine(StringPrintf("%s* interface_;  // Owned by caller.",
                             method_interface.c_str()));
   text.AddLine("chromeos::dbus_utils::DBusObject dbus_object_;");
+  text.AddLine("// Owned by |dbus_object_|.");
+  text.AddLine("chromeos::dbus_utils::DBusInterface* dbus_interface_;");
   text.AddLine(StringPrintf(
       "DISALLOW_COPY_AND_ASSIGN(%s);", adaptor_name.c_str()));
   text.PopOffset();
@@ -130,17 +143,22 @@ void AdaptorGenerator::AddConstructor(const Interface& interface,
   block.PushOffset(kLineContinuationOffset);
   block.AddLine("object_manager,");
   block.AddLine("object_manager->GetBus(),");
-  block.AddLine("dbus::ObjectPath(object_path)) {");
+  block.AddLine("dbus::ObjectPath(object_path)),");
+  block.PopOffset();
+  block.AddLine("dbus_interface_(");
+  block.PushOffset(kLineContinuationOffset);
+  block.AddLine(StringPrintf(
+      "dbus_object_.AddOrGetInterface(\"%s\")) {", interface.name.c_str()));
   block.PopOffset();
   block.PopOffset();
   block.PopOffset();
   block.PushOffset(kBlockOffset);
-  block.AddLine("auto* itf =");
-  block.AddLineWithOffset(StringPrintf(
-      "dbus_object_.AddOrGetInterface(\"%s\");", interface.name.c_str()),
-      kLineContinuationOffset);
   for (const auto& method : interface.methods) {
-    block.AddLine("itf->AddMethodHandler(");
+    if (method.output_arguments.size() > 1) {
+      // TODO(pstew): Accept multiple output arguments.  crbug.com/419271
+      continue;
+    }
+    block.AddLine("dbus_interface_->AddMethodHandler(");
     block.PushOffset(kLineContinuationOffset);
     block.AddLine(StringPrintf("\"%s\",", method.name.c_str()));
     block.AddLine("base::Unretained(interface_),");
@@ -171,10 +189,13 @@ void AdaptorGenerator::AddMethodInterface(const Interface& interface,
   for (const auto& method : interface.methods) {
     string return_type("void");
     if (!method.output_arguments.empty()) {
-      CHECK_EQ(1UL, method.output_arguments.size())
-          << "Method " << method.name << " has "
-          << method.output_arguments.size()
-          << " output arguments which is invalid.";
+      if (method.output_arguments.size() > 1) {
+        // TODO(pstew): Accept multiple output arguments.  crbug.com://419271
+        LOG(WARNING) << "Method " << method.name << " has "
+                     << method.output_arguments.size()
+                     << " output arguments which is unsupported.";
+        continue;
+      }
       CHECK(signature.Parse(method.output_arguments[0].type, &return_type));
     }
     block.AddLine(StringPrintf("virtual %s %s(",
diff --git a/chromeos-dbus-bindings/adaptor_generator_unittest.cc b/chromeos-dbus-bindings/adaptor_generator_unittest.cc
index 4ba313a..89683ca 100644
--- a/chromeos-dbus-bindings/adaptor_generator_unittest.cc
+++ b/chromeos-dbus-bindings/adaptor_generator_unittest.cc
@@ -32,6 +32,9 @@ const char kMethod1Name[] = "Tetsuo";
 const char kMethod1Argument1[] = "i";
 const char kMethod1Return[] = "x";
 const char kMethod2Name[] = "Kei";
+const char kMethod3Name[] = "Kiyoko";
+const char kMethod3Return0[] = "x";
+const char kMethod3Return1[] = "s";
 const char kInterfaceName[] = "org.chromium.TestInterface";
 const char kExpectedContent[] = R"literal_string(
 #include <string>
@@ -68,18 +71,18 @@ class TestInterfaceAdaptor {
         dbus_object_(
             object_manager,
             object_manager->GetBus(),
-            dbus::ObjectPath(object_path)) {
-    auto* itf =
-        dbus_object_.AddOrGetInterface("org.chromium.TestInterface");
-    itf->AddMethodHandler(
+            dbus::ObjectPath(object_path)),
+        dbus_interface_(
+            dbus_object_.AddOrGetInterface("org.chromium.TestInterface")) {
+    dbus_interface_->AddMethodHandler(
         "Kaneda",
         base::Unretained(interface_),
         &TestInterfaceAdaptorMethodInterface::Kaneda);
-    itf->AddMethodHandler(
+    dbus_interface_->AddMethodHandler(
         "Tetsuo",
         base::Unretained(interface_),
         &TestInterfaceAdaptorMethodInterface::Tetsuo);
-    itf->AddMethodHandler(
+    dbus_interface_->AddMethodHandler(
         "Kei",
         base::Unretained(interface_),
         &TestInterfaceAdaptorMethodInterface::Kei);
@@ -88,9 +91,17 @@ class TestInterfaceAdaptor {
   }
   virtual ~TestInterfaceAdaptor() = default;
   virtual void OnRegisterComplete(bool success) {}
+
+ protected:
+  chromeos::dbus_utils::DBusInterface* dbus_interface() {
+    return dbus_interface_;
+  }
+
  private:
   TestInterfaceAdaptorMethodInterface* interface_;  // Owned by caller.
   chromeos::dbus_utils::DBusObject dbus_object_;
+  // Owned by |dbus_object_|.
+  chromeos::dbus_utils::DBusInterface* dbus_interface_;
   DISALLOW_COPY_AND_ASSIGN(TestInterfaceAdaptor);
 };
 
@@ -124,14 +135,21 @@ TEST_F(AdaptorGeneratorTest, GenerateAdaptors) {
   interface.methods.emplace_back(
       kMethod0Name,
       vector<Interface::Argument>{
-          Interface::Argument(kMethod0ArgumentName0, kMethod0Argument0),
-          Interface::Argument(kMethod0ArgumentName1, kMethod0Argument1) },
-      vector<Interface::Argument>{ Interface::Argument("", kMethod0Return) });
+          {kMethod0ArgumentName0, kMethod0Argument0},
+          {kMethod0ArgumentName1, kMethod0Argument1}},
+      vector<Interface::Argument>{{"", kMethod0Return}});
   interface.methods.emplace_back(
       kMethod1Name,
-      vector<Interface::Argument>{ Interface::Argument("", kMethod1Argument1) },
-      vector<Interface::Argument>{ Interface::Argument("", kMethod1Return) });
+      vector<Interface::Argument>{{"", kMethod1Argument1}},
+      vector<Interface::Argument>{{"", kMethod1Return}});
   interface.methods.emplace_back(kMethod2Name);
+  // Interface methods with more than one return argument should be ignored.
+  interface.methods.emplace_back(
+      kMethod3Name,
+      vector<Interface::Argument>{},
+      vector<Interface::Argument>{
+          {"", kMethod3Return0},
+          {"", kMethod3Return1}});
   base::FilePath output_path = temp_dir_.path().Append("output.h");
   EXPECT_TRUE(generator_.GenerateAdaptor(interface, output_path));
   string contents;
diff --git a/chromeos-dbus-bindings/indented_text.cc b/chromeos-dbus-bindings/indented_text.cc
index 9a8a7f7..a27b0e7 100644
--- a/chromeos-dbus-bindings/indented_text.cc
+++ b/chromeos-dbus-bindings/indented_text.cc
@@ -17,6 +17,10 @@ namespace chromeos_dbus_bindings {
 
 IndentedText::IndentedText() : offset_(0) {}
 
+void IndentedText::AddBlankLine() {
+  AddLine("");
+}
+
 void IndentedText::AddBlock(const IndentedText& block) {
   AddBlockWithOffset(block, 0);
 }
@@ -38,8 +42,10 @@ void IndentedText::AddLineWithOffset(const std::string& line, size_t shift) {
 string IndentedText::GetContents() const {
   string output;
   for (const auto& member : contents_) {
-    string indent(member.second, ' ');
-    output.append(indent + member.first + "\n");
+    const string& line = member.first;
+    size_t shift = line.empty() ? 0 : member.second;
+    string indent(shift, ' ');
+    output.append(indent + line + "\n");
   }
   return output;
 }
diff --git a/chromeos-dbus-bindings/indented_text.h b/chromeos-dbus-bindings/indented_text.h
index 059e85d..6d24900 100644
--- a/chromeos-dbus-bindings/indented_text.h
+++ b/chromeos-dbus-bindings/indented_text.h
@@ -18,6 +18,9 @@ class IndentedText {
   IndentedText();
   virtual ~IndentedText() = default;
 
+  // Insert a blank line.
+  void AddBlankLine();
+
   // Insert a block of indented text.
   void AddBlock(const IndentedText& block);
   void AddBlockWithOffset(const IndentedText& block, size_t shift);