chromeos-dbus-bindings: Add support for STRUCT and typed async methods

The async method handlers in libchromeos have been changed to use
strongly typed response objects. This change mainly changes the
adapter generator to produce the correct signatures for the async
adapter methods.

In the attempt to support existing XML files from shill and
modemmanager I implemented some features that are required there:

1. Allow nested <node> elements (which are still ignored but at
   least parsing of XML files doesn't fail.
2. Add support for <tp:docstring> element to extract comments
   from interface/method/signal/property definitions and add
   then to adaptor/proxy code which should help user to make
   sense of method intentions.
3. Added support for STRUCT D-Bus types (...). Now using
   std::tuple<> to represent D-Bus structures.
4. a{sv} are converted to chromeos::VariantDictionary instead of
   std::map<std::string, chromeos::Any> which is shorter and
   being very popular type in D-Bus, makes a few generated
   function signatures neater.
5. Added support for "org.freedesktop.DBus.GLib.Async" annotation
   to be an alias for "org.chromium.DBus.Method.Kind"="async".
6. Fixed chromeos::string_utils::Split() to eliminate empty
   strings if they were created by trimming all-whitespace
   elements of the split array.

BUG=None
TEST=FEATURES=test emerge-link chromeos-dbus-bindings

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

14 files changed, 202 insertions, 31 deletions

diff --git a/chromeos-dbus-bindings/adaptor_generator.cc b/chromeos-dbus-bindings/adaptor_generator.cc
index 5f71816..e8b5fe1 100644
--- a/chromeos-dbus-bindings/adaptor_generator.cc
+++ b/chromeos-dbus-bindings/adaptor_generator.cc
@@ -73,6 +73,7 @@ void AdaptorGenerator::GenerateInterfaceAdaptor(
   text->AddBlankLine();
   text->AddLine(StringPrintf("// Interface definition for %s.",
                              full_itf_name.c_str()));
+  text->AddComments(interface.doc_string_);
   text->AddLine(StringPrintf("class %s {", itf_name.c_str()));
   text->AddLineWithOffset("public:", kScopeOffset);
   text->PushOffset(kBlockOffset);
@@ -99,7 +100,6 @@ void AdaptorGenerator::GenerateInterfaceAdaptor(
   AddSignalDataMembers(interface, text);
   AddPropertyDataMembers(interface, text);
 
-  text->AddBlankLine();
   text->AddLine(StringPrintf(
       "%s* interface_;  // Owned by container of this adapter.",
       itf_name.c_str()));
@@ -224,12 +224,19 @@ void AdaptorGenerator::AddInterfaceMethods(const Interface& interface,
         method_params.push_back("chromeos::ErrorPtr* error");
         return_type = "bool";
         break;
-      case Interface::Method::Kind::kAsync:
-        method_params.push_back(
-            "scoped_ptr<chromeos::dbus_utils::DBusMethodResponse> response");
-        // Async methods don't return values directly.
+      case Interface::Method::Kind::kAsync: {
+        std::vector<std::string> out_types;
+        for (const auto& argument : output_arguments_copy) {
+          string param_type;
+          CHECK(signature.Parse(argument.type, &param_type));
+          out_types.push_back(param_type);
+        }
+        method_params.push_back(base::StringPrintf(
+            "scoped_ptr<chromeos::dbus_utils::DBusMethodResponse<%s>> response",
+             chromeos::string_utils::Join(", ", out_types).c_str()));
         output_arguments_copy.clear();
         break;
+      }
       case Interface::Method::Kind::kRaw:
         method_params.push_back("dbus::MethodCall* method_call");
         method_params.push_back("chromeos::dbus_utils::ResponseSender sender");
@@ -238,6 +245,7 @@ void AdaptorGenerator::AddInterfaceMethods(const Interface& interface,
         output_arguments_copy.clear();
         break;
     }
+    block.AddComments(method.doc_string_);
     string method_start = StringPrintf("virtual %s %s(",
                                        return_type.c_str(),
                                        method.name.c_str());
@@ -285,6 +293,7 @@ void AdaptorGenerator::AddSendSignalMethods(
     block.AddBlankLine();
 
   for (const auto& signal : interface.signals) {
+    block.AddComments(signal.doc_string_);
     string method_start = StringPrintf("void Send%sSignal(",
                                        signal.name.c_str());
     string method_end = ") {";
@@ -379,6 +388,7 @@ void AdaptorGenerator::AddPropertyMethodImplementation(
     string variable_name = GetPropertyVariableName(property.name);
 
     // Getter method.
+    block.AddComments(property.doc_string_);
     block.AddLine(StringPrintf("%s Get%s() const {",
                                type.c_str(),
                                property.name.c_str()));
@@ -423,6 +433,9 @@ void AdaptorGenerator::AddPropertyDataMembers(const Interface& interface,
     block.AddLine(StringPrintf("%s_;", variable_name.c_str()));
     block.PopOffset();
   }
+  if (!interface.properties.empty())
+    block.AddBlankLine();
+
   text->AddBlock(block);
 }
 
diff --git a/chromeos-dbus-bindings/adaptor_generator_unittest.cc b/chromeos-dbus-bindings/adaptor_generator_unittest.cc
index 2646c26..9076823 100644
--- a/chromeos-dbus-bindings/adaptor_generator_unittest.cc
+++ b/chromeos-dbus-bindings/adaptor_generator_unittest.cc
@@ -169,7 +169,7 @@ class Test2Interface {
   virtual std::string Kaneda2(
       const std::string& in_iwata) const = 0;
   virtual void Tetsuo2(
-      scoped_ptr<chromeos::dbus_utils::DBusMethodResponse> response,
+      scoped_ptr<chromeos::dbus_utils::DBusMethodResponse<int64_t>> response,
       int32_t in_1) = 0;
 };
 
@@ -193,7 +193,6 @@ class Test2Adaptor {
   }
 
  private:
-
   Test2Interface* interface_;  // Owned by container of this adapter.
 
   DISALLOW_COPY_AND_ASSIGN(Test2Adaptor);
@@ -269,7 +268,7 @@ TEST_F(AdaptorGeneratorTest, GenerateAdaptors) {
   interface2.methods.emplace_back(
       kMethod1Name2,
       vector<Interface::Argument>{{"", kMethod1Argument1}},
-      vector<Interface::Argument>{});
+      vector<Interface::Argument>{{"", kMethod1Return}});
   interface2.methods.back().kind = Interface::Method::Kind::kAsync;
 
   base::FilePath output_path = temp_dir_.path().Append("output.h");
diff --git a/chromeos-dbus-bindings/dbus_signature.cc b/chromeos-dbus-bindings/dbus_signature.cc
index 922ad16..a6e59d4 100644
--- a/chromeos-dbus-bindings/dbus_signature.cc
+++ b/chromeos-dbus-bindings/dbus_signature.cc
@@ -6,6 +6,7 @@
 
 #include <base/logging.h>
 #include <base/strings/stringprintf.h>
+#include <chromeos/strings/string_utils.h>
 #include <dbus/dbus-protocol.h>
 
 using base::StringPrintf;
@@ -30,6 +31,9 @@ const char DbusSignature::kUnsigned16Typename[] = "uint16_t";
 const char DbusSignature::kUnsigned32Typename[] = "uint32_t";
 const char DbusSignature::kUnsigned64Typename[] = "uint64_t";
 const char DbusSignature::kVariantTypename[] = "chromeos::Any";
+const char DbusSignature::kVariantDictTypename[] =
+    "chromeos::VariantDictionary";
+const char DbusSignature::kTupleTypename[] = "std::tuple";
 
 DbusSignature::DbusSignature()
     : object_path_typename_(kDefaultObjectPathTypename) {}
@@ -61,6 +65,12 @@ bool DbusSignature::GetTypenameForSignature(
   string::const_iterator cur = signature;
   int signature_value = *cur++;
   switch (signature_value) {
+    case DBUS_STRUCT_BEGIN_CHAR:
+      if (!GetStructTypenameForSignature(cur, end, &cur, output)) {
+        return false;
+      }
+      break;
+
     case DBUS_TYPE_ARRAY:
       if (!GetArrayTypenameForSignature(cur, end, &cur, output)) {
         return false;
@@ -168,8 +178,13 @@ bool DbusSignature::GetArrayTypenameForSignature(
                  << " where only 2 children is valid.";
       return false;
     }
-    *output = StringPrintf("%s<%s,%s>", kDictTypename,
-                           children[0].c_str(), children[1].c_str());
+    string dict_signature{signature, cur};
+    if (dict_signature == "{sv}") {
+      *output = kVariantDictTypename;
+    } else {
+      *output = StringPrintf("%s<%s, %s>", kDictTypename,
+                             children[0].c_str(), children[1].c_str());
+    }
   } else {
     string child;
     if (!GetTypenameForSignature(cur, end, &cur, &child)) {
@@ -187,4 +202,44 @@ bool DbusSignature::GetArrayTypenameForSignature(
   return true;
 }
 
+bool DbusSignature::GetStructTypenameForSignature(
+    string::const_iterator signature,
+    string::const_iterator end,
+    string::const_iterator* next,
+    string* output) {
+  string::const_iterator cur = signature;
+  if (cur == end) {
+    LOG(ERROR) << "At end of string while reading struct parameter";
+    return false;
+  }
+
+  vector<string> children;
+  while (cur != end && *cur != DBUS_STRUCT_END_CHAR) {
+    children.emplace_back();
+    if (!GetTypenameForSignature(cur, end, &cur, &children.back())) {
+      LOG(ERROR) << "Unable to decode child elements starting at "
+                 << string(cur, end);
+      return false;
+    }
+  }
+  if (cur == end) {
+    LOG(ERROR) << "At end of string while processing struct "
+               << "starting at " << string(signature, end);
+    return false;
+  }
+
+  DCHECK_EQ(DBUS_STRUCT_END_CHAR, *cur);
+  ++cur;
+
+  *output = StringPrintf(
+      "%s<%s>", kTupleTypename,
+      chromeos::string_utils::Join(", ", children).c_str());
+
+  if (next) {
+    *next = cur;
+  }
+
+  return true;
+}
+
 }  // namespace chromeos_dbus_bindings
diff --git a/chromeos-dbus-bindings/dbus_signature.h b/chromeos-dbus-bindings/dbus_signature.h
index 707d3a6..6a5a10b 100644
--- a/chromeos-dbus-bindings/dbus_signature.h
+++ b/chromeos-dbus-bindings/dbus_signature.h
@@ -47,6 +47,9 @@ class DbusSignature {
   static const char kUnsigned32Typename[];
   static const char kUnsigned64Typename[];
   static const char kVariantTypename[];
+  static const char kVariantDictTypename[];
+  static const char kPairTypename[];
+  static const char kTupleTypename[];
 
   // Returns the C++ type name for the next D-Bus signature in the string at
   // |signature| in |output|, as well as the next position within the string
@@ -66,6 +69,15 @@ class DbusSignature {
                                     std::string::const_iterator* next,
                                     std::string* output);
 
+  // Utility task for GetTypenameForSignature() which handles STRUCT objects
+  // and decodes them into a pair or tuple depending on the number of structure
+  // elements.  The arguments and return values are the same
+  // as GetTypenameForSignature().
+  bool GetStructTypenameForSignature(std::string::const_iterator signature,
+                                     std::string::const_iterator end,
+                                     std::string::const_iterator* next,
+                                     std::string* output);
+
 
   // The C++ typename to be used for D-Bus object pathnames.
   std::string object_path_typename_;
diff --git a/chromeos-dbus-bindings/dbus_signature_unittest.cc b/chromeos-dbus-bindings/dbus_signature_unittest.cc
index f8a6d46..18d9530 100644
--- a/chromeos-dbus-bindings/dbus_signature_unittest.cc
+++ b/chromeos-dbus-bindings/dbus_signature_unittest.cc
@@ -45,6 +45,9 @@ const char kStringStringVariantDictSignature[] = "a{sa{sv}}";
 const char kStringVariantDictSignature[] = "a{sv}";
 const char kStringVariantDictWithTrailingSignature[] = "a{sv}NoneOfThisParses";
 const char kUnsigned64ArraySignature[] = "at";
+const char kIntVariantDictSignature[] = "a{iv}";
+const char kPairSignature[] = "(ib)";
+const char kTupleSignature[] = "(ibs)";
 
 // Corresponding typenames for signatures above.
 const char kBoolArrayTypename[] = "std::vector<bool>";
@@ -52,17 +55,20 @@ const char kByteArrayTypename[] = "std::vector<uint8_t>";
 const char kByteArrayArrayTypename[] = "std::vector<std::vector<uint8_t>>";
 const char kObjectArrayTypename[] = "std::vector<ObjectPathType>";
 const char kObjectDictBlobTypename[] =
-    "std::map<ObjectPathType,std::map<std::string,std::map"
-    "<std::string,chromeos::Any>>>";
-const char kObjectNameDictTypename[] = "std::map<ObjectPathType,std::string>";
+    "std::map<ObjectPathType, std::map<std::string, "
+    "chromeos::VariantDictionary>>";
+const char kObjectNameDictTypename[] = "std::map<ObjectPathType, std::string>";
 const char kStringArrayTypename[] = "std::vector<std::string>";
-const char kStringStringDictTypename[] = "std::map<std::string,std::string>";
+const char kStringStringDictTypename[] = "std::map<std::string, std::string>";
 const char kStringStringStringDictTypename[] =
-    "std::map<std::string,std::map<std::string,std::string>>";
+    "std::map<std::string, std::map<std::string, std::string>>";
 const char kStringStringVariantDictTypename[] =
-    "std::map<std::string,std::map<std::string,chromeos::Any>>";
-const char kStringVariantDictTypename[] = "std::map<std::string,chromeos::Any>";
+    "std::map<std::string, chromeos::VariantDictionary>";
+const char kStringVariantDictTypename[] = "chromeos::VariantDictionary";
 const char kUnsigned64ArrayTypename[] = "std::vector<uint64_t>";
+const char kIntVariantDictTypename[] = "std::map<int32_t, chromeos::Any>";
+const char kPairTypename[] = "std::tuple<int32_t, bool>";
+const char kTupleTypename[] = "std::tuple<int32_t, bool, std::string>";
 
 // Define an object type name to disambiguate the typenames above.
 const char kObjectPathTypename[] = "ObjectPathType";
@@ -132,6 +138,9 @@ TEST_F(DbusSignatureTest, ParseSuccesses) {
     { kStringVariantDictSignature, kStringVariantDictTypename },
     { kStringVariantDictWithTrailingSignature, kStringVariantDictTypename },
     { kUnsigned64ArraySignature, kUnsigned64ArrayTypename },
+    { kIntVariantDictSignature, kIntVariantDictTypename },
+    { kPairSignature, kPairTypename },
+    { kTupleSignature, kTupleTypename },
   };
   signature_.set_object_path_typename(kObjectPathTypename);
   for (const auto& parse_test : parse_values) {
diff --git a/chromeos-dbus-bindings/generate_chromeos_dbus_bindings.cc b/chromeos-dbus-bindings/generate_chromeos_dbus_bindings.cc
index c53168c..842a64f 100644
--- a/chromeos-dbus-bindings/generate_chromeos_dbus_bindings.cc
+++ b/chromeos-dbus-bindings/generate_chromeos_dbus_bindings.cc
@@ -89,7 +89,7 @@ int main(int argc, char** argv) {
 
   if (cl->HasSwitch(switches::kProxy)) {
     std::string proxy_file = cl->GetSwitchValueASCII(switches::kProxy);
-    LOG(INFO) << "Outputting proxy to " << proxy_file;
+    VLOG(1) << "Outputting proxy to " << proxy_file;
     if (!ProxyGenerator::GenerateProxy(parser.interfaces(),
                                        base::FilePath(proxy_file))) {
       LOG(ERROR) << "Failed to output proxy.";
diff --git a/chromeos-dbus-bindings/header_generator.cc b/chromeos-dbus-bindings/header_generator.cc
index da4bc74..f947650 100644
--- a/chromeos-dbus-bindings/header_generator.cc
+++ b/chromeos-dbus-bindings/header_generator.cc
@@ -80,6 +80,7 @@ bool HeaderGenerator::WriteTextToFile(
   return true;
 }
 
+// static
 string HeaderGenerator::GetArgName(const char* prefix,
                                    const string& arg_name,
                                    int arg_index) {
diff --git a/chromeos-dbus-bindings/indented_text.cc b/chromeos-dbus-bindings/indented_text.cc
index a27b0e7..ea6b45a 100644
--- a/chromeos-dbus-bindings/indented_text.cc
+++ b/chromeos-dbus-bindings/indented_text.cc
@@ -9,6 +9,7 @@
 #include <vector>
 
 #include <base/logging.h>
+#include <chromeos/strings/string_utils.h>
 
 using std::string;
 using std::vector;
@@ -39,6 +40,14 @@ void IndentedText::AddLineWithOffset(const std::string& line, size_t shift) {
   contents_.emplace_back(line, shift + offset_);
 }
 
+void IndentedText::AddComments(const std::string& doc_string) {
+  // Split at \n, trim all whitespaces and eliminate empty strings.
+  auto lines = chromeos::string_utils::Split(doc_string, '\n', true, true);
+  for (const auto& line : lines) {
+    AddLine("// " + line);
+  }
+}
+
 string IndentedText::GetContents() const {
   string output;
   for (const auto& member : contents_) {
diff --git a/chromeos-dbus-bindings/indented_text.h b/chromeos-dbus-bindings/indented_text.h
index 6d24900..701c2e6 100644
--- a/chromeos-dbus-bindings/indented_text.h
+++ b/chromeos-dbus-bindings/indented_text.h
@@ -29,6 +29,9 @@ class IndentedText {
   void AddLine(const std::string& line);
   void AddLineWithOffset(const std::string& line, size_t shift);
 
+  // Adds a block of comments.
+  void AddComments(const std::string& doc_string);
+
   // Return a string representing the indented text.
   std::string GetContents() const;
 
diff --git a/chromeos-dbus-bindings/interface.h b/chromeos-dbus-bindings/interface.h
index 214511b..b7f5b0e 100644
--- a/chromeos-dbus-bindings/interface.h
+++ b/chromeos-dbus-bindings/interface.h
@@ -38,6 +38,7 @@ struct Interface {
     std::string name;
     std::vector<Argument> input_arguments;
     std::vector<Argument> output_arguments;
+    std::string doc_string_;
     Kind kind{Kind::kNormal};
     bool is_const{false};
   };
@@ -48,6 +49,7 @@ struct Interface {
     explicit Signal(const std::string& name_in) : name(name_in) {}
     std::string name;
     std::vector<Argument> arguments;
+    std::string doc_string_;
   };
   struct Property {
     Property(const std::string& name_in,
@@ -57,6 +59,7 @@ struct Interface {
     std::string name;
     std::string type;
     std::string access;
+    std::string doc_string_;
   };
 
   Interface() = default;
@@ -70,6 +73,7 @@ struct Interface {
   std::vector<Method> methods;
   std::vector<Signal> signals;
   std::vector<Property> properties;
+  std::string doc_string_;
 };
 
 }  // namespace chromeos_dbus_bindings
diff --git a/chromeos-dbus-bindings/proxy_generator.cc b/chromeos-dbus-bindings/proxy_generator.cc
index 67cba22..44a1c56 100644
--- a/chromeos-dbus-bindings/proxy_generator.cc
+++ b/chromeos-dbus-bindings/proxy_generator.cc
@@ -49,6 +49,7 @@ bool ProxyGenerator::GenerateProxy(
   text.AddLine("#include <chromeos/dbus/dbus_method_invoker.h>");
   text.AddLine("#include <chromeos/dbus/dbus_signal_handler.h>");
   text.AddLine("#include <chromeos/errors/error.h>");
+  text.AddLine("#include <chromeos/variant_dictionary.h>");
   text.AddLine("#include <dbus/bus.h>");
   text.AddLine("#include <dbus/message.h>");
   text.AddLine("#include <dbus/object_path.h>");
@@ -60,7 +61,7 @@ bool ProxyGenerator::GenerateProxy(
   }
   text.AddBlankLine();
 
-  text.AddLine(StringPrintf("class %s {", proxy_name.c_str()));
+  text.AddLine(StringPrintf("class %s final {", proxy_name.c_str()));
   text.AddLineWithOffset("public:", kScopeOffset);
   text.PushOffset(kBlockOffset);
   AddSignalReceiver(interfaces, &text);
@@ -104,6 +105,7 @@ void ProxyGenerator::AddConstructor(const vector<Interface>& interfaces,
                                     const string& class_name,
                                     IndentedText* text) {
   IndentedText block;
+  block.AddBlankLine();
   block.AddLine(StringPrintf("%s(", class_name.c_str()));
   block.PushOffset(kLineContinuationOffset);
   block.AddLine("const scoped_refptr<dbus::Bus>& bus,");
@@ -154,7 +156,8 @@ void ProxyGenerator::AddConstructor(const vector<Interface>& interfaces,
 void ProxyGenerator::AddDestructor(const string& class_name,
                                    IndentedText* text) {
   IndentedText block;
-  block.AddLine(StringPrintf("virtual ~%s() {", class_name.c_str()));
+  block.AddBlankLine();
+  block.AddLine(StringPrintf("~%s() {", class_name.c_str()));
   block.PushOffset(kBlockOffset);
   block.AddLine("dbus_object_proxy_->Detach();");
   block.AddLine(
@@ -167,6 +170,7 @@ void ProxyGenerator::AddDestructor(const string& class_name,
 // static
 void ProxyGenerator::AddSignalConnectedCallback(IndentedText* text) {
   IndentedText block;
+  block.AddBlankLine();
   block.AddLine("void OnDBusSignalConnected(");
   block.PushOffset(kLineContinuationOffset);
   block.AddLine("const std::string& interface,");
@@ -199,9 +203,9 @@ void ProxyGenerator::AddSignalReceiver(const vector<Interface>& interfaces,
   DbusSignature signature;
   for (const auto& interface : interfaces) {
     for (const auto& signal : interface.signals) {
+      block.AddComments(signal.doc_string_);
       string signal_begin = StringPrintf(
-          "virtual void %s(",
-          GetHandlerNameForSignal(signal.name).c_str());
+          "virtual void %s(", GetHandlerNameForSignal(signal.name).c_str());
       string signal_end = ") {}";
 
       if (signal.arguments.empty()) {
@@ -241,7 +245,9 @@ void ProxyGenerator::AddMethodProxy(const Interface::Method& method,
                                     IndentedText* text) {
   IndentedText block;
   DbusSignature signature;
-  block.AddLine(StringPrintf("virtual bool %s(", method.name.c_str()));
+  block.AddBlankLine();
+  block.AddComments(method.doc_string_);
+  block.AddLine(StringPrintf("bool %s(", method.name.c_str()));
   block.PushOffset(kLineContinuationOffset);
   vector<string> argument_names;
   int argument_number = 0;
diff --git a/chromeos-dbus-bindings/proxy_generator_unittest.cc b/chromeos-dbus-bindings/proxy_generator_unittest.cc
index 7bb1ab9..6c5e1a6 100644
--- a/chromeos-dbus-bindings/proxy_generator_unittest.cc
+++ b/chromeos-dbus-bindings/proxy_generator_unittest.cc
@@ -57,6 +57,7 @@ const char kExpectedContent[] = R"literal_string(
 #include <chromeos/dbus/dbus_method_invoker.h>
 #include <chromeos/dbus/dbus_signal_handler.h>
 #include <chromeos/errors/error.h>
+#include <chromeos/variant_dictionary.h>
 #include <dbus/bus.h>
 #include <dbus/message.h>
 #include <dbus/object_path.h>
@@ -65,7 +66,7 @@ const char kExpectedContent[] = R"literal_string(
 namespace org {
 namespace chromium {
 
-class TestInterfaceProxy {
+class TestInterfaceProxy final {
  public:
   class SignalReceiver {
    public:
@@ -74,6 +75,7 @@ class TestInterfaceProxy {
         const std::vector<std::string>&,
         uint8_t) {}
   };
+
   TestInterfaceProxy(
       const scoped_refptr<dbus::Bus>& bus,
       const std::string& service_name,
@@ -105,10 +107,12 @@ class TestInterfaceProxy {
             &TestInterfaceProxy::OnDBusSignalConnected,
             base::Unretained(this)));
   }
-  virtual ~TestInterfaceProxy() {
+
+  ~TestInterfaceProxy() {
     dbus_object_proxy_->Detach();
     bus_->RemoveObjectProxy(service_name_, object_path_, base::Closure());
   }
+
   void OnDBusSignalConnected(
       const std::string& interface,
       const std::string& signal,
@@ -120,7 +124,8 @@ class TestInterfaceProxy {
           << object_path_.value();
     }
   }
-  virtual bool Elements(
+
+  bool Elements(
       const std::string& in_space_walk,
       const std::vector<dbus::ObjectPath>& in_ramblin_man,
       std::string* out_3,
@@ -135,7 +140,8 @@ class TestInterfaceProxy {
     return response && chromeos::dbus_utils::ExtractMethodCallResults(
         response.get(), error, out_3);
   }
-  virtual bool ReturnToPatagonia(
+
+  bool ReturnToPatagonia(
       int64_t* out_1,
       chromeos::ErrorPtr* error) {
     auto response = chromeos::dbus_utils::CallMethodAndBlock(
@@ -146,7 +152,8 @@ class TestInterfaceProxy {
     return response && chromeos::dbus_utils::ExtractMethodCallResults(
         response.get(), error, out_1);
   }
-  virtual bool NiceWeatherForDucks(
+
+  bool NiceWeatherForDucks(
       bool in_1,
       chromeos::ErrorPtr* error) {
     auto response = chromeos::dbus_utils::CallMethodAndBlock(
@@ -158,7 +165,10 @@ class TestInterfaceProxy {
     return response && chromeos::dbus_utils::ExtractMethodCallResults(
         response.get(), error);
   }
-  virtual bool ExperimentNumberSix(
+
+  // Comment line1
+  // line2
+  bool ExperimentNumberSix(
       chromeos::ErrorPtr* error) {
     auto response = chromeos::dbus_utils::CallMethodAndBlock(
         dbus_object_proxy_,
@@ -168,7 +178,8 @@ class TestInterfaceProxy {
     return response && chromeos::dbus_utils::ExtractMethodCallResults(
         response.get(), error);
   }
-  virtual bool GetPersonInfo(
+
+  bool GetPersonInfo(
       std::string* out_name,
       int32_t* out_age,
       chromeos::ErrorPtr* error) {
@@ -238,6 +249,7 @@ TEST_F(ProxyGeneratorTest, GenerateAdaptors) {
       vector<Interface::Argument>{
           {"", kSignal2Argument1},
           {"", kSignal2Argument2}});
+  interface.methods.back().doc_string_ = "Comment line1\nline2";
   Interface interface2;
   interface2.name = kInterfaceName2;
   interface2.methods.emplace_back(
diff --git a/chromeos-dbus-bindings/xml_interface_parser.cc b/chromeos-dbus-bindings/xml_interface_parser.cc
index 3baef84..dd30d43 100644
--- a/chromeos-dbus-bindings/xml_interface_parser.cc
+++ b/chromeos-dbus-bindings/xml_interface_parser.cc
@@ -24,6 +24,7 @@ const char XmlInterfaceParser::kNodeTag[] = "node";
 const char XmlInterfaceParser::kSignalTag[] = "signal";
 const char XmlInterfaceParser::kPropertyTag[] = "property";
 const char XmlInterfaceParser::kAnnotationTag[] = "annotation";
+const char XmlInterfaceParser::kDocStringTag[] = "tp:docstring";
 const char XmlInterfaceParser::kNameAttribute[] = "name";
 const char XmlInterfaceParser::kTypeAttribute[] = "type";
 const char XmlInterfaceParser::kValueAttribute[] = "value";
@@ -37,6 +38,8 @@ const char XmlInterfaceParser::kFalse[] = "false";
 
 const char XmlInterfaceParser::kMethodConst[] =
     "org.chromium.DBus.Method.Const";
+const char XmlInterfaceParser::kMethodAsync[] =
+    "org.freedesktop.DBus.GLib.Async";
 
 const char XmlInterfaceParser::kMethodKind[] = "org.chromium.DBus.Method.Kind";
 const char XmlInterfaceParser::kMethodKindSimple[] = "simple";
@@ -56,6 +59,7 @@ bool XmlInterfaceParser::ParseXmlInterfaceFile(
   XML_SetElementHandler(parser,
                         &XmlInterfaceParser::HandleElementStart,
                         &XmlInterfaceParser::HandleElementEnd);
+  XML_SetCharacterDataHandler(parser, &XmlInterfaceParser::HandleCharData);
   const int kIsFinal = XML_TRUE;
 
   element_path_.clear();
@@ -81,7 +85,7 @@ void XmlInterfaceParser::OnOpenElement(
     prev_element = element_path_.back();
   element_path_.push_back(element_name);
   if (element_name == kNodeTag) {
-    CHECK(prev_element.empty())
+    CHECK(prev_element.empty() || prev_element == kNodeTag)
         << "Unexpected tag " << element_name << " inside " << prev_element;
   } else if (element_name == kInterfaceTag) {
     CHECK_EQ(kNodeTag, prev_element)
@@ -129,6 +133,9 @@ void XmlInterfaceParser::OnOpenElement(
       if (name == kMethodConst) {
         CHECK(value == kTrue || value == kFalse);
         method.is_const = (value == kTrue);
+      } else if (name == kMethodAsync) {
+        // Support GLib.Async annotation as well.
+        method.kind = Interface::Method::Kind::kAsync;
       } else if (name == kMethodKind) {
         if (value == kMethodKindSimple) {
           method.kind = Interface::Method::Kind::kSimple;
@@ -150,9 +157,38 @@ void XmlInterfaceParser::OnOpenElement(
       LOG(FATAL) << "Unexpected tag " << element_name
                  << " inside " << prev_element;
     }
+  } else if (element_name == kDocStringTag) {
+    CHECK(!prev_element.empty() && prev_element != kNodeTag)
+        << "Unexpected tag " << element_name << " inside " << prev_element;
+  }
+}
+
+void XmlInterfaceParser::OnCharData(const std::string& content) {
+  // Handle the text data only for <tp:docstring> element.
+  if (element_path_.back() != kDocStringTag)
+    return;
+
+  CHECK_GT(element_path_.size(), 1u);
+  string* doc_string_ptr = nullptr;
+  string target_element = element_path_[element_path_.size() - 2];
+  if (target_element == kInterfaceTag) {
+    doc_string_ptr = &(interfaces_.back().doc_string_);
+  } else if (target_element == kMethodTag) {
+    doc_string_ptr = &(interfaces_.back().methods.back().doc_string_);
+  } else if (target_element == kSignalTag) {
+    doc_string_ptr = &(interfaces_.back().signals.back().doc_string_);
+  } else if (target_element == kPropertyTag) {
+    doc_string_ptr = &(interfaces_.back().properties.back().doc_string_);
   }
+
+  // If <tp:docstring> is attached to elements we don't care about, do nothing.
+  if (doc_string_ptr == nullptr)
+    return;
+
+  (*doc_string_ptr) += content;
 }
 
+
 void XmlInterfaceParser::AddMethodArgument(const XmlAttributeMap& attributes) {
   string argument_direction;
   bool is_direction_paramter_present = GetElementAttribute(
@@ -275,4 +311,12 @@ void XmlInterfaceParser::HandleElementEnd(void* user_data,
   parser->OnCloseElement(element);
 }
 
+// static
+void XmlInterfaceParser::HandleCharData(void* user_data,
+                                        const char *content,
+                                        int length) {
+  auto parser = reinterpret_cast<XmlInterfaceParser*>(user_data);
+  parser->OnCharData(string(content, length));
+}
+
 }  // namespace chromeos_dbus_bindings
diff --git a/chromeos-dbus-bindings/xml_interface_parser.h b/chromeos-dbus-bindings/xml_interface_parser.h
index 059f1d6..7883ac5 100644
--- a/chromeos-dbus-bindings/xml_interface_parser.h
+++ b/chromeos-dbus-bindings/xml_interface_parser.h
@@ -44,6 +44,7 @@ class XmlInterfaceParser {
   static const char kSignalTag[];
   static const char kPropertyTag[];
   static const char kAnnotationTag[];
+  static const char kDocStringTag[];
 
   // XML attribute names.
   static const char kNameAttribute[];
@@ -61,6 +62,7 @@ class XmlInterfaceParser {
   static const char kFalse[];
 
   static const char kMethodConst[];
+  static const char kMethodAsync[];
 
   static const char kMethodKind[];
   static const char kMethodKindSimple[];
@@ -73,6 +75,7 @@ class XmlInterfaceParser {
   void OnOpenElement(const std::string& element_name,
                      const XmlAttributeMap& attributes);
   void OnCloseElement(const std::string& element_name);
+  void OnCharData(const std::string& content);
 
   // Methods for appending individual argument elements to the parser.
   void AddMethodArgument(const XmlAttributeMap& attributes);
@@ -109,6 +112,7 @@ class XmlInterfaceParser {
                                  const XML_Char* element,
                                  const XML_Char** attr);
   static void HandleElementEnd(void* user_data, const XML_Char* element);
+  static void HandleCharData(void* user_data, const char *content, int length);
 
   // The output of the parse.
   std::vector<Interface> interfaces_;