Fix design shortcomings in Message classes.

- Prohibit to instantiate Message class.
  Rationale: this is not corresponding to any D-Bus message types.

- Get rid of Message::reset_raw_message()
  Rationale: this was breaking encapsulation. For instance, It was possible
  to inject a DBUS_MESSAGE_TYPE_ERROR raw message to a MethodCall message,
  which should not be allowed.

- Prohibit to instantiate Response/ErrorResponse with NULL raw message.
  Rationale: Message objects should be backed up by valid raw messages.

- Change Object::CallMethodAndBlock() to return Response*.
  Rationale: the original API requred a Response object with raw_message_ set
  to NULL, which we no longer allow.

- Add message_type header to ToString().

BUG=90036
TEST=dbus_unittests


Review URL: http://codereview.chromium.org/7709009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@97983 0039d316-1c4b-4281-b951-d872f2087c98

1 files changed, 79 insertions, 100 deletions

diff --git a/dbus/message_unittest.cc b/dbus/message_unittest.cc
index 6e45e55..473a6ef 100644
--- a/dbus/message_unittest.cc
+++ b/dbus/message_unittest.cc
@@ -12,12 +12,11 @@
 // Test that a byte can be properly written and read. We only have this
 // test for byte, as repeating this for other basic types is too redundant.
 TEST(MessageTest, AppendAndPopByte) {
-  dbus::Message message;
-  message.reset_raw_message(dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_CALL));
-  dbus::MessageWriter writer(&message);
+  scoped_ptr<dbus::Response> message(dbus::Response::CreateEmpty());
+  dbus::MessageWriter writer(message.get());
   writer.AppendByte(123);  // The input is 123.
 
-  dbus::MessageReader reader(&message);
+  dbus::MessageReader reader(message.get());
   ASSERT_TRUE(reader.HasMoreData());  // Should have data to read.
   ASSERT_EQ(dbus::Message::BYTE, reader.GetDataType());
 
