Adding a router class to handle messages that expect responses.

mojo/bindings/java/src/org/chromium/mojo/bindings/MessageHeader.java

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.mojo.bindings;
+
+import org.chromium.mojo.bindings.Struct.DataHeader;
+
+/**
+ * Header information for a message.
+ */
+public class MessageHeader {
+
+    private static final int SIMPLE_MESSAGE_SIZE = 16;
+    private static final int MESSAGE_WITH_REQUEST_ID_SIZE = 24;

[rmcilroy, 2014/07/10 19:03:58]

Group common fields and insert newlines to seperate - e.g,: 

   SIMPLE_MESSAGE_SIZE
   MESSAGE_WITH_REQUEST_ID_SIZE
   SIMPLE_MESSAGE_STRUCT_INFO

   MESSAGE_WITH_REQUEST_ID_SIZE
   MESSAGE_WITH_REQUEST_ID_NUM_FIELDS
   MESSAGE_WITH_REQUEST_ID_STRUCT_INFO

   TYPE_OFFSET 
   FLAGS_OFFSET
   REQUEST_ID_OFFSET

[qsr, 2014/07/11 11:42:08]

Done.

+    private static final int SIMPLE_MESSAGE_NUM_FIELDS = 2;
+    private static final int MESSAGE_WITH_REQUEST_ID_NUM_FIELDS = 3;
+    private static final DataHeader SIMPLE_MESSAGE_STRUCT_INFO = new DataHeader(SIMPLE_MESSAGE_SIZE,

[rmcilroy, 2014/07/10 19:03:58]

nit - newline at "new" instead of SIMPLE_MESSAGE_NUM_FIELDS.

[qsr, 2014/07/11 11:42:08]

Done.

+            SIMPLE_MESSAGE_NUM_FIELDS);
+    private static final DataHeader MESSAGE_WITH_REQUEST_ID_STRUCT_INFO = new DataHeader(

[rmcilroy, 2014/07/10 19:03:58]

ditto

[qsr, 2014/07/11 11:42:08]

Done.

+            MESSAGE_WITH_REQUEST_ID_SIZE, MESSAGE_WITH_REQUEST_ID_NUM_FIELDS);
+    private static final int TYPE_OFFSET = 8;
+    private static final int FLAGS_OFFSET = 12;
+    private static final int REQUEST_ID_OFFSET = 16;

[rmcilroy, 2014/07/10 19:03:58]

newline

[qsr, 2014/07/11 11:42:08]

Done.

+    /**
+     * Flag for a header of a message that expected a response.
+     */
+    public static final int MESSAGE_EXPECTS_RESPONSE_FLAG = 1 << 0;
+
+    /**
+     * Flag for a header of a message that is a response.
+     */
+    public static final int MESSAGE_IS_RESPONSE_FLAG = 1 << 1;
+
+    private final DataHeader mDataHeader;
+    private final int mType;
+    private final int mFlags;
+    private long mRequestId;
+
+    /**
+     * Constructor for the header of a message not having a response.

[rmcilroy, 2014/07/10 19:03:58]

/not having/which does not have

[qsr, 2014/07/11 11:42:08]

Done.

+     */
+    public MessageHeader(int type, int flags) {
+        assert !mustHaveRequestId(flags);
+        mDataHeader = SIMPLE_MESSAGE_STRUCT_INFO;
+        mType = type;
+        mFlags = flags;
+        mRequestId = 0;
+    }
+
+    /**
+     * Constructor for the header of a message having a response or being itself a response.

[rmcilroy, 2014/07/10 19:03:58]

ditto

[qsr, 2014/07/11 11:42:07]

Done.

+     */
+    public MessageHeader(int type, int flags, long requestId) {
+        assert mustHaveRequestId(flags);
+        mDataHeader = MESSAGE_WITH_REQUEST_ID_STRUCT_INFO;
+        mType = type;
+        mFlags = flags;
+        mRequestId = requestId;
+    }
+
+    /**
+     * Constructor, parsing the header from a message. Should only be used by {@link Message}
+     * itself.
+     */
+    MessageHeader(Message message) {
+        Decoder decoder = new Decoder(message);
+        mDataHeader = decoder.readDataHeader();
+        if (mDataHeader.numFields < SIMPLE_MESSAGE_NUM_FIELDS) {
+            throw new DeserializationException(
+                    "Incorrect number of fields, expecting at least " + SIMPLE_MESSAGE_NUM_FIELDS
+                    + ", but got: " + mDataHeader.numFields);
+        }
+        if (mDataHeader.size < SIMPLE_MESSAGE_STRUCT_INFO.size) {
+            throw new DeserializationException(
+                    "Incorrect message size, expecting at least " + SIMPLE_MESSAGE_STRUCT_INFO.size
+                    + ", but got: " + mDataHeader.size);
+        }
+        if (mDataHeader.numFields == 2 && mDataHeader.size != SIMPLE_MESSAGE_STRUCT_INFO.size) {

[rmcilroy, 2014/07/10 19:03:58]

/s/2/SIMPLE_MESSAGE_NUM_FIELDS (and below for "3")

[qsr, 2014/07/11 11:42:07]

Done.

+            throw new DeserializationException(
+                    "Incorrect message size, expecting " + SIMPLE_MESSAGE_STRUCT_INFO.size

[rmcilroy, 2014/07/10 19:03:58]

Mention that you got a message with 2 fields and unexpected size (to
differentiate from error below, which should also be updated similarly)

[qsr, 2014/07/11 11:42:08]

Done.

+                    + ", but got: " + mDataHeader.size);
+        }
+        if (mDataHeader.numFields == 3
+                && mDataHeader.size != MESSAGE_WITH_REQUEST_ID_STRUCT_INFO.size) {
+            throw new DeserializationException(
+                    "Incorrect message size, expecting " + MESSAGE_WITH_REQUEST_ID_STRUCT_INFO.size
+                    + ", but got: " + mDataHeader.size);
+        }

[rmcilroy, 2014/07/10 19:03:58]

I think this would be clearer if you factor out the validation from lines 69-89
to a private static helper function.

[qsr, 2014/07/11 11:42:08]

Done.

+        mType = decoder.readInt(TYPE_OFFSET);
+        mFlags = decoder.readInt(FLAGS_OFFSET);
+        if (mustHaveRequestId(mFlags)) {
+            if (mDataHeader.size < MESSAGE_WITH_REQUEST_ID_STRUCT_INFO.size) {
+                throw new DeserializationException("Incorrect message size, expecting at least "

[rmcilroy, 2014/07/10 19:03:58]

Also mention something in the message about why you expect the message to have
this size

[qsr, 2014/07/11 11:42:08]

Done.

+                        + MESSAGE_WITH_REQUEST_ID_STRUCT_INFO.size + ", but got: "
+                        + mDataHeader.size);
+
+            }
+            mRequestId = decoder.readLong(REQUEST_ID_OFFSET);
+        } else {
+            mRequestId = 0;
+        }
+    }
+
+    public int getSize() {
+        return mDataHeader.size;
+    }
+
+    /**
+     * Returns the type of the message.
+     */
+    public int getType() {
+        return mType;
+    }
+
+    /**
+     * Returns the flags associated to the message.
+     */
+    public int getFlags() {
+        return mFlags;
+    }
+
+    /**
+     * Returns if the message has the given flag.
+     */
+    public boolean hasFlag(int flag) {
+        return (mFlags & flag) == flag;
+    }
+
+    /**
+     * Returns if the message has a request id.
+     */
+    public boolean hasRequestId() {
+        return mustHaveRequestId(mFlags);
+    }
+
+    /**
+     * Return the request id for the message. Must only be called if the message has a request id.
+     */
+    public long getRequestId() {
+        assert hasRequestId();
+        return mRequestId;
+    }
+
+    /**
+     * Encode the header.
+     */
+    public void encode(Encoder encoder) {
+        encoder.encode(mDataHeader);
+        encoder.encode(getType(), TYPE_OFFSET);
+        encoder.encode(getFlags(), FLAGS_OFFSET);
+        if (hasRequestId()) {
+            encoder.encode(getRequestId(), REQUEST_ID_OFFSET);
+        }
+    }
+
+    /**
+     * Returns if the header has the expected flags. Only consider flags this class knows about to

[rmcilroy, 2014/07/10 19:03:58]

Returns true if...
/s/consider/considers

[qsr, 2014/07/11 11:42:08]

Done.

+     * allow working with future version of the header format.

[rmcilroy, 2014/07/10 19:03:58]

/s/to allow working/in order to allow this class to work/

[qsr, 2014/07/11 11:42:08]

Done.

+     */
+    public boolean validateHeader(int expectedFlags) {
+        int knownFlags = getFlags() & (MESSAGE_EXPECTS_RESPONSE_FLAG | MESSAGE_IS_RESPONSE_FLAG);
+        return knownFlags == expectedFlags;
+    }
+
+    /**
+     * Returns if the header has the expected type and flags. Only consider flags this class knows
+     * about to allow working with future version of the header format.

[rmcilroy, 2014/07/10 19:03:58]

same changes as validateHeader above

[qsr, 2014/07/11 11:42:08]

Done.

+     */
+    public boolean validateHeader(int expectedType, int expectedFlags) {
+        return getType() == expectedType && validateHeader(expectedFlags);
+    }
+
+    /**
+     * Set the given request id on the given message. Should only be called if the message requires
+     * a request id.
+     */
+    public static void setRequestId(Message message, long requestId) {
+        assert message.getHeader().hasRequestId();
+        message.buffer.putLong(REQUEST_ID_OFFSET, requestId);
+        // If the message has a cached header, it is now invalid.
+        message.resetHeader();
+    }
+
+    /**
+     * return whether a message with the given flags must have a request Id.

[rmcilroy, 2014/07/10 19:03:58]

/s/return/Returns

[qsr, 2014/07/11 11:42:08]

Done.

+     */
+    private static boolean mustHaveRequestId(int flags) {
+        return (flags & (MESSAGE_EXPECTS_RESPONSE_FLAG | MESSAGE_IS_RESPONSE_FLAG)) != 0;
+    }
+}