Add ProtoDecoder for sending messages between Host and Controller when pairing

components/pairing/proto_decoder.cc

Index: components/pairing/proto_decoder.cc
diff --git a/components/pairing/proto_decoder.cc b/components/pairing/proto_decoder.cc
new file mode 100644
index 0000000000000000000000000000000000000000..2659bb36d432da3e48c481699bed6b128da99bb9
--- /dev/null
+++ b/components/pairing/proto_decoder.cc
@@ -0,0 +1,204 @@
+// 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.
+
+#include "components/pairing/proto_decoder.h"
+
+#include "components/pairing/message_buffer.h"
+#include "components/pairing/pairing_api.pb.h"
+#include "net/base/io_buffer.h"
+
+namespace {
+enum {
+  MESSAGE_NONE,
+  MESSAGE_HOST_STATUS,
+  MESSAGE_CONFIGURE_HOST,
+  MESSAGE_PAIR_DEVICES,
+  MESSAGE_COMPLETE_SETUP,
+  MESSAGE_ERROR,
+  NUM_MESSAGES,
+};
+}
+
+namespace pairing_chromeos {
+
+ProtoDecoder::ProtoDecoder(Observer* observer)
+    : observer_(observer),
+      message_buffer_(new MessageBuffer),
+      next_message_type_(MESSAGE_NONE),
+      next_message_size_(0) {}

[achuithb, 2014/08/11 22:04:31]

DCHECK(observer);

[Zachary Kuznia, 2014/08/11 23:40:29]

Done.

+
+ProtoDecoder::~ProtoDecoder() {}
+
+bool ProtoDecoder::DecodeIOBuffer(int size,
+                                  scoped_refptr<net::IOBuffer> io_buffer) {
+  // Update the message buffer.
+  message_buffer_->AddIOBuffer(io_buffer, size);
+
+  // If there is no current message, the next byte is the message type.
+  if (next_message_type_ == MESSAGE_NONE) {
+    if (message_buffer_->AvailableBytes() < (int)sizeof(uint8_t)) {

[achuithb, 2014/08/11 22:04:31]

Don't need curly braces.

You should use reinterpret_cast here as well:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Castin...

I'm wondering if the type of AvailableBytes should be size_t?

[Zachary Kuznia, 2014/08/11 23:40:29]

I went back and forth on that as well, but this minimizes type conversion.

+      return true;
+    }
+
+    uint8_t message_type = MESSAGE_NONE;

[achuithb, 2014/08/11 22:04:31]

Why not char instead of uint8_t?

[Zachary Kuznia, 2014/08/11 23:40:29]

Unsigned types have less undefined behavior with type conversions.

+    message_buffer_->ReadBytes(reinterpret_cast<char*>(&message_type),
+                               sizeof(message_type));
+
+    if (message_type == MESSAGE_NONE || message_type >= NUM_MESSAGES) {
+      LOG(ERROR) << "Unknown message type received: " << message_type;
+      return false;
+    }
+    next_message_type_ = message_type;
+  }
+
+  // If the message size isn't set, the next two bytes are the message size.
+  if (next_message_size_ == 0) {
+    if (message_buffer_->AvailableBytes() < (int)sizeof(uint16_t)) {

[achuithb, 2014/08/11 22:04:30]

reinterpret cast, no curly braces, consider changing the return type of
AvailableBytes()

[Zachary Kuznia, 2014/08/11 23:40:29]

Changed to static_cast, removed braces.

I considered changing the return value, but it added more casts elsewhere.

+      return true;
+    }
+
+    // The size is sent in network byte order.
+    uint8_t high_byte = 0;
+    message_buffer_->ReadBytes(reinterpret_cast<char*>(&high_byte),
+                               sizeof(high_byte));
+    uint8_t low_byte = 0;
+    message_buffer_->ReadBytes(reinterpret_cast<char*>(&low_byte),
+                               sizeof(low_byte));
+
+    next_message_size_ = (high_byte << 8) + low_byte;

[achuithb, 2014/08/11 22:04:31]

It feels like hte proto libraries should help us with this stuff? It doesn't
feel right to be fiddling with bits here.

[Zachary Kuznia, 2014/08/11 23:40:29]

See comment at line 190

+  }
+
+  // If the whole proto buffer is not yet available, return early.
+  if (message_buffer_->AvailableBytes() < next_message_size_) {

[achuithb, 2014/08/11 22:04:30]

No curly braces.

[Zachary Kuznia, 2014/08/11 23:40:29]

Done.

+    return true;
+  }
+
+  std::vector<char> buffer(next_message_size_);
+  message_buffer_->ReadBytes(&buffer[0], next_message_size_);
+
+  switch (next_message_type_) {
+    case MESSAGE_HOST_STATUS: {
+        pairing_api::HostStatus message;
+        message.ParseFromArray(&buffer[0], buffer.size());
+        observer_->OnHostStatusMessage(message);
+      }
+      break;
+    case MESSAGE_CONFIGURE_HOST: {
+        pairing_api::ConfigureHost message;
+        message.ParseFromArray(&buffer[0], buffer.size());
+        observer_->OnConfigureHostMessage(message);
+      }
+      break;
+    case MESSAGE_PAIR_DEVICES: {
+        pairing_api::PairDevices message;
+        message.ParseFromArray(&buffer[0], buffer.size());
+        observer_->OnPairDevicesMessage(message);
+      }
+      break;
+    case MESSAGE_COMPLETE_SETUP: {
+        pairing_api::CompleteSetup message;
+        message.ParseFromArray(&buffer[0], buffer.size());
+        observer_->OnCompleteSetupMessage(message);
+      }
+      break;
+    case MESSAGE_ERROR: {
+        pairing_api::Error message;
+        message.ParseFromArray(&buffer[0], buffer.size());
+        observer_->OnErrorMessage(message);
+      }
+      break;
+
+    default:
+      NOTREACHED();
+      break;
+  }
+
+  // Reset the message data.
+  next_message_type_ = MESSAGE_NONE;
+  next_message_size_ = 0;
+
+  return true;
+}
+
+scoped_refptr<net::IOBuffer> ProtoDecoder::SendHostStatus(
+    const pairing_api::HostStatus& message, int* size) {
+  std::string serialized_proto;
+  if (!message.SerializeToString(&serialized_proto)) {

[achuithb, 2014/08/11 22:04:31]

Shouldn't this be a DCHECK?

Here and everywhere below.

[Zachary Kuznia, 2014/08/11 23:40:29]

I avoid putting a function call in any kind of assert, because the calling
semantics can change without notice.

+    NOTREACHED();
+  }
+
+  return SendMessage(MESSAGE_HOST_STATUS, serialized_proto, size);
+}
+
+scoped_refptr<net::IOBuffer> ProtoDecoder::SendConfigureHost(
+    const pairing_api::ConfigureHost& message, int* size) {
+  std::string serialized_proto;
+  if (!message.SerializeToString(&serialized_proto)) {
+    NOTREACHED();
+  }
+
+  return SendMessage(MESSAGE_CONFIGURE_HOST, serialized_proto, size);
+}
+
+scoped_refptr<net::IOBuffer> ProtoDecoder::SendPairDevices(
+    const pairing_api::PairDevices& message, int* size) {
+  std::string serialized_proto;
+  if (!message.SerializeToString(&serialized_proto)) {
+    NOTREACHED();
+  }
+
+  return SendMessage(MESSAGE_PAIR_DEVICES, serialized_proto, size);
+}
+
+scoped_refptr<net::IOBuffer> ProtoDecoder::SendCompleteSetup(
+    const pairing_api::CompleteSetup& message, int* size) {
+  std::string serialized_proto;
+  if (!message.SerializeToString(&serialized_proto)) {
+    NOTREACHED();
+  }
+
+  return SendMessage(MESSAGE_COMPLETE_SETUP, serialized_proto, size);
+}
+
+scoped_refptr<net::IOBuffer> ProtoDecoder::SendError(
+    const pairing_api::Error& message, int* size) {
+  std::string serialized_proto;
+  if (!message.SerializeToString(&serialized_proto)) {
+    NOTREACHED();
+  }
+
+  return SendMessage(MESSAGE_ERROR, serialized_proto, size);
+}
+
+scoped_refptr<net::IOBuffer> ProtoDecoder::SendMessage(
+    uint8_t message_type,
+    const std::string& message,
+    int* size) {

[achuithb, 2014/08/11 22:04:31]

Should this be size_t? You're doing an implicit conversion below.

[Zachary Kuznia, 2014/08/11 23:40:29]

The BluetoothSocket function deals in IOBuffer and int size, so I wanted to keep
type conversions isolated to this class.

+  uint16_t message_size = message.size();
+
+  *size = sizeof(message_type) + sizeof(message_size) + message.size();
+  scoped_refptr<net::IOBuffer> io_buffer(new net::IOBuffer(*size));
+
+  // Write the message type.
+  int offset = 0;
+  memcpy(&io_buffer->data()[offset], &message_type, sizeof(message_type));
+  offset += sizeof(message_type);
+
+  // Network byte order.
+  // Write the high byte of the size.
+  uint8_t data = (message_size >> 8) & 0xFF;

[achuithb, 2014/08/11 22:04:31]

Is there no way of getting protobufs to do this work as well?

[Zachary Kuznia, 2014/08/11 23:40:29]

The other call sites I've seen in Chrome serialize the message size themselves
as well.  Trying to get the protobuffers to handle the type and size looked
possible, but would have been a more complex solution and require more overhead,
so I opted for sticking to reading/writing three bytes.

+  memcpy(&io_buffer->data()[offset], &data, sizeof(data));
+  offset += sizeof(data);
+  // Write the low byte of the size.
+  data = message_size & 0xFF;
+  memcpy(&io_buffer->data()[offset], &data, sizeof(data));
+  offset += sizeof(data);
+
+  // Write the actual message.
+  memcpy(&io_buffer->data()[offset], message.data(), message.size());
+
+  return io_buffer;
+}
+
+}  // namespace pairing_chromeos