Create new class "CastTransport", which encapsulates the message read and write event loops.

extensions/browser/api/cast_channel/cast_transport.h

Index: extensions/browser/api/cast_channel/cast_transport.h
diff --git a/extensions/browser/api/cast_channel/cast_transport.h b/extensions/browser/api/cast_channel/cast_transport.h
new file mode 100644
index 0000000000000000000000000000000000000000..a18c09acb636f8716728d46da38eb664accfd43a
--- /dev/null
+++ b/extensions/browser/api/cast_channel/cast_transport.h
@@ -0,0 +1,182 @@
+// 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.
+
+#ifndef EXTENSIONS_BROWSER_API_CAST_CHANNEL_CAST_TRANSPORT_H_
+#define EXTENSIONS_BROWSER_API_CAST_CHANNEL_CAST_TRANSPORT_H_
+
+#include <queue>
+#include <string>
+
+#include "base/memory/ref_counted.h"
+#include "base/threading/thread_checker.h"
+#include "extensions/common/api/cast_channel.h"
+#include "net/base/completion_callback.h"
+
+namespace net {
+class DrainableIOBuffer;
+class IPEndPoint;
+class IOBuffer;
+class DrainableIOBuffer;
+class GrowableIOBuffer;
+}  // namespace net
+
+namespace extensions {
+namespace core_api {
+namespace cast_channel {
+class CastMessage;
+struct LastErrors;
+class Logger;
+class MessageFramer;
+
+// TODO(kmarshall): Migrate CastSocket to new interface.
+// Skeletal placeholder for a future implementation of CastSocket.
+// New to this interface are read/write calls that operate on simple buffers.
+// Methods that operate on CastMessage/MessageInfo objects will be removed.
+class CastSocketPlaceholder {

[mark a. foltz, 2014/09/12 19:41:01]

ISTM that this is a useful abstraction for unit testing - even after the real
implementation is complete.  Rename to CastSocketInterface?

[Wez, 2014/09/12 19:59:59]

Seconded.

[Kevin M, 2014/09/12 20:22:25]

Done.

[Kevin M, 2014/09/12 20:22:25]

Done.

+ public:
+  CastSocketPlaceholder() {}
+  virtual ~CastSocketPlaceholder() {}
+
+  virtual int Write(net::IOBuffer* buffer,
+                    size_t size,
+                    const net::CompletionCallback& callback) = 0;
+  virtual int Read(net::IOBuffer* buf,
+                   int buf_len,
+                   const net::CompletionCallback& callback) = 0;

[Wez, 2014/09/12 20:23:04]

Clarify the semantics of these; in particular Write() returns net::OK iff all of
|size| bytes of |buffer| were written, in contrast to the usual write semantics
of code in net::

[Kevin M, 2014/09/12 21:26:23]

Sorry, I don't follow. The sync unit tests exercise the latter functionality.
When Write() returns a positive value it means that some number of bytes were
written synchronously to the socket. Returning a net::OK vs. error makes it
impossible for the write state machine to follow through with partial writes.

  EXPECT_CALL(mock_socket_, Write(NotNull(), serialized_message.size(), _))
      .WillOnce(DoAll(ReadBufferToString<0, 1>(&output),
                      Return(serialized_message.size())));

[Wez, 2014/09/19 00:01:32]

My reading of the impl was that CastSocket::Write() always either succeeds to
wrie *all* the data, or returns an error; didn't realise it still returned after
partial writes.

+  virtual void CloseWithError(ChannelError error) = 0;
+  virtual const net::IPEndPoint& ip_endpoint() const = 0;
+  virtual ChannelAuthType channel_auth() const = 0;
+  virtual int id() const = 0;
+};
+
+// Manager class for reading and writing messages to/from a CastSocket.
+class CastTransport {
+ public:
+  // Object to be informed of incoming messages and errors.
+  class Delegate {
+   public:
+    // An error occurred on the channel. |last_errors| contains the last errors
+    // logged for the channel from the implementation.
+    virtual void OnError(const CastSocketPlaceholder* socket,
+                         ChannelError error_state,
+                         const LastErrors& last_errors) = 0;
+    // A message was received on the channel.
+    virtual void OnMessage(const CastSocketPlaceholder* socket,
+                           const CastMessage& message) = 0;
+
+   protected:
+    virtual ~Delegate() {}
+  };
+
+  CastTransport(CastSocketPlaceholder* socket,
+                Delegate* read_delegate,
+                scoped_refptr<Logger> logger);
+  virtual ~CastTransport();
+
+  // Sends a CastMessage to |socket_|.
+  // |message|: The message to send.
+  // |callback|: Callback to be invoked when the write operation has finished.
+  void SendMessage(const CastMessage& message,
+                   const net::CompletionCallback& callback);
+
+  // Starts reading messages from |socket_|.
+  void StartReadLoop();
+
+  // Internal write states.

[Wez, 2014/09/12 19:59:59]

If these are internal, why aren't they protected/private?

[Kevin M, 2014/09/12 20:22:25]

They need to be visible to the ...ToProto() family of functions. Those functions
reference enum types from logging.pb.h that, for reasons mysterious to me, I
can't #include here. Which means I can't make the conversion functions private
to this class nor set up friend relationships with them.

If you have a sec I'd love it if you had an idea as to *why* I can't include
them. I'm assuming it's some sort of build order/build phase technicality that
I'm unaware of.

[Wez, 2014/09/12 20:25:05]

OK, mail me the error you get if you do the include there, off-CL?

[Kevin M, 2014/09/12 21:26:23]

Done, sent.

[Kevin M, 2014/09/16 21:23:09]

Resolved in another CL, 576483003.

+  enum WriteState {
+    WRITE_STATE_NONE,
+    WRITE_STATE_WRITE,
+    WRITE_STATE_WRITE_COMPLETE,
+    WRITE_STATE_DO_CALLBACK,
+    WRITE_STATE_ERROR,
+  };
+
+  // Internal read states.
+  enum ReadState {
+    READ_STATE_NONE,
+    READ_STATE_READ,
+    READ_STATE_READ_COMPLETE,
+    READ_STATE_DO_CALLBACK,
+    READ_STATE_ERROR,
+  };
+
+ private:
+  // Holds a message to be written to the socket. |callback| is invoked when the
+  // message is fully written or an error occurrs.
+  struct WriteRequest {
+    explicit WriteRequest(const std::string& namespace_,
+                          const std::string& payload,
+                          const net::CompletionCallback& callback);
+    ~WriteRequest();
+
+    // Namespace of the serialized message.
+    std::string message_namespace;
+    // Write completion callback, invoked when the operation has completed or
+    // failed.
+    net::CompletionCallback callback;
+    // Buffer with outgoing data.
+    scoped_refptr<net::DrainableIOBuffer> io_buffer;
+  };
+  // Terminates all in-flight write callbacks with error code ERR_FAILED.

[Wez, 2014/09/12 19:59:59]

nit: Personally I hate code that squishes things together like this; I prefer
blank lines to precede comments, in general, to clearly separate them from the
preceding code for readability (applies here & below).

[Kevin M, 2014/09/12 20:22:25]

OK. I like more whitespace too, but previous teams' coding styles forced me to
change my habits. Done.

[Wez, 2014/09/12 20:25:05]

New team, new habits. :D

+  void FlushWriteQueue();
+
+  /////////////////////////////////////////////////////////////////////////////
+  // Following methods work together to implement write flow.

[Wez, 2014/09/12 19:59:59]

This seems implicit from the naming, and the comments on the methods, so
consider removing this (same for reads).

[Kevin M, 2014/09/12 20:22:25]

Done.

+  //
+  // Main method that performs write flow state transitions.
+  void DoWriteLoop(int result);

[Wez, 2014/09/12 19:59:59]

nit: Consider calling this OnWriteResult and similarly, below, for read?

[Kevin M, 2014/09/12 20:22:25]

Done.

+  // Each of the below Do* method is executed in the corresponding
+  // write state. For example when write state is WRITE_STATE_WRITE_COMPLETE
+  // DowriteComplete is called, and so on.
+  int DoWrite();
+  int DoWriteComplete(int result);
+  int DoWriteCallback();
+  int DoWriteError(int result);
+  /////////////////////////////////////////////////////////////////////////////
+
+  /////////////////////////////////////////////////////////////////////////////
+  // Following methods work together to implement read flow.
+  //
+  // Main method that performs write flow state transitions.
+  void DoReadLoop(int result);
+  // Each of the below Do* method is executed in the corresponding
+  // write state. For example when write state is READ_STATE_READ_COMPLETE
+  // DoReadComplete is called, and so on.
+  int DoRead();
+  int DoReadComplete(int result);
+  int DoReadCallback();
+  int DoReadError(int result);
+
+  void SetReadState(ReadState read_state);
+  void SetWriteState(WriteState write_state);
+  void SetErrorState(ChannelError error_state);
+
+  // Queue of pending writes. The message at the front of the queue is the one
+  // being written.
+  std::queue<WriteRequest> write_queue_;
+  // Buffer used for read operations. Reused for every read.
+  scoped_refptr<net::GrowableIOBuffer> read_buffer_;
+  // Constructs and parses the wire representation of message frames.
+  scoped_ptr<MessageFramer> framer_;
+  // Last message received on the socket.
+  scoped_ptr<CastMessage> current_message_;
+  // Socket used for I/O operations.
+  CastSocketPlaceholder* socket_;
+  // Methods for communicating message receipt and error status to client code.
+  Delegate* read_delegate_;
+  // Write flow state machine state.
+  WriteState write_state_;
+  // Read flow state machine state.
+  ReadState read_state_;
+  // Most recent error that occurred during read or write operation, if any.
+  ChannelError error_state_;
+
+  scoped_refptr<Logger> logger_;
+  base::ThreadChecker thread_checker_;
+};

[Wez, 2014/09/12 19:59:59]

DISALLOW_COPY_AND_ASSIGN

[Kevin M, 2014/09/12 20:22:25]

Done.

+}  // namespace cast_channel
+}  // namespace core_api
+}  // namespace extensions
+
+#endif  // EXTENSIONS_BROWSER_API_CAST_CHANNEL_CAST_TRANSPORT_H_