Adding a new message type to the Mojo channel.

mojo/edk/system/channel.h

Index: mojo/edk/system/channel.h
diff --git a/mojo/edk/system/channel.h b/mojo/edk/system/channel.h
index aa6d70c3a2cfa5920ff41f0bc0e71fd9b70d99a3..b8685077d994dc7038d0d1b943ee530520e31113 100644
--- a/mojo/edk/system/channel.h
+++ b/mojo/edk/system/channel.h
@@ -21,7 +21,8 @@ const size_t kChannelMessageAlignment = 8;
 // Channel provides a thread-safe interface to read and write arbitrary
 // delimited messages over an underlying I/O channel, optionally transferring
 // one or more platform handles in the process.
-class Channel : public base::RefCountedThreadSafe<Channel> {
+class MOJO_SYSTEM_IMPL_EXPORT Channel
+    : public base::RefCountedThreadSafe<Channel> {
  public:
   struct Message;
 
@@ -29,42 +30,53 @@ class Channel : public base::RefCountedThreadSafe<Channel> {
 
   // A message to be written to a channel.
   struct Message {

[Ken Rockot(use gerrit already), 2017/02/23 22:07:05]

You also need an EXPORT macro on the nested types you reference in tests.

[Jay Civelli, 2017/02/27 17:07:03]

Yes, that's the only other one used in the test I think.

-#pragma pack(push, 1)
-    struct Header {
-      enum class MessageType : uint16_t {
-        // A normal message.
-        NORMAL = 0,
+    enum class MessageType : uint16_t {
+      // A normal message.

[Ken Rockot(use gerrit already), 2017/02/23 22:07:05]

nit: update comment?

[Jay Civelli, 2017/02/27 17:07:03]

Done.

+      NORMAL_LEGACY = 0,
 #if defined(OS_MACOSX)
-        // A control message containing handles to echo back.
-        HANDLES_SENT,
-        // A control message containing handles that can now be closed.
-        HANDLES_SENT_ACK,
+      // A control message containing handles to echo back.
+      HANDLES_SENT,
+      // A control message containing handles that can now be closed.
+      HANDLES_SENT_ACK,
 #endif
-      };
+      NORMAL_VERSIONED,

[Ken Rockot(use gerrit already), 2017/02/23 22:07:05]

nit: NORMAL is probably fine if we call the old one NORMAL_LEGACY
nit: add comment?

[Jay Civelli, 2017/02/27 17:07:03]

Done.

+    };
 
+#pragma pack(push, 1)
+    // Old message wire format for ChromeOS and Android, used by NORMAL_LEGACY
+    // messages.
+    // TODO: Remove once Arc++ uses versioned messages.
+    struct LegacyHeader {
       // Message size in bytes, including the header.
       uint32_t num_bytes;
 
-#if defined(MOJO_EDK_LEGACY_PROTOCOL)
-      // Old message wire format for ChromeOS and Android.
       // Number of attached handles.
       uint16_t num_handles;
 
       MessageType message_type;
-#else
+    };
+
+    // Header used by NORMAL_VERSIONED messages.
+    // To preserve backward compatibility with LegacyHeader, the num_bytes and
+    // message_type field should be at the same offset than in LegacyHeader.

[Ken Rockot(use gerrit already), 2017/02/23 22:07:05]

nit: s/should be/must be/

Also please add a notice to increment kMessageCurrentVersion in channel.cc when
changing this structure, and note that such changes may only augment the data
structure (i.e. we can never delete or rearrange existing fields.) We may
someday wish to violate that policy but until we need to, this grants us a
reasonable level of flexibility while keeping backwards-compatibility relatively
simple.

[Jay Civelli, 2017/02/27 17:07:03]

Done.

+    struct VersionedHeader {
+      // Message size in bytes, including the header.
+      uint32_t num_bytes;
+
       // Total size of header, including extra header data (i.e. HANDLEs on
       // windows).
       uint16_t num_header_bytes;
 
+      MessageType message_type;
+
+      uint16_t version_number;
+
       // Number of attached handles. May be less than the reserved handle
       // storage size in this message on platforms that serialise handles as
       // data (i.e. HANDLEs on Windows, Mach ports on OSX).
       uint16_t num_handles;
 
-      MessageType message_type;
-
-      char padding[6];
-#endif  // defined(MOJO_EDK_LEGACY_PROTOCOL)
+      char padding[4];
     };
 
 #if defined(OS_MACOSX) && !defined(OS_IOS)
@@ -106,7 +118,7 @@ class Channel : public base::RefCountedThreadSafe<Channel> {
     // |payload_size| bytes plus a header, plus |max_handles| platform handles.
     Message(size_t payload_size,
             size_t max_handles,
-            Header::MessageType message_type = Header::MessageType::NORMAL);
+            MessageType message_type = MessageType::NORMAL_LEGACY);

[Ken Rockot(use gerrit already), 2017/02/23 22:07:05]

nit: I'd prefer to use a delegating constructor rather than a default argument

[Jay Civelli, 2017/02/27 17:07:03]

Done.

 
     ~Message();
 
@@ -116,26 +128,16 @@ class Channel : public base::RefCountedThreadSafe<Channel> {
     const void* data() const { return data_; }
     size_t data_num_bytes() const { return size_; }
 
-#if defined(MOJO_EDK_LEGACY_PROTOCOL)
-    void* mutable_payload() { return static_cast<void*>(header_ + 1); }
-    const void* payload() const {
-      return static_cast<const void*>(header_ + 1);
-    }
-    size_t payload_size() const;
-#else
-    const void* extra_header() const { return data_ + sizeof(Header); }
-    void* mutable_extra_header() { return data_ + sizeof(Header); }
-    size_t extra_header_size() const {
-      return header_->num_header_bytes - sizeof(Header);
-    }
-
-    void* mutable_payload() { return data_ + header_->num_header_bytes; }
-    const void* payload() const { return data_ + header_->num_header_bytes; }
+    const void* extra_header() const;
+    void* mutable_extra_header();
+    size_t extra_header_size() const;
+
+    void* mutable_payload();
+    const void* payload() const;
     size_t payload_size() const;
-#endif  // defined(MOJO_EDK_LEGACY_PROTOCOL)
 
-    size_t num_handles() const { return header_->num_handles; }
-    bool has_handles() const { return header_->num_handles > 0; }
+    size_t num_handles() const;
+    bool has_handles() const;
 #if defined(OS_MACOSX) && !defined(OS_IOS)
     bool has_mach_ports() const;
 #endif
@@ -161,11 +163,14 @@ class Channel : public base::RefCountedThreadSafe<Channel> {
                                PlatformHandleVector* handles);
 #endif
 
+    void SetVersionForTest(uint16_t version_number);
+
    private:
     size_t size_;
     size_t max_handles_;
     char* data_;
-    Header* header_;
+    LegacyHeader* legacy_header_ = nullptr;

[Ken Rockot(use gerrit already), 2017/02/23 22:07:05]

nit: use an anonymous union for legacy_header_ and versioned_header_ storage?

[Jay Civelli, 2017/02/27 17:07:03]

I use the fact that versioned_header_ is not null to figure out if I am in
legacy or not mode. I could not do it if this was unionized, right?

+    VersionedHeader* versioned_header_ = nullptr;
 
     ScopedPlatformHandleVectorPtr handle_vector_;
 
@@ -270,7 +275,7 @@ class Channel : public base::RefCountedThreadSafe<Channel> {
 
   // Handles a received control message. Returns |true| if the message is
   // accepted, or |false| otherwise.
-  virtual bool OnControlMessage(Message::Header::MessageType message_type,
+  virtual bool OnControlMessage(Message::MessageType message_type,
                                 const void* payload,
                                 size_t payload_size,
                                 ScopedPlatformHandleVectorPtr handles);