[cast_channel] Implement CastSocketService::OpenSocket()

components/cast_channel/cast_socket_service.h

Index: components/cast_channel/cast_socket_service.h
diff --git a/components/cast_channel/cast_socket_service.h b/components/cast_channel/cast_socket_service.h
index d7d016ba41806bc6f48babb97d43c1526b83308f..99baf8fec1ae68362028c8b751d72faa996848cc 100644
--- a/components/cast_channel/cast_socket_service.h
+++ b/components/cast_channel/cast_socket_service.h
@@ -8,6 +8,7 @@
 #include <map>
 #include <memory>
 
+#include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/threading/thread_checker.h"
 #include "components/cast_channel/cast_socket.h"
@@ -16,17 +17,42 @@
 
 namespace cast_channel {
 
+// Receives incoming messages and errors and pass them to inner delegate.
+class PassThroughMessageHandler : public CastTransport::Delegate {
+ public:
+  PassThroughMessageHandler();
+  ~PassThroughMessageHandler() override;
+
+  void RegisterDelegate(std::unique_ptr<CastTransport::Delegate> delegate);
+
+  // CastTransport::Delegate implementation.
+  void OnError(ChannelError error_state) override;
+  void OnMessage(const CastMessage& message) override;
+  void Start() override;
+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(CastSocketServiceTest, TestRegisterDelegate);
+
+  std::unique_ptr<CastTransport::Delegate> inner_delegate_;
+
+  THREAD_CHECKER(thread_checker_);
+
+  DISALLOW_COPY_AND_ASSIGN(PassThroughMessageHandler);
+};
+
 // This class adds, removes, and returns cast sockets created by CastChannelAPI
 // to underlying storage.
 // Instance of this class is created on the UI thread and destroyed on the IO
 // thread. All public API must be called from the IO thread.
 class CastSocketService : public RefcountedKeyedService {
  public:
-  CastSocketService();
+  // Callback invoked when cast socket opends.
+  // |channel_id|: Channel id of cast socket being opened
+  // |error_state|: Channel error encountered when opening cast socket.
+  using OnOpenCallback =
+      base::Callback<void(int channel_id, ChannelError error_state)>;
 
-  // Adds |socket| to |sockets_| and returns the new channel_id. Takes ownership
-  // of |socket|.
-  int AddSocket(std::unique_ptr<CastSocket> socket);
+  CastSocketService();
 
   // Removes the CastSocket corresponding to |channel_id| from the
   // CastSocketRegistry. Returns nullptr if no such CastSocket exists.
@@ -36,17 +62,117 @@ class CastSocketService : public RefcountedKeyedService {
   // otherwise.
   CastSocket* GetSocket(int channel_id) const;
 
+  // Opens cast socket with |ip_endpoint| and invokes |open_cb| when opening
+  // operation finishes. If cast socket with |ip_endpoint| already exists,
+  // invoke |open_cb| directly with existing socket's channel ID.
+  // Parameters:
+  // |ip_endpoint|: IP address of the remote host.
+  // |channel_auth|: Authentication method used for connecting to a Cast
+  //                 receiver. |channel_auth| must not be CHANNEL_AUTH_NONE.

[mark a. foltz, 2017/06/12 21:14:08]

See previous comment re: CHANNEL_AUTH_NONE

[zhaobin, 2017/06/20 01:37:42]

Created crbug.com/732669.

+  // |net_log|: Log of socket events.
+  // |connect_timeout|: Connection timeout interval.
+  // |ping_interval|: Ping interval.
+  // |liveness_timeout|: Liveness timeout for connect calls.
+  // |logger|: Log of cast channel events.
+  // |device_capabilities|: Device capabilities.
+  // |open_cb|: Invoked when cast socket opens.
+  int OpenSocket(const net::IPEndPoint& ip_endpoint,

[mark a. foltz, 2017/06/12 21:14:08]

With 7 parameters (not including the deprecated ChannelAuthType), it makes an
argument for introducing a Parameter Object (struct).  However it depends on how
many times these are used together.

https://sourcemaking.com/refactoring/introduce-parameter-object

If this parameter list is repeated several times (e.g., in unit tests)
introducing a struct does improve readability I think.

[zhaobin, 2017/06/20 01:37:42]

Created crbug.com/734855

+                 ChannelAuthType channel_auth,
+                 net::NetLog* net_log,
+                 const base::TimeDelta& connect_timeout,
+                 const base::TimeDelta& ping_interval,
+                 const base::TimeDelta& liveness_timeout,
+                 const scoped_refptr<Logger>& logger,
+                 uint64_t device_capabilities,
+                 const OnOpenCallback& open_cb);
+
+  // Opens cast socket with |ip_endpoint| and invokes |open_cb| when opening
+  // operation finishes. If cast socket with |ip_endpoint| already exists,
+  // invoke |open_cb| directly with existing socket's channel ID.
+  // |ip_endpoint|: IP endpoint to be connected to.
+  // |net_log|: Net log passed to cast socket.
+  // |open_cb|: first parameter is channel id of newly created socket; second
+  // parameter is channel error encountered during channel opening.
+  int OpenSocket(const net::IPEndPoint& ip_endpoint,
+                 net::NetLog* net_log,
+                 const OnOpenCallback& open_cb);
+
+  // Register read delegate to cast socket with |channel_id|. Returns false if
+  // such socket does not exist.
+  bool RegisterDelegate(int channel_id,
+                        std::unique_ptr<CastTransport::Delegate> delegate);
+
+  // Sets the CastSocket instance to be used for testing.
+  void SetSocketForTest(
+      std::unique_ptr<cast_channel::CastSocket> socket_for_test);
+
+  // Sets injected ping timeout timer for testing.
+  void SetPingTimeoutTimerForTest(std::unique_ptr<base::Timer> timer);
+
  private:
+  FRIEND_TEST_ALL_PREFIXES(CastSocketServiceTest, TestOpenChannel);
+  friend class CastSocketServiceTest;
+
+  class CastSocketRecord {
+   public:
+    // Does not take ownership of |message_handler|.
+    CastSocketRecord(std::unique_ptr<CastSocket> socket,
+                     const OnOpenCallback& on_open_callback,
+                     PassThroughMessageHandler* message_handler);
+
+    ~CastSocketRecord();
+
+    // Underlying cast socket.
+    std::unique_ptr<CastSocket> cast_socket;
+
+    // Pending callbacks to be invoked when cast socket opens.
+    std::vector<OnOpenCallback> pending_on_open_callbacks;
+
+    // Real read delegate for incoming errors and messages. Owned by |socket|'s
+    // keep alive delegate.
+    PassThroughMessageHandler* pass_through_message_handler;
+  };
+
   ~CastSocketService() override;
 
   // RefcountedKeyedService implementation.
   void ShutdownOnUIThread() override;
 
+  // Creates a socket record, adds it to |socket_records_| and returns the
+  // new channel_id. Takes ownership of |socket|.
+  // |socket|: Underlying cast socket.
+  // |on_open_callback|: Callback to be invoked when cast socket opens.
+  // |message_handler|: Real read delegate for incoming errors and messages.
+  // Does not take ownership of |message_handler|.
+  int AddSocketRecord(std::unique_ptr<CastSocket> socket,
+                      const OnOpenCallback& on_open_callback,
+                      PassThroughMessageHandler* message_handler);
+
+  // Returns the socket record corresponding to |channel_id| if one exists, or
+  // nullptr otherwise.
+  CastSocketRecord* GetSocketRecord(int channel_id) const;

[mark a. foltz, 2017/06/12 21:14:08]

Should this return a pointer to const, or is it okay for callers to modify the
returned object?

[zhaobin, 2017/06/20 01:37:42]

Code removed.

+
+  // Returns the socket record whose ip_endpoint() equals to |ip_endpoint|, or
+  // nullptr if none exists.
+  CastSocketRecord* GetSocketRecord(const net::IPEndPoint& ip_endpoint) const;

[mark a. foltz, 2017/06/12 21:14:08]

Same comment applies here.

[zhaobin, 2017/06/20 01:37:42]

Code removed.

+
+  // Invoked when cast socket opens.
+  // |channel_id|: Channel id of cast socket.
+  // |error_state|: error state encountered during cast socket opening. None if
+  // socket open succeeds.
+  void OnOpen(int channel_id, ChannelError error_state);
+
   // Used to generate CastSocket id.
   static int last_channel_id_;
 
-  // The collection of CastSocket keyed by channel_id.
-  std::map<int, std::unique_ptr<CastSocket>> sockets_;
+  // The collection of cast socket record keyed by channel_id.
+  std::map<int, std::unique_ptr<CastSocketRecord>> socket_records_;
+
+  // Logger object for reporting error details.
+  scoped_refptr<Logger> logger_;
+
+  std::unique_ptr<cast_channel::CastSocket> socket_for_test_;
+  std::unique_ptr<base::Timer> injected_timeout_timer_;
 
   THREAD_CHECKER(thread_checker_);