Mojo: prepare for moving num_bytes out of MessageHeader

Review URL: https://codereview.chromium.org/180033005

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

10 files changed, 108 insertions, 76 deletions

diff --git a/mojo/public/bindings/generators/cpp_templates/interface_definition.tmpl b/mojo/public/bindings/generators/cpp_templates/interface_definition.tmpl
index f4198f1..6add826 100644
--- a/mojo/public/bindings/generators/cpp_templates/interface_definition.tmpl
+++ b/mojo/public/bindings/generators/cpp_templates/interface_definition.tmpl
@@ -47,9 +47,9 @@ void {{proxy_name}}::{{method.name}}({{params_list(method)}}) {
 {%-   endfor %}
 
   mojo::Message message;
-  params->EncodePointersAndHandles(&message.handles);
+  params->EncodePointersAndHandles(message.mutable_handles());
 
-  message.data = builder.Finish();
+  message.AdoptData(builder.Finish());
 
   receiver_->Accept(&message);
 }
@@ -76,12 +76,12 @@ params->{{param.name}}()
 {%- endmacro %}
 
 bool {{class_name}}Stub::Accept(mojo::Message* message) {
-  switch (message->data->header.name) {
+  switch (message->data()->header.name) {
 {%- for method in interface.methods %}
     case internal::k{{class_name}}_{{method.name}}_Name: {
       internal::{{class_name}}_{{method.name}}_Params_Data* params =
           reinterpret_cast<internal::{{class_name}}_{{method.name}}_Params_Data*>(
-              message->data->payload);
+              message->mutable_data()->payload);
 
       if (!params->DecodePointersAndHandles(message))
         return false;
diff --git a/mojo/public/bindings/generators/cpp_templates/struct_macros.tmpl b/mojo/public/bindings/generators/cpp_templates/struct_macros.tmpl
index 9d09052..106a18d 100644
--- a/mojo/public/bindings/generators/cpp_templates/struct_macros.tmpl
+++ b/mojo/public/bindings/generators/cpp_templates/struct_macros.tmpl
@@ -76,7 +76,8 @@ if (!mojo::internal::Decode(&{{pf.field.name}}_, message))
   return false;
 {%    endfor %}
 {%-   for pf in struct.packed.packed_fields if pf.field.kind|is_handle_kind -%}
-if (!mojo::internal::DecodeHandle(&{{pf.field.name}}_, &message->handles))
+if (!mojo::internal::DecodeHandle(&{{pf.field.name}}_,
+                                  message->mutable_handles()))
   return false;
 {%    endfor %}
 {%- endmacro -%}
diff --git a/mojo/public/bindings/lib/array_internal.cc b/mojo/public/bindings/lib/array_internal.cc
index 17a26b2..a10fe23 100644
--- a/mojo/public/bindings/lib/array_internal.cc
+++ b/mojo/public/bindings/lib/array_internal.cc
@@ -49,7 +49,7 @@ bool ArraySerializationHelper<Handle>::DecodePointersAndHandles(
     ElementType* elements,
     Message* message) {
   for (uint32_t i = 0; i < header->num_elements; ++i) {
-    if (!DecodeHandle(&elements[i], &message->handles))
+    if (!DecodeHandle(&elements[i], message->mutable_handles()))
       return false;
   }
   return true;
diff --git a/mojo/public/bindings/lib/bindings_serialization.cc b/mojo/public/bindings/lib/bindings_serialization.cc
index 919a4ed..f0b14d6 100644
--- a/mojo/public/bindings/lib/bindings_serialization.cc
+++ b/mojo/public/bindings/lib/bindings_serialization.cc
@@ -40,8 +40,8 @@ bool ValidatePointer(const void* ptr, const Message& message) {
   if (reinterpret_cast<ptrdiff_t>(data) % 8 != 0)
     return false;
 
-  const uint8_t* data_start = reinterpret_cast<const uint8_t*>(message.data);
-  const uint8_t* data_end = data_start + message.data->header.num_bytes;
+  const uint8_t* data_start = reinterpret_cast<const uint8_t*>(message.data());
+  const uint8_t* data_end = data_start + message.data()->header.num_bytes;
 
   return data >= data_start && data < data_end;
 }
diff --git a/mojo/public/bindings/lib/connector.cc b/mojo/public/bindings/lib/connector.cc
index 9cbcfda..e2709fd 100644
--- a/mojo/public/bindings/lib/connector.cc
+++ b/mojo/public/bindings/lib/connector.cc
@@ -41,18 +41,19 @@ bool Connector::Accept(Message* message) {
 
   MojoResult rv = WriteMessageRaw(
       message_pipe_.get(),
-      message->data,
-      message->data->header.num_bytes,
-      message->handles.empty() ? NULL :
-          reinterpret_cast<const MojoHandle*>(&message->handles[0]),
-      static_cast<uint32_t>(message->handles.size()),
+      message->data(),
+      message->data()->header.num_bytes,
+      message->mutable_handles()->empty() ? NULL :
+          reinterpret_cast<const MojoHandle*>(
+              &message->mutable_handles()->front()),
+      static_cast<uint32_t>(message->mutable_handles()->size()),
       MOJO_WRITE_MESSAGE_FLAG_NONE);
 
   switch (rv) {
     case MOJO_RESULT_OK:
       // The handles were successfully transferred, so we don't need the message
       // to track their lifetime any longer.
-      message->handles.clear();
+      message->mutable_handles()->clear();
       break;
     case MOJO_RESULT_FAILED_PRECONDITION:
       // There's no point in continuing to write to this pipe since the other
@@ -118,16 +119,17 @@ void Connector::ReadMore() {
     }
 
     Message message;
-    message.data = static_cast<MessageData*>(malloc(num_bytes));
-    message.handles.resize(num_handles);
-
-    rv = ReadMessageRaw(message_pipe_.get(),
-                        message.data,
-                        &num_bytes,
-                        message.handles.empty() ? NULL :
-                            reinterpret_cast<MojoHandle*>(&message.handles[0]),
-                        &num_handles,
-                        MOJO_READ_MESSAGE_FLAG_NONE);
+    message.AllocData(num_bytes);
+    message.mutable_handles()->resize(num_handles);
+
+    rv = ReadMessageRaw(
+        message_pipe_.get(),
+        message.mutable_data(),
+        &num_bytes,
+        message.mutable_handles()->empty() ? NULL :
+            reinterpret_cast<MojoHandle*>(&message.mutable_handles()->front()),
+        &num_handles,
+        MOJO_READ_MESSAGE_FLAG_NONE);
     if (rv != MOJO_RESULT_OK) {
       error_ = true;
       break;
diff --git a/mojo/public/bindings/lib/message.cc b/mojo/public/bindings/lib/message.cc
index 308c5b5..56561b3 100644
--- a/mojo/public/bindings/lib/message.cc
+++ b/mojo/public/bindings/lib/message.cc
@@ -4,6 +4,7 @@
 
 #include "mojo/public/bindings/message.h"
 
+#include <assert.h>
 #include <stdlib.h>
 
 #include <algorithm>
@@ -11,22 +12,32 @@
 namespace mojo {
 
 Message::Message()
-    : data(NULL) {
+    : data_(NULL) {
 }
 
 Message::~Message() {
-  free(data);
+  free(data_);
 
-  for (std::vector<Handle>::iterator it = handles.begin(); it != handles.end();
-       ++it) {
+  for (std::vector<Handle>::iterator it = handles_.begin();
+       it != handles_.end(); ++it) {
     if (it->is_valid())
       CloseRaw(*it);
   }
 }
 
+void Message::AllocData(uint32_t num_bytes) {
+  assert(!data_);
+  data_ = static_cast<MessageData*>(malloc(num_bytes));
+}
+
+void Message::AdoptData(MessageData* data) {
+  assert(!data_);
+  data_ = data;
+}
+
 void Message::Swap(Message* other) {
-  std::swap(data, other->data);
-  std::swap(handles, other->handles);
+  std::swap(data_, other->data_);
+  std::swap(handles_, other->handles_);
 }
 
 }  // namespace mojo
diff --git a/mojo/public/bindings/lib/sync_dispatcher.cc b/mojo/public/bindings/lib/sync_dispatcher.cc
index cee4e5b..eee0be4 100644
--- a/mojo/public/bindings/lib/sync_dispatcher.cc
+++ b/mojo/public/bindings/lib/sync_dispatcher.cc
@@ -30,18 +30,17 @@ bool WaitForMessageAndDispatch(MessagePipeHandle handle,
   }
 
   Message message;
-  message.data = static_cast<MessageData*>(malloc(num_bytes));
-  message.handles.resize(num_handles);
-
-  MojoResult rv =
-      ReadMessageRaw(handle,
-                     message.data,
-                     &num_bytes,
-                     message.handles.empty()
-                         ? NULL
-                         : reinterpret_cast<MojoHandle*>(&message.handles[0]),
-                     &num_handles,
-                     MOJO_READ_MESSAGE_FLAG_NONE);
+  message.AllocData(num_bytes);
+  message.mutable_handles()->resize(num_handles);
+
+  MojoResult rv = ReadMessageRaw(
+      handle,
+      message.mutable_data(),
+      &num_bytes,
+      message.mutable_handles()->empty() ? NULL :
+          reinterpret_cast<MojoHandle*>(&message.mutable_handles()->front()),
+      &num_handles,
+      MOJO_READ_MESSAGE_FLAG_NONE);
   if (rv != MOJO_RESULT_OK)
     return false;
   return receiver->Accept(&message);
diff --git a/mojo/public/bindings/message.h b/mojo/public/bindings/message.h
index 5e680f8..fa13515 100644
--- a/mojo/public/bindings/message.h
+++ b/mojo/public/bindings/message.h
@@ -14,6 +14,7 @@ namespace mojo {
 #pragma pack(push, 1)
 
 struct MessageHeader {
+  //uint32_t deprecated_num_bytes;
   uint32_t num_bytes;
   uint32_t name;
 };
@@ -35,13 +36,23 @@ class Message {
   Message();
   ~Message();
 
+  // These may only be called on a newly initialized Message object.
+  void AllocData(uint32_t num_bytes);  // |data()| is uninitialized.
+  void AdoptData(MessageData* data);
+
+  // Swaps data and handles between this Message and another.
   void Swap(Message* other);
 
-  MessageData* data;  // Heap-allocated using malloc.
-  // TODO(davemoore): Turn these into ScopedHandles and fix bindings generation.
-  std::vector<Handle> handles;
+  const MessageData* data() const { return data_; }
+  MessageData* mutable_data() { return data_; }
+
+  const std::vector<Handle>* handles() const { return &handles_; }
+  std::vector<Handle>* mutable_handles() { return &handles_; }
 
  private:
+  MessageData* data_;  // Heap-allocated using malloc.
+  std::vector<Handle> handles_;
+
   MOJO_DISALLOW_COPY_AND_ASSIGN(Message);
 };
 
diff --git a/mojo/public/bindings/tests/connector_unittest.cc b/mojo/public/bindings/tests/connector_unittest.cc
index 1c1181e..8ff7de5 100644
--- a/mojo/public/bindings/tests/connector_unittest.cc
+++ b/mojo/public/bindings/tests/connector_unittest.cc
@@ -52,10 +52,11 @@ class ConnectorTest : public testing::Test {
   void AllocMessage(const char* text, Message* message) {
     size_t payload_size = strlen(text) + 1;  // Plus null terminator.
     size_t num_bytes = sizeof(MessageHeader) + payload_size;
-    message->data = static_cast<MessageData*>(malloc(num_bytes));
-    message->data->header.num_bytes = static_cast<uint32_t>(num_bytes);
-    message->data->header.name = 1;
-    memcpy(message->data->payload, text, payload_size);
+    message->AllocData(static_cast<uint32_t>(num_bytes));
+    message->mutable_data()->header.num_bytes =
+        static_cast<uint32_t>(num_bytes);
+    message->mutable_data()->header.name = 1;
+    memcpy(message->mutable_data()->payload, text, payload_size);
   }
 
   void PumpMessages() {
@@ -92,9 +93,10 @@ TEST_F(ConnectorTest, Basic) {
   Message message_received;
   accumulator.Pop(&message_received);
 
-  EXPECT_EQ(std::string(kText),
-            std::string(
-                reinterpret_cast<char*>(message_received.data->payload)));
+  EXPECT_EQ(
+      std::string(kText),
+      std::string(
+          reinterpret_cast<const char*>(message_received.data()->payload)));
 }
 
 TEST_F(ConnectorTest, Basic_EarlyIncomingReceiver) {
@@ -118,9 +120,10 @@ TEST_F(ConnectorTest, Basic_EarlyIncomingReceiver) {
   Message message_received;
   accumulator.Pop(&message_received);
 
-  EXPECT_EQ(std::string(kText),
-            std::string(
-                reinterpret_cast<char*>(message_received.data->payload)));
+  EXPECT_EQ(
+      std::string(kText),
+      std::string(
+          reinterpret_cast<const char*>(message_received.data()->payload)));
 }
 
 TEST_F(ConnectorTest, Basic_TwoMessages) {
@@ -147,9 +150,10 @@ TEST_F(ConnectorTest, Basic_TwoMessages) {
     Message message_received;
     accumulator.Pop(&message_received);
 
-    EXPECT_EQ(std::string(kText[i]),
-              std::string(
-                  reinterpret_cast<char*>(message_received.data->payload)));
+    EXPECT_EQ(
+        std::string(kText[i]),
+        std::string(
+            reinterpret_cast<const char*>(message_received.data()->payload)));
   }
 }
 
@@ -187,17 +191,17 @@ TEST_F(ConnectorTest, MessageWithHandles) {
 
   const char kText[] = "hello world";
 
-  Message message;
-  AllocMessage(kText, &message);
+  Message message1;
+  AllocMessage(kText, &message1);
 
   ScopedMessagePipeHandle handles[2];
   CreateMessagePipe(&handles[0], &handles[1]);
-  message.handles.push_back(handles[0].release());
+  message1.mutable_handles()->push_back(handles[0].release());
 
-  connector0.Accept(&message);
+  connector0.Accept(&message1);
 
   // The message should have been transferred, releasing the handles.
-  EXPECT_TRUE(message.handles.empty());
+  EXPECT_TRUE(message1.handles()->empty());
 
   MessageAccumulator accumulator;
   connector1.set_incoming_receiver(&accumulator);
@@ -209,24 +213,27 @@ TEST_F(ConnectorTest, MessageWithHandles) {
   Message message_received;
   accumulator.Pop(&message_received);
 
-  EXPECT_EQ(std::string(kText),
-            std::string(
-                reinterpret_cast<char*>(message_received.data->payload)));
-  ASSERT_EQ(1U, message_received.handles.size());
+  EXPECT_EQ(
+      std::string(kText),
+      std::string(
+          reinterpret_cast<const char*>(message_received.data()->payload)));
+  ASSERT_EQ(1U, message_received.handles()->size());
 
   // Now send a message to the transferred handle and confirm it's sent through
   // to the orginal pipe.
   // TODO(vtl): Do we need a better way of "downcasting" the handle types?
   ScopedMessagePipeHandle smph;
-  smph.reset(MessagePipeHandle(message_received.handles[0].value()));
-  message_received.handles[0] = Handle();  // |smph| now owns this handle.
+  smph.reset(MessagePipeHandle(message_received.handles()->front().value()));
+  message_received.mutable_handles()->front() = Handle();
+  // |smph| now owns this handle.
 
   internal::Connector connector_received(smph.Pass());
   internal::Connector connector_original(handles[1].Pass());
 
-  AllocMessage(kText, &message);
+  Message message2;
+  AllocMessage(kText, &message2);
 
-  connector_received.Accept(&message);
+  connector_received.Accept(&message2);
   connector_original.set_incoming_receiver(&accumulator);
   PumpMessages();
 
@@ -234,9 +241,10 @@ TEST_F(ConnectorTest, MessageWithHandles) {
 
   accumulator.Pop(&message_received);
 
-  EXPECT_EQ(std::string(kText),
-            std::string(
-                reinterpret_cast<char*>(message_received.data->payload)));
+  EXPECT_EQ(
+      std::string(kText),
+      std::string(
+          reinterpret_cast<const char*>(message_received.data()->payload)));
 }
 
 }  // namespace test
diff --git a/mojo/public/bindings/tests/sample_service_unittest.cc b/mojo/public/bindings/tests/sample_service_unittest.cc
index a252f1f..b7d8f6c 100644
--- a/mojo/public/bindings/tests/sample_service_unittest.cc
+++ b/mojo/public/bindings/tests/sample_service_unittest.cc
@@ -270,8 +270,8 @@ class SimpleMessageReceiver : public mojo::MessageReceiver {
     // Imagine some IPC happened here.
 
     if (g_dump_message_as_hex) {
-      DumpHex(reinterpret_cast<const uint8_t*>(message->data),
-              message->data->header.num_bytes);
+      DumpHex(reinterpret_cast<const uint8_t*>(message->data()),
+              message->data()->header.num_bytes);
     }
 
     // In the receiving process, an implementation of ServiceStub is known to