@@ -36,9 +35,8 @@ TEST(MessageTest, AppendAndPopByte) {
 
 // Check all basic types can be properly written and read.
 TEST(MessageTest, AppendAndPopBasicDataTypes) {
-  dbus::Message message;
-  message.reset_raw_message(dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_CALL));
-  dbus::MessageWriter writer(&message);
+  scoped_ptr<dbus::Response> message(dbus::Response::CreateEmpty());
+  dbus::MessageWriter writer(message.get());
 
   // Append 0, 1, 2, 3, 4, 5, 6, 7, 8, "string", "/object/path".
   writer.AppendByte(0);
@@ -65,7 +63,7 @@ TEST(MessageTest, AppendAndPopBasicDataTypes) {
   std::string string_value;
   std::string object_path_value;
 
-  dbus::MessageReader reader(&message);
+  dbus::MessageReader reader(message.get());
   ASSERT_TRUE(reader.HasMoreData());
   ASSERT_TRUE(reader.PopByte(&byte_value));
   ASSERT_TRUE(reader.PopBool(&bool_value));
@@ -96,9 +94,8 @@ TEST(MessageTest, AppendAndPopBasicDataTypes) {
 
 // Check all variant types can be properly written and read.
 TEST(MessageTest, AppendAndPopVariantDataTypes) {
-  dbus::Message message;
-  message.reset_raw_message(dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_CALL));
-  dbus::MessageWriter writer(&message);
+  scoped_ptr<dbus::Response> message(dbus::Response::CreateEmpty());
+  dbus::MessageWriter writer(message.get());
 
   // Append 0, 1, 2, 3, 4, 5, 6, 7, 8, "string", "/object/path".
   writer.AppendVariantOfByte(0);
@@ -125,7 +122,7 @@ TEST(MessageTest, AppendAndPopVariantDataTypes) {
   std::string string_value;
   std::string object_path_value;
 
-  dbus::MessageReader reader(&message);
+  dbus::MessageReader reader(message.get());
   ASSERT_TRUE(reader.HasMoreData());
   ASSERT_TRUE(reader.PopVariantOfByte(&byte_value));
   ASSERT_TRUE(reader.PopVariantOfBool(&bool_value));
@@ -155,16 +152,15 @@ TEST(MessageTest, AppendAndPopVariantDataTypes) {
 }
 
 TEST(MessageTest, ArrayOfBytes) {
-  dbus::Message message;
-  message.reset_raw_message(dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_CALL));
-  dbus::MessageWriter writer(&message);
+  scoped_ptr<dbus::Response> message(dbus::Response::CreateEmpty());
+  dbus::MessageWriter writer(message.get());
   std::vector<uint8> bytes;
   bytes.push_back(1);
   bytes.push_back(2);
   bytes.push_back(3);
   writer.AppendArrayOfBytes(bytes.data(), bytes.size());
 
-  dbus::MessageReader reader(&message);
+  dbus::MessageReader reader(message.get());
   uint8* output_bytes = NULL;
   size_t length = 0;
   ASSERT_TRUE(reader.PopArrayOfBytes(&output_bytes, &length));
@@ -176,16 +172,15 @@ TEST(MessageTest, ArrayOfBytes) {
 }
 
 TEST(MessageTest, ArrayOfObjectPaths) {
-  dbus::Message message;
-  message.reset_raw_message(dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_CALL));
-  dbus::MessageWriter writer(&message);
+  scoped_ptr<dbus::Response> message(dbus::Response::CreateEmpty());
+  dbus::MessageWriter writer(message.get());
   std::vector<std::string> object_paths;
   object_paths.push_back("/object/path/1");
   object_paths.push_back("/object/path/2");
   object_paths.push_back("/object/path/3");
   writer.AppendArrayOfObjectPaths(object_paths);
 
-  dbus::MessageReader reader(&message);
+  dbus::MessageReader reader(message.get());
   std::vector<std::string> output_object_paths;
   ASSERT_TRUE(reader.PopArrayOfObjectPaths(&output_object_paths));
   ASSERT_FALSE(reader.HasMoreData());
@@ -199,19 +194,18 @@ TEST(MessageTest, ArrayOfObjectPaths) {
 // test for array, as repeating this for other container types is too
 // redundant.
 TEST(MessageTest, OpenArrayAndPopArray) {
-  dbus::Message message;
-  message.reset_raw_message(dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_CALL));
-  dbus::MessageWriter writer(&message);
-  dbus::MessageWriter array_writer(&message);
+  scoped_ptr<dbus::Response> message(dbus::Response::CreateEmpty());
+  dbus::MessageWriter writer(message.get());
+  dbus::MessageWriter array_writer(message.get());
   writer.OpenArray("s", &array_writer);  // Open an array of strings.
   array_writer.AppendString("foo");
   array_writer.AppendString("bar");
   array_writer.AppendString("baz");
   writer.CloseContainer(&array_writer);
 
-  dbus::MessageReader reader(&message);
+  dbus::MessageReader reader(message.get());
   ASSERT_EQ(dbus::Message::ARRAY, reader.GetDataType());
-  dbus::MessageReader array_reader(&message);
+  dbus::MessageReader array_reader(message.get());
   ASSERT_TRUE(reader.PopArray(&array_reader));
   ASSERT_FALSE(reader.HasMoreData());  // Should not have more data to read.
 
@@ -229,17 +223,16 @@ TEST(MessageTest, OpenArrayAndPopArray) {
 // Create a complex message using array, struct, variant, dict entry, and
 // make sure it can be read properly.
 TEST(MessageTest, CreateComplexMessageAndReadIt) {
-  dbus::Message message;
-  message.reset_raw_message(dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_CALL));
-  dbus::MessageWriter writer(&message);
+  scoped_ptr<dbus::Response> message(dbus::Response::CreateEmpty());
+  dbus::MessageWriter writer(message.get());
   {
-    dbus::MessageWriter array_writer(&message);
+    dbus::MessageWriter array_writer(message.get());
     // Open an array of variants.
     writer.OpenArray("v", &array_writer);
     {
       // The first value in the array.
       {
-        dbus::MessageWriter variant_writer(&message);
+        dbus::MessageWriter variant_writer(message.get());
         // Open a variant of a boolean.
         array_writer.OpenVariant("b", &variant_writer);
         variant_writer.AppendBool(true);
@@ -248,11 +241,11 @@ TEST(MessageTest, CreateComplexMessageAndReadIt) {
 
       // The second value in the array.
       {
-        dbus::MessageWriter variant_writer(&message);
+        dbus::MessageWriter variant_writer(message.get());
         // Open a variant of a struct that contains a string and an int32.
         array_writer.OpenVariant("(si)", &variant_writer);
         {
-          dbus::MessageWriter struct_writer(&message);
+          dbus::MessageWriter struct_writer(message.get());
           variant_writer.OpenStruct(&struct_writer);
           struct_writer.AppendString("string");
           struct_writer.AppendInt32(123);
@@ -263,16 +256,16 @@ TEST(MessageTest, CreateComplexMessageAndReadIt) {
 
       // The third value in the array.
       {
-        dbus::MessageWriter variant_writer(&message);
+        dbus::MessageWriter variant_writer(message.get());
         // Open a variant of an array of string-to-int64 dict entries.
         array_writer.OpenVariant("a{sx}", &variant_writer);
         {
           // Opens an array of string-to-int64 dict entries.
-          dbus::MessageWriter dict_array_writer(&message);
+          dbus::MessageWriter dict_array_writer(message.get());
           variant_writer.OpenArray("{sx}", &dict_array_writer);
           {
             // Opens a string-to-int64 dict entries.
-            dbus::MessageWriter dict_entry_writer(&message);
+            dbus::MessageWriter dict_entry_writer(message.get());
             dict_array_writer.OpenDictEntry(&dict_entry_writer);
             dict_entry_writer.AppendString("foo");
             dict_entry_writer.AppendInt64(GG_INT64_C(1234567890123456789));
@@ -286,7 +279,8 @@ TEST(MessageTest, CreateComplexMessageAndReadIt) {
     writer.CloseContainer(&array_writer);
   }
   // What we have created looks like this:
-  EXPECT_EQ("signature: av\n"
+  EXPECT_EQ("message_type: MESSAGE_METHOD_RETURN\n"
+            "signature: av\n"
             "\n"
             "array [\n"
             "  variant     bool true\n"
@@ -301,10 +295,10 @@ TEST(MessageTest, CreateComplexMessageAndReadIt) {
             "      }\n"
             "    ]\n"
             "]\n",
-            message.ToString());
+            message->ToString());
 
-  dbus::MessageReader reader(&message);
-  dbus::MessageReader array_reader(&message);
+  dbus::MessageReader reader(message.get());
+  dbus::MessageReader array_reader(message.get());
   ASSERT_TRUE(reader.PopArray(&array_reader));
 
   // The first value in the array.
@@ -314,10 +308,10 @@ TEST(MessageTest, CreateComplexMessageAndReadIt) {
 
   // The second value in the array.
   {
-    dbus::MessageReader variant_reader(&message);
+    dbus::MessageReader variant_reader(message.get());
     ASSERT_TRUE(array_reader.PopVariant(&variant_reader));
     {
-      dbus::MessageReader struct_reader(&message);
+      dbus::MessageReader struct_reader(message.get());
       ASSERT_TRUE(variant_reader.PopStruct(&struct_reader));
       std::string string_value;
       ASSERT_TRUE(struct_reader.PopString(&string_value));
@@ -332,13 +326,13 @@ TEST(MessageTest, CreateComplexMessageAndReadIt) {
 
   // The third value in the array.
   {
-    dbus::MessageReader variant_reader(&message);
+    dbus::MessageReader variant_reader(message.get());
     ASSERT_TRUE(array_reader.PopVariant(&variant_reader));
     {
-      dbus::MessageReader dict_array_reader(&message);
+      dbus::MessageReader dict_array_reader(message.get());
       ASSERT_TRUE(variant_reader.PopArray(&dict_array_reader));
       {
-        dbus::MessageReader dict_entry_reader(&message);
+        dbus::MessageReader dict_entry_reader(message.get());
         ASSERT_TRUE(dict_array_reader.PopDictEntry(&dict_entry_reader));
         std::string string_value;
         ASSERT_TRUE(dict_entry_reader.PopString(&string_value));
@@ -355,23 +349,19 @@ TEST(MessageTest, CreateComplexMessageAndReadIt) {
   ASSERT_FALSE(reader.HasMoreData());
 }
 
-TEST(MessageTest, Message) {
-  dbus::Message message;
-  EXPECT_TRUE(message.raw_message() == NULL);
-  EXPECT_EQ(dbus::Message::MESSAGE_INVALID, message.GetMessageType());
-}
-
 TEST(MessageTest, MethodCall) {
   dbus::MethodCall method_call("com.example.Interface", "SomeMethod");
   EXPECT_TRUE(method_call.raw_message() != NULL);
   EXPECT_EQ(dbus::Message::MESSAGE_METHOD_CALL, method_call.GetMessageType());
+  EXPECT_EQ("MESSAGE_METHOD_CALL", method_call.GetMessageTypeAsString());
   method_call.SetDestination("com.example.Service");
   method_call.SetPath("/com/example/Object");
 
   dbus::MessageWriter writer(&method_call);
   writer.AppendString("payload");
 
-  EXPECT_EQ("destination: com.example.Service\n"
+  EXPECT_EQ("message_type: MESSAGE_METHOD_CALL\n"
+            "destination: com.example.Service\n"
             "path: /com/example/Object\n"
             "interface: com.example.Interface\n"
             "member: SomeMethod\n"
@@ -396,12 +386,14 @@ TEST(MessageTest, Signal) {
   dbus::Signal signal("com.example.Interface", "SomeSignal");
   EXPECT_TRUE(signal.raw_message() != NULL);
   EXPECT_EQ(dbus::Message::MESSAGE_SIGNAL, signal.GetMessageType());
+  EXPECT_EQ("MESSAGE_SIGNAL", signal.GetMessageTypeAsString());
   signal.SetPath("/com/example/Object");
 
   dbus::MessageWriter writer(&signal);
   writer.AppendString("payload");
 
-  EXPECT_EQ("path: /com/example/Object\n"
+  EXPECT_EQ("message_type: MESSAGE_SIGNAL\n"
+            "path: /com/example/Object\n"
             "interface: com.example.Interface\n"
             "member: SomeSignal\n"
             "signature: s\n"
@@ -422,11 +414,10 @@ TEST(MessageTest, Signal_FromRawMessage) {
 }
 
 TEST(MessageTest, Response) {
-  dbus::Response response;
-  EXPECT_TRUE(response.raw_message() == NULL);
-  response.reset_raw_message(
-      dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN));
-  EXPECT_EQ(dbus::Message::MESSAGE_METHOD_RETURN, response.GetMessageType());
+  scoped_ptr<dbus::Response> response(dbus::Response::CreateEmpty());
+  EXPECT_TRUE(response->raw_message());
+  EXPECT_EQ(dbus::Message::MESSAGE_METHOD_RETURN, response->GetMessageType());
+  EXPECT_EQ("MESSAGE_METHOD_RETURN", response->GetMessageTypeAsString());
 }
 
 TEST(MessageTest, Response_FromMethodCall) {
@@ -437,18 +428,11 @@ TEST(MessageTest, Response_FromMethodCall) {
   scoped_ptr<dbus::Response> response(
       dbus::Response::FromMethodCall(&method_call));
   EXPECT_EQ(dbus::Message::MESSAGE_METHOD_RETURN, response->GetMessageType());
+  EXPECT_EQ("MESSAGE_METHOD_RETURN", response->GetMessageTypeAsString());
   // The serial should be copied to the reply serial.
   EXPECT_EQ(kSerial, response->GetReplySerial());
 }
 
-TEST(MessageTest, ErrorResponse) {
-  dbus::ErrorResponse error_response;
-  EXPECT_TRUE(error_response.raw_message() == NULL);
-  error_response.reset_raw_message(
-      dbus_message_new(DBUS_MESSAGE_TYPE_ERROR));
-  EXPECT_EQ(dbus::Message::MESSAGE_ERROR, error_response.GetMessageType());
-}
-
 TEST(MessageTest, ErrorResponse_FromMethodCall) {
   const uint32 kSerial = 123;
 const char kErrorMessage[] = "error message";
@@ -461,6 +445,7 @@ const char kErrorMessage[] = "error message";
                                           DBUS_ERROR_FAILED,
                                           kErrorMessage));
   EXPECT_EQ(dbus::Message::MESSAGE_ERROR, error_response->GetMessageType());
+  EXPECT_EQ("MESSAGE_ERROR", error_response->GetMessageTypeAsString());
   // The serial should be copied to the reply serial.
   EXPECT_EQ(kSerial, error_response->GetReplySerial());
 
@@ -471,39 +456,33 @@ const char kErrorMessage[] = "error message";
   EXPECT_EQ(kErrorMessage, error_message);
 }
 
-TEST(MessageTest, ToString_EmptyMessage) {
-  dbus::Message message;
-  EXPECT_EQ("", message.ToString());
-}
-
 TEST(MessageTest, GetAndSetHeaders) {
-  dbus::Message message;
-  message.reset_raw_message(dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_CALL));
-
-  EXPECT_EQ("", message.GetDestination());
-  EXPECT_EQ("", message.GetPath());
-  EXPECT_EQ("", message.GetInterface());
-  EXPECT_EQ("", message.GetMember());
-  EXPECT_EQ("", message.GetErrorName());
-  EXPECT_EQ("", message.GetSender());
-  EXPECT_EQ(0U, message.GetSerial());
-  EXPECT_EQ(0U, message.GetReplySerial());
-
-  message.SetDestination("org.chromium.destination");
-  message.SetPath("/org/chromium/path");
-  message.SetInterface("org.chromium.interface");
-  message.SetMember("member");
-  message.SetErrorName("org.chromium.error");
-  message.SetSender(":1.2");
-  message.SetSerial(123);
-  message.SetReplySerial(456);
-
-  EXPECT_EQ("org.chromium.destination", message.GetDestination());
-  EXPECT_EQ("/org/chromium/path", message.GetPath());
-  EXPECT_EQ("org.chromium.interface", message.GetInterface());
-  EXPECT_EQ("member", message.GetMember());
-  EXPECT_EQ("org.chromium.error", message.GetErrorName());
-  EXPECT_EQ(":1.2", message.GetSender());
-  EXPECT_EQ(123U, message.GetSerial());
-  EXPECT_EQ(456U, message.GetReplySerial());
+  scoped_ptr<dbus::Response> message(dbus::Response::CreateEmpty());
+
+  EXPECT_EQ("", message->GetDestination());
+  EXPECT_EQ("", message->GetPath());
+  EXPECT_EQ("", message->GetInterface());
+  EXPECT_EQ("", message->GetMember());
+  EXPECT_EQ("", message->GetErrorName());
+  EXPECT_EQ("", message->GetSender());
+  EXPECT_EQ(0U, message->GetSerial());
+  EXPECT_EQ(0U, message->GetReplySerial());
+
+  message->SetDestination("org.chromium.destination");
+  message->SetPath("/org/chromium/path");
+  message->SetInterface("org.chromium.interface");
+  message->SetMember("member");
+  message->SetErrorName("org.chromium.error");
+  message->SetSender(":1.2");
+  message->SetSerial(123);
+  message->SetReplySerial(456);
+
+  EXPECT_EQ("org.chromium.destination", message->GetDestination());
+  EXPECT_EQ("/org/chromium/path", message->GetPath());
+  EXPECT_EQ("org.chromium.interface", message->GetInterface());
+  EXPECT_EQ("member", message->GetMember());
+  EXPECT_EQ("org.chromium.error", message->GetErrorName());
+  EXPECT_EQ(":1.2", message->GetSender());
+  EXPECT_EQ(123U, message->GetSerial());
+  EXPECT_EQ(456U, message->GetReplySerial());
 }