Refactor CastSocket code for the following:

chrome/browser/extensions/api/cast_channel/cast_socket.h

Index: chrome/browser/extensions/api/cast_channel/cast_socket.h
===================================================================
--- chrome/browser/extensions/api/cast_channel/cast_socket.h	(revision 236353)
+++ chrome/browser/extensions/api/cast_channel/cast_socket.h	(working copy)
@@ -16,6 +16,7 @@
 #include "base/threading/thread_checker.h"
 #include "chrome/browser/extensions/api/api_resource.h"
 #include "chrome/browser/extensions/api/api_resource_manager.h"
+#include "chrome/browser/extensions/api/cast_channel/cast_channel.pb.h"
 #include "chrome/common/extensions/api/cast_channel.h"
 #include "net/base/completion_callback.h"
 #include "net/base/io_buffer.h"
@@ -27,6 +28,7 @@
 class AddressList;
 class CertVerifier;
 class SSLClientSocket;
+class StreamSocket;
 class TCPClientSocket;
 class TransportSecurityState;
 }
@@ -35,8 +37,6 @@
 namespace api {
 namespace cast_channel {
 
-class CastMessage;
-
 // Size, in bytes, of the largest allowed message payload on the wire (without
 // the header).
 extern const uint32 kMaxMessageSize;
@@ -54,9 +54,11 @@
   class Delegate {
    public:
     // An error occurred on the channel.
+    // It is fine to delete the socket in this callback.

[Ryan Sleevi, 2013/12/11 08:14:14]

Design nit: Within net/, the delegates tend not to take the individual socket.
It's typically up to the "embedder" to handle memoization.

This is important, because as this code is currently written, it creates an
ownership ambiguity situation that should not exist. The CastSocket does not own
itself, but it "implies" that by passing itself to the Delegate and suggesting
it's ok to delete.

Instead, we make Delegates handle memoization (eg: OnError(ChannelError error) /
OnMessage(const MessageInfo& message) ). This makes it clear it's the Delegate's
responsibility to handle ownership and lifetime responsibly.

[Munjal (Google), 2013/12/11 23:39:41]

I agree with your feedback. Since this change requires chagnes to the embedder,
I will make this change separately.

     virtual void OnError(const CastSocket* socket,
                          ChannelError error) = 0;
-    // A string message was received on the channel.
+    // A message was received on the channel.
+    // Do NOT delete the socket in this callback.
     virtual void OnMessage(const CastSocket* socket,
                            const MessageInfo& message) = 0;
    protected:
@@ -86,21 +88,31 @@
   // Returns the state of the channel.
   ReadyState ready_state() const { return ready_state_; }
 
-  // Returns the last error that occurred on this channel, or CHANNEL_ERROR_NONE
-  // if no error has occurred.
+  // Returns the last error that occurred on this channel, or
+  // CHANNEL_ERROR_NONE if no error has occurred.
   ChannelError error_state() const { return error_state_; }
 
   // Connects the channel to the peer. If successful, the channel will be in
   // READY_STATE_OPEN.
+  // It is fine to delete the CastSocket object in |callback|.
   virtual void Connect(const net::CompletionCallback& callback);
 
   // Sends a message over a connected channel. The channel must be in
   // READY_STATE_OPEN.
+  //
+  // Note that if an error occurs the following happens:
+  // 1. Completion callbacks for all pending writes are invoked with error.
+  // 2. Delegate::OnError is called once.
+  // 3. Castsocket is closed.
+  //
+  // DO NOT delete the CastSocket object in write completion callback.
+  // But it is fine to delete the socket in Delegate::ONError

[Ryan Sleevi, 2013/12/11 08:14:14]

Typo (ONError = OnError)

[Munjal (Google), 2013/12/11 23:39:41]

Done.

   virtual void SendMessage(const MessageInfo& message,
                            const net::CompletionCallback& callback);
 
   // Closes the channel. On completion, the channel will be in
   // READY_STATE_CLOSED.
+  // It is fine to delete the CastSocket object in |callback|.
   virtual void Close(const net::CompletionCallback& callback);
 
   // Fills |channel_info| with the status of this channel.
@@ -109,16 +121,15 @@
  protected:

[Ryan Sleevi, 2013/12/11 08:14:14]

Why are these methods protected?

Seems like they should be private. Am I missing something?

[Munjal (Google), 2013/12/11 23:39:41]

They are overridden in tests to inject mock results.

But changed CalledOnValidThread to be private.

[Ryan Sleevi, 2013/12/12 00:48:14]

If they are overridden in tests, they can be private but virtual.

There's no need to make them protected-virtual if your goal is to have your mock
stub in results.

   // Creates an instance of TCPClientSocket.
   virtual scoped_ptr<net::TCPClientSocket> CreateTcpSocket();
-  // Creates an instance of SSLClientSocket.
-  virtual scoped_ptr<net::SSLClientSocket> CreateSslSocket();
+  // Creates an instance of SSLClientSocket with the given underlying |socket|.
+  virtual scoped_ptr<net::SSLClientSocket> CreateSslSocket(
+      scoped_ptr<net::StreamSocket> socket);
+  // Returns IPEndPoint for the URL to connect to.
+  const net::IPEndPoint& ip_endpoint() const { return ip_endpoint_; }
   // Extracts peer certificate from SSLClientSocket instance when the socket
   // is in cert error state.
   // Returns whether certificate is successfully extracted.
   virtual bool ExtractPeerCert(std::string* cert);
-  // Sends a challenge request to the receiver.
-  virtual int SendAuthChallenge();
-  // Reads auth challenge reply from the receiver.
-  virtual int ReadAuthChallengeReply();
   // Verifies whether the challenge reply received from the peer is valid:
   // 1. Signature in the reply is valid.
   // 2. Certificate is rooted to a trusted CA.
@@ -147,6 +158,24 @@
     CONN_STATE_AUTH_CHALLENGE_REPLY_COMPLETE,
   };
 
+  // Internal write states.
+  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,
+  };
+
   /////////////////////////////////////////////////////////////////////////////
   // Following methods work together to implement the following flow:
   // 1. Create a new TCP socket and connect to it
@@ -157,9 +186,9 @@
   // 5. If SSL socket is connected successfully, and if protocol is casts://
   //    then issue an auth challenge request.
   // 6. Validate the auth challenge response.
-
+  //
   // Main method that performs connection state transitions.
-  int DoConnectLoop(int result);
+  void DoConnectLoop(int result);
   // Each of the below Do* method is executed in the corresponding
   // connection state. For e.g. when connection state is TCP_CONNECT
   // DoTcpConnect is called, and so on.
@@ -172,49 +201,61 @@
   int DoAuthChallengeReplyComplete(int result);
   /////////////////////////////////////////////////////////////////////////////
 
-  // Callback method for callbacks from underlying sockets.
-  void OnConnectComplete(int result);
+  /////////////////////////////////////////////////////////////////////////////
+  // Following methods work together to implement write flow.
+  //
+  // Main method that performs write flow state transitions.
+  void DoWriteLoop(int result);
+  // Each of the below Do* method is executed in the corresponding
+  // write state. For e.g. when write state is WRITE_STATE_WRITE_COMPLETE

[Ryan Sleevi, 2013/12/11 08:14:14]

For example or e.g. , but not For e.g.

[Munjal (Google), 2013/12/11 23:39:41]

Done.

+  // DowriteComplete is called, and so on.
+  int DoWrite();
+  int DoWriteComplete(int result);
+  int DoWriteCallback();
+  int DoWriteError(int result);
+  /////////////////////////////////////////////////////////////////////////////
 
-  // Callback method when a challenge request is sent or a reply is received.
-  void OnChallengeEvent(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 e.g. 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);
+  /////////////////////////////////////////////////////////////////////////////
 
   // Runs the external connection callback and resets it.
   void DoConnectCallback(int result);
-
   // Verifies that the URL is a valid cast:// or casts:// URL and sets url_ to
   // the result.
   bool ParseChannelUrl(const GURL& url);
-
-  // Sends the given |message| and invokes the given callback when done.
-  int SendMessageInternal(const CastMessage& message,
-                          const net::CompletionCallback& callback);
-
-  // Writes data to the socket from the WriteRequest at the head of the queue.
-  // Calls OnWriteData() on completion.
-  int WriteData();
-  void OnWriteData(int result);
-
-  // Reads data from the socket into one of the read buffers. Calls
-  // OnReadData() on completion.
-  int ReadData();
-  void OnReadData(int result);
-
-  // Processes the contents of header_read_buffer_ and returns true on success.
+  // Adds |message| to the write queue and starts the write loop if needed.
+  void SendCastMessageInternal(const CastMessage& message,
+                               const net::CompletionCallback& callback);
+  // Posts a task tostart the connect loop with the given |result|.

[Ryan Sleevi, 2013/12/11 08:14:14]

typo: tostart -> to start

That said, these comments seem unnecessary per the style guide (same on 242)

[Munjal (Google), 2013/12/11 23:39:41]

Done.

+  void PostTaskToStartConnectLoop(int result);
+  // Posts a task to start the read loop.
+  void PostTaskToStartReadLoop();
+  // Stars the read loop if not already started.
+  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();
-  // Processes the contents of body_read_buffer_ and returns true on success.
+  // Parses the contents of body_read_buffer_ and sets current_message_ to
+  // the message received.
   bool ProcessBody();
-  // Parses the message held in body_read_buffer_ and notifies |delegate_| if a
-  // message was extracted from the buffer.  Returns true on success.
-  bool ParseMessageFromBody();
-
+  // Closes the socket and sets |error_state_|. Also signals |error| via
+  // |delegate_|.
+  void CloseWithError(ChannelError error);

[Ryan Sleevi, 2013/12/11 08:14:14]

comment nit:

// Closes socket, updating the error state and signaling the delegate that
|error| has occurred.

Design Nit: By having this function responsible for signaling the delegate_, it
makes it hard to at a glance verify that the lifetime semantics are being
observed (namely, that the object may be deleted by the return of this
function).

[Munjal (Google), 2013/12/11 23:39:41]

Done.

Once I make the change you suggest of removing the socket paramter from the
delegate, your (valid) comment here will be addressed automatically.

   // Serializes the content of message_proto (with a header) to |message_data|.
   static bool Serialize(const CastMessage& message_proto,
                         std::string* message_data);
 
-  // Closes the socket and sets |error_state_|. Also signals |error| via
-  // |delegate_|.
-  void CloseWithError(ChannelError error);
-
   base::ThreadChecker thread_checker_;
 
   // The id of the channel.
@@ -228,16 +269,7 @@
   bool auth_required_;
   // The IP endpoint of the peer.
   net::IPEndPoint ip_endpoint_;
-  // The last error encountered by the channel.
-  ChannelError error_state_;
-  // The current status of the channel.
-  ReadyState ready_state_;
 
-  // True when there is a write callback pending.
-  bool write_callback_pending_;
-  // True when there is a read callback pending.
-  bool read_callback_pending_;
-
   // IOBuffer for reading the message header.
   scoped_refptr<net::GrowableIOBuffer> header_read_buffer_;
   // IOBuffer for reading the message body.
@@ -246,14 +278,14 @@
   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.
+  CastMessage current_message_;
 
   // The NetLog for this service.
   net::NetLog* net_log_;
   // The NetLog source for this service.
   net::NetLog::Source net_log_source_;
 
-  // Next connection state to transition to.
-  ConnectionState next_state_;
   // Owned ptr to the underlying TCP socket.
   scoped_ptr<net::TCPClientSocket> tcp_socket_;
   // Owned ptr to the underlying SSL socket.
@@ -269,6 +301,17 @@
   // Callback invoked when the socket is connected.
   net::CompletionCallback connect_callback_;
 
+  // Connection flow state machine state.
+  ConnectionState connect_state_;
+  // Write flow state machine state.
+  WriteState write_state_;
+  // Read flow state machine state.
+  ReadState read_state_;
+  // The last error encountered by the channel.
+  ChannelError error_state_;
+  // The current status of the channel.
+  ReadyState ready_state_;
+
   // Message header struct. If fields are added, be sure to update
   // kMessageHeaderSize in the .cc.
   struct MessageHeader {
@@ -301,9 +344,6 @@
   // being written.
   std::queue<WriteRequest> write_queue_;
 
-  // Used to protect against DoConnectLoop() re-entrancy.
-  bool in_connect_loop_;
-
   FRIEND_TEST_ALL_PREFIXES(CastSocketTest, TestCastURLs);
   FRIEND_TEST_ALL_PREFIXES(CastSocketTest, TestRead);
   FRIEND_TEST_ALL_PREFIXES(CastSocketTest, TestReadMany);