[cast_channel] Make CastMessageHandler a CastSocket::Observer instead of CastTransport::Delegate

components/cast_channel/cast_socket.h

Index: components/cast_channel/cast_socket.h
diff --git a/components/cast_channel/cast_socket.h b/components/cast_channel/cast_socket.h
index 6afee6a91bb34101507820d7478776f5d6f3a2a4..af29832ba19370002abb034986e142aa5430aeb2 100644
--- a/components/cast_channel/cast_socket.h
+++ b/components/cast_channel/cast_socket.h
@@ -55,6 +55,19 @@ enum CastDeviceCapability {
 // Public interface of the CastSocket class.
 class CastSocket {
  public:
+  class Observer {
+   public:
+    virtual ~Observer() {}
+
+    // Invoked when an error occurs on |socket|.
+    virtual void OnError(const CastSocket& socket,
+                         cast_channel::ChannelError error_state) = 0;
+
+    // Invoked when |socket| receives a message.
+    virtual void OnMessage(const CastSocket& socket,
+                           const cast_channel::CastMessage& message) = 0;
+  };
+
   virtual ~CastSocket() {}
 
   // Used by BrowserContextKeyedAPIFactory.
@@ -68,10 +81,8 @@ class CastSocket {
   // If the CastSocket is destroyed while the connection is pending, |callback|
   // will be invoked with CHANNEL_ERROR_UNKNOWN. In this case, invoking
   // |callback| must not result in any re-entrancy behavior.
-  // |delegate| receives message receipt and error events.
   // Ownership of |delegate| is transferred to this CastSocket.
-  virtual void Connect(std::unique_ptr<CastTransport::Delegate> delegate,
-                       base::Callback<void(ChannelError)> callback) = 0;
+  virtual void Connect(base::Callback<void(ChannelError)> callback) = 0;
 
   // Closes the channel if not already closed. On completion, the channel will
   // be in READY_STATE_CLOSED.
@@ -112,6 +123,14 @@ class CastSocket {
   // Returns a pointer to the socket's message transport layer. Can be used to
   // send and receive CastMessages over the socket.
   virtual CastTransport* transport() const = 0;
+
+  // Returns true if current socket has an observer with |id|.
+  virtual bool HasObserver(const std::string& id) = 0;

[imcheng, 2017/06/16 23:39:25]

I'm not sure if we need to associate an id with an observer. This method, for
instance, can just take a Observer*.

[zhaobin, 2017/06/20 00:54:36]

Done.

+
+  // Register |observer| with current socket to receive message receipt
+  // and error events.
+  virtual void AddObserver(const std::string& id,
+                           std::unique_ptr<Observer> observer) = 0;

[imcheng, 2017/06/16 23:39:25]

It's a bit unusual to take ownership of observers... There's no way to remove
observers, and CastSocket does not know when the observer is no longer valid,
resulting in wasted work (CastMessageHandler::*) and memory leaks
(CastMessageHandlers created in Profiles that no longer exist won't get deleted
until the CastSocket is deleted)

[zhaobin, 2017/06/20 00:54:36]

Make CastSocketService own observers.

 };
 
 // This class implements a channel between Chrome and a Cast device using a TCP
@@ -153,8 +172,7 @@ class CastSocketImpl : public CastSocket {
   ~CastSocketImpl() override;
 
   // CastSocket interface.
-  void Connect(std::unique_ptr<CastTransport::Delegate> delegate,
-               base::Callback<void(ChannelError)> callback) override;
+  void Connect(base::Callback<void(ChannelError)> callback) override;
   CastTransport* transport() const override;
   void Close(const net::CompletionCallback& callback) override;
   const net::IPEndPoint& ip_endpoint() const override;
@@ -164,6 +182,9 @@ class CastSocketImpl : public CastSocket {
   ChannelError error_state() const override;
   bool keep_alive() const override;
   bool audio_only() const override;
+  bool HasObserver(const std::string& id) override;
+  void AddObserver(const std::string& id,
+                   std::unique_ptr<Observer> observer) override;
 
  protected:
   // CastTransport::Delegate methods for receiving handshake messages.
@@ -189,6 +210,22 @@ class CastSocketImpl : public CastSocket {
     LastError last_error_;
   };
 
+  // CastTransport::Delegate methods to receive normal messages and errors.
+  class CastSocketMessageDelegate : public CastTransport::Delegate {
+   public:
+    CastSocketMessageDelegate(cast_channel::CastSocketImpl* socket);
+    ~CastSocketMessageDelegate() override;
+
+    // CastTransport::Delegate implementation.
+    void OnError(cast_channel::ChannelError error_state) override;
+    void OnMessage(const cast_channel::CastMessage& message) override;
+    void Start() override;
+
+   private:
+    CastSocketImpl* const socket_;
+    DISALLOW_COPY_AND_ASSIGN(CastSocketMessageDelegate);
+  };
+
   // Replaces the internally-constructed transport object with one provided
   // by the caller (e.g. a mock).
   void SetTransportForTesting(std::unique_ptr<CastTransport> transport);
@@ -204,7 +241,9 @@ class CastSocketImpl : public CastSocket {
                            TestConnectChallengeReplyReceiveError);
   FRIEND_TEST_ALL_PREFIXES(CastSocketTest,
                            TestConnectChallengeVerificationFails);
+  FRIEND_TEST_ALL_PREFIXES(CastSocketTest, TestObservers);
   friend class AuthTransportDelegate;
+  friend class CastSocketMessageDelegate;
   friend class CastSocketTest;
   friend class TestCastSocket;
 
@@ -377,6 +416,11 @@ class CastSocketImpl : public CastSocket {
   // information.
   AuthTransportDelegate* auth_delegate_;
 
+  // Map of CastSocket::Observer keyed by observer id. For extension side
+  // observers, id is extension_id; For browser side observers, id is a hard
+  // coded string.
+  std::map<std::string, std::unique_ptr<Observer>> observer_map_;

[mark a. foltz, 2017/06/16 23:26:14]

Would a base::ObserverList work here?  I.e. can something keep track that
exactly one Observer is added for the browser, and one per extension?

[zhaobin, 2017/06/20 00:54:36]

Done.

+
   DISALLOW_COPY_AND_ASSIGN(CastSocketImpl);
 };
 }  // namespace cast_channel