Refactor BlimpConnection to TCPConnection

blimp/net/tcp_client_transport.h

Index: blimp/net/tcp_client_transport.h
diff --git a/blimp/net/tcp_client_transport.h b/blimp/net/tcp_client_transport.h
index aa8f049ef96e67f21465d855defa5b9c170b3f27..c781118c6f5d688b5ade607cb3aa165fe49a6994 100644
--- a/blimp/net/tcp_client_transport.h
+++ b/blimp/net/tcp_client_transport.h
@@ -37,7 +37,10 @@ class BLIMP_NET_EXPORT TCPClientTransport : public BlimpTransport {
 
   // BlimpTransport implementation.
   void Connect(const net::CompletionCallback& callback) override;
-  std::unique_ptr<MessagePort> TakeMessagePort() override;
+  std::unique_ptr<BlimpConnection> MakeConnection() override;
+
+  std::unique_ptr<MessagePort> TakeMessagePort();

[Garrett Casto, 2016/10/25 18:33:50]

Nit: You have a non-overriden function in the middle of the BlimpTransport
functions. This should be above the rest and the overriden functions should
generally be grouped together with no
spaces.

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

Why do we have TakeMessagePort *and* MakeConnection? The side effect of one will
prevent the other from working properly. This introduces additional coupling
between BlimpTransport and BlimpConnection that we probably don't want going
forward. It feels a bit odd - like we should have another entity turning
MessagePorts into BlimpConnections.

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

Kevin: Again, I wanted to minimize the amount of churn in the *existing* code
that is about to go away soon. Note that this was happening implicitly before
(in two places: Client/Engine session classes). I just moved into this one class
which won't happen with the gRPC classes.

Initially I tried to have some sort of ownership change in the TCP* classes but
I figured that refactoring solely for TCP* classes is wasted effort IMO. I can
add a TODO if you'd like... WDYT?

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

My bad. thanks

+
   const char* GetName() const override;
 
  protected: