Refactor BlimpConnection to TCPConnection

blimp/net/blimp_connection.h

Index: blimp/net/blimp_connection.h
diff --git a/blimp/net/blimp_connection.h b/blimp/net/blimp_connection.h
index 10450d0a2853b79f13282138a717f267136df8bc..440633fb416e53cc3c2eb2c439dba0cb361f5619 100644
--- a/blimp/net/blimp_connection.h
+++ b/blimp/net/blimp_connection.h
@@ -15,15 +15,11 @@
 namespace blimp {
 
 class BlimpMessageProcessor;
-class BlimpMessagePump;
-class BlimpMessageSender;
-class MessagePort;
 
 // Encapsulates the state and logic used to exchange BlimpMessages over
 // a network connection.
 class BLIMP_NET_EXPORT BlimpConnection : public ConnectionErrorObserver {

[Kevin M, 2016/10/25 18:49:23]

Perhaps there needs to be more detail in the CL description going over the "why"
of this CL, but IMO the boundaries of the abstraction you're going for seems
really fuzzy. There seems to be an odd mixture of abstract and concrete
functionality, which signals to me that it might be cleaner use composition and
pure virtual interfaces rather than virtual base class inheritance.

[perumaal, 2016/10/25 23:02:39]

This is interim and only to have a bridge until we have HeliumTransport at which
time we would throw away all this code.

It's a balance between rewriting / touching a lot of this code that is going
away vs spending time supporting new code to work with this existing codepath.

Right now, I have put a lot of effort in making sure that GrpcStream (next CL)
matches the checked in HeliumStream interface and making sure BlimpConnection
goes through this same interface to minimize switching time (without
compromising the existing design too much).

I will update the CL description.

  public:
-  explicit BlimpConnection(std::unique_ptr<MessagePort> message_port);
   ~BlimpConnection() override;
 
   // Adds |observer| to the connection's error observer list.
@@ -35,10 +31,11 @@ class BLIMP_NET_EXPORT BlimpConnection : public ConnectionErrorObserver {
   // Sets the processor which will take incoming messages for this connection.
   // Can be set multiple times, but previously set processors are discarded.
   // Caller retains the ownership of |processor|.
-  virtual void SetIncomingMessageProcessor(BlimpMessageProcessor* processor);
+  virtual void SetIncomingMessageProcessor(BlimpMessageProcessor* processor)
+    = 0;

[Kevin M, 2016/10/25 18:49:23]

"git cl format" ?

[perumaal, 2016/10/25 23:02:39]

Nice! Thanks! :)

 
   // Gets a processor for BrowserSession->BlimpConnection message routing.
-  virtual BlimpMessageProcessor* GetOutgoingMessageProcessor();
+  virtual BlimpMessageProcessor* GetOutgoingMessageProcessor() = 0;
 
  protected:
   class EndConnectionFilter;
@@ -46,16 +43,16 @@ class BLIMP_NET_EXPORT BlimpConnection : public ConnectionErrorObserver {
 
   BlimpConnection();
 
+  void AddEndConnectionProcessor(BlimpMessageProcessor* processor);
+
   // ConnectionErrorObserver implementation.
   void OnConnectionError(int error) override;
 
+  BlimpMessageProcessor* GetEndConnectionProcessor() const;
+
  private:
-  std::unique_ptr<MessagePort> message_port_;
-  std::unique_ptr<BlimpMessagePump> message_pump_;
-  std::unique_ptr<BlimpMessageSender> outgoing_msg_processor_;
-  base::ObserverList<ConnectionErrorObserver> error_observers_;
   std::unique_ptr<EndConnectionFilter> end_connection_filter_;
-
+  base::ObserverList<ConnectionErrorObserver> error_observers_;
   DISALLOW_COPY_AND_ASSIGN(BlimpConnection);
 };