Create dedicated class for handling wire message formatting and parsing.

extensions/browser/api/cast_channel/cast_socket.h

Index: extensions/browser/api/cast_channel/cast_socket.h
diff --git a/extensions/browser/api/cast_channel/cast_socket.h b/extensions/browser/api/cast_channel/cast_socket.h
index 1de7ac3717430e5d9f0f6767f1e0da1abe14dc17..04bbaa7e9d2136e93f694a76a4a69887ff0f810c 100644
--- a/extensions/browser/api/cast_channel/cast_socket.h
+++ b/extensions/browser/api/cast_channel/cast_socket.h
@@ -38,6 +38,7 @@ namespace cast_channel {
 class CastMessage;
 class Logger;
 struct LastErrors;
+class PacketFramer;
 
 // This class implements a channel between Chrome and a Cast device using a TCP
 // socket with SSL.  The channel may authenticate that the receiver is a genuine
@@ -155,30 +156,6 @@ class CastSocket : public ApiResource {
     READ_STATE_ERROR,
   };
 
- protected:
-  // Message header struct. If fields are added, be sure to update
-  // header_size().  Protected to allow use of *_size() methods in unit tests.
-  struct MessageHeader {
-    MessageHeader();
-    // Sets the message size.
-    void SetMessageSize(size_t message_size);
-    // Prepends this header to |str|.
-    void PrependToString(std::string* str);
-    // Reads |header| from the beginning of |buffer|.
-    static void ReadFromIOBuffer(net::GrowableIOBuffer* buffer,
-                                 MessageHeader* header);
-    // Size (in bytes) of the message header.
-    static uint32 header_size() { return sizeof(uint32); }
-
-    // Maximum size (in bytes) of a message payload on the wire (does not
-    // include header).
-    static uint32 max_message_size() { return 65536; }
-
-    std::string ToString();
-    // The size of the following protocol message in bytes, in host byte order.
-    uint32 message_size;
-  };
-
  private:
   friend class ApiResourceManager<CastSocket>;
   friend class CastSocketTest;
@@ -266,12 +243,6 @@ class CastSocket : public ApiResource {
   void PostTaskToStartConnectLoop(int result);
   void PostTaskToStartReadLoop();
   void StartReadLoop();
-  // Parses the contents of header_read_buffer_ and sets current_message_size_
-  // to the size of the body of the message.
-  bool ProcessHeader();
-  // Parses the contents of body_read_buffer_ and sets current_message_ to
-  // the message received.
-  bool ProcessBody();
   // Closes socket, signaling the delegate that |error| has occurred.
   void CloseWithError();
   // Frees resources and cancels pending callbacks.  |ready_state_| will be set
@@ -308,15 +279,8 @@ class CastSocket : public ApiResource {
   Delegate* delegate_;
 
   // IOBuffer for reading the message header.
-  scoped_refptr<net::GrowableIOBuffer> header_read_buffer_;
-  // IOBuffer for reading the message body.
-  scoped_refptr<net::GrowableIOBuffer> body_read_buffer_;
-  // IOBuffer to currently read into.
-  scoped_refptr<net::GrowableIOBuffer> current_read_buffer_;
-  // The number of bytes in the current message body.
-  uint32 current_message_size_;
-  // Last message received on the socket.
-  scoped_ptr<CastMessage> current_message_;
+  scoped_refptr<net::GrowableIOBuffer> read_buffer_;
+  scoped_ptr<PacketFramer> framer_;
 
   // The NetLog for this service.
   net::NetLog* net_log_;
@@ -355,6 +319,8 @@ class CastSocket : public ApiResource {
   // canceled.
   bool is_canceled_;
 
+  scoped_ptr<CastMessage> current_message_;
+
   // Connection flow state machine state.
   ConnectionState connect_state_;
   // Write flow state machine state.
@@ -401,6 +367,71 @@ class CastSocket : public ApiResource {
   DISALLOW_COPY_AND_ASSIGN(CastSocket);
 };
 
+// Class for constructing and parsing CastMessage packet data.
+class PacketFramer {

[mark a. foltz, 2014/08/26 20:37:03]

nit: The underlying network protocol (i.e. TCP) provides framing of individual
packets.  Here we're dealing with how to frame messages.  So I might call this
MessageFramer.

[Kevin M, 2014/08/27 01:14:03]

Done.

+ public:
+  explicit PacketFramer(scoped_refptr<net::GrowableIOBuffer> buffer);

[mark a. foltz, 2014/08/26 20:37:03]

s/buffer/input_buffer/?

[Kevin M, 2014/08/27 01:14:03]

Done.

+  ~PacketFramer();
+
+  // The number of bytes to be read from a socket and provided via Ingest(...).

[mark a. foltz, 2014/08/26 20:37:02]

nit: The framer should be independent of the mechanism to read/write the data. 
So I might say "The number of bytes requested from the incoming data stream."

[Kevin M, 2014/08/27 01:14:03]

Done.

+  size_t BytesRequested();
+
+  // Constructs a packet with the serialized form of |message_proto| in
+  // |message_data|.
+  // Returns true if the message was serialized successfully, false otherwise.
+  static bool Serialize(const CastMessage& message_proto,

[mark a. foltz, 2014/08/26 20:37:03]

It seems like the caller would want to pass in a buffer to write to, so it could
then turn around and pass that buffer to socket.Write().

So would this be  

size_t Serialize(const CastMessage& message_proto, scoped_refptr<net::IOBUffer>
output_buffer)

returning the bytes written, or -1 if an error?

[Kevin M, 2014/08/27 01:14:04]

Hmmm. My understanding is that this approach would require the code to flop
between different two different buffer types. First, I'd use a StringIOBuffer or
GrowableIOBuffer for filling the buffer. Then I'd need to wrap it in a
DrainableIOBuffer for writing the data to the socket. This seems less elegant,
or am I missing something?

+                        std::string* message_data);
+
+  // Collects data from an incoming data stream and assembles CastMessage
+  // objects.

[mark a. foltz, 2014/08/26 20:37:02]

This documentation is a little confusing since it takes a CastMessage, not raw
data read from the socket.

[Kevin M, 2014/08/27 01:14:03]

It LGTM, but I tweaked the wording a bit.

+  //
+  // num_bytes: the number of bytes received by a read operation.

[mark a. foltz, 2014/08/26 20:37:03]

The framer could keep track of the previous offset in |buffer| and compute the
number of bytes appended since the last call to ParseBytes/Ingest.

[Kevin M, 2014/08/27 01:14:04]

Socket reads don't not advance the offset; either the framer or the CastSocket
code needs to do that. I chose to modify the offset in the framer for better
encapsulation.

+  //            Value must be <= BytesRequested().

[mark a. foltz, 2014/08/26 20:37:03]

What happens if this isn't true?

[Kevin M, 2014/08/27 01:14:03]

CHECK failure, because it upsets one of the assumed invariants of socket reading
behavior. Too harsh?

+  // message: a pointer to the deserialized message object.
+  // message_length: size of the deserialized message object, in bytes. For
+  //                 logging purposes.
+  // error: the status of the ingest operation.
+  // return value: true if a CastMessage was fully parsed, false otherwise.
+  bool Ingest(uint32 num_bytes,

[mark a. foltz, 2014/08/26 20:37:03]

Perhaps this would be  

size_t ParseBytes(CastMessage** message, size_t* message_size, ChannelError*
error)

that sets *message to a new CastMessage on read of a completed message with size
*message_size, sets *error on an error, or otherwise returns the number of bytes
left to read?

[mark a. foltz, 2014/08/26 20:37:03]

s/uint32/size_t/?

[Kevin M, 2014/08/27 01:14:04]

How about this?

A size_t return value would still need to be saved as an instance variable under
CastSocket for the state machine to pick up later. A bit more fragile than the
current approach of querying BytesRequested(), IMO.

[Kevin M, 2014/08/27 01:14:04]

Done.

+              CastMessage* message,
+              size_t* message_length,
+              ChannelError* error);
+
+  // Message header struct. If fields are added, be sure to update
+  // header_size().  Protected to allow use of *_size() methods in unit tests.
+  //
+  // MessageHeader is public visiblefor testing purposes.

[mark a. foltz, 2014/08/26 20:37:02]

Need space before 'for'

[Kevin M, 2014/08/27 01:14:04]

Done.

+  struct MessageHeader {
+    MessageHeader();
+    // Sets the message size.
+    void SetMessageSize(size_t message_size);
+    // Prepends this header to |str|.
+    void PrependToString(std::string* str);
+    // Reads |header| from the bytes specified by |data|.
+    static void Deserialize(char* data, MessageHeader* header);
+    // Size (in bytes) of the message header.
+    static uint32 header_size();

[mark a. foltz, 2014/08/26 20:37:02]

Let's use size_t consistently in this class and do safe casts to deal with the
net:: APIs that take uint32.

[Kevin M, 2014/08/27 01:14:04]

Done.

+    // Maximum size (in bytes) of a message payload on the wire (does not
+    // include header).
+    static uint32 max_message_size();
+    std::string ToString();
+    // The size of the following protocol message in bytes, in host byte order.
+    uint32 message_size;
+  };
+
+ private:
+  enum PacketElement { HEADER, BODY };

[mark a. foltz, 2014/08/26 20:37:03]

MessageElement?

[Kevin M, 2014/08/27 01:14:03]

Done.

+
+  // Prepares the framer for receiving the next packet.

[mark a. foltz, 2014/08/26 20:37:03]

The framer doesn't receive packets directly?

[Kevin M, 2014/08/27 01:14:04]

Done.

+  void Reset();
+
+  PacketElement current_element_;

[mark a. foltz, 2014/08/26 20:37:03]

Please add documentation for each of the class fields.

[Kevin M, 2014/08/27 01:14:04]

Done.

+  size_t packet_bytes_read_;

[mark a. foltz, 2014/08/26 20:37:03]

message_bytes_read_?

[Kevin M, 2014/08/27 01:14:04]

Done.

+  size_t message_size_;
+  scoped_refptr<net::GrowableIOBuffer> buffer_;
+
+  DISALLOW_COPY_AND_ASSIGN(PacketFramer);
+};
 }  // namespace cast_channel
 }  // namespace core_api
 }  // namespace extensions