Cleanup channel dispatchers

remoting/protocol/channel_dispatcher_base.cc

Index: remoting/protocol/channel_dispatcher_base.cc
diff --git a/remoting/protocol/channel_dispatcher_base.cc b/remoting/protocol/channel_dispatcher_base.cc
index 0209cc27063766888e99d621e330ca27b2a36d73..ee90ee56dcbc7bc02756ebf80e919e90889d6cbd 100644
--- a/remoting/protocol/channel_dispatcher_base.cc
+++ b/remoting/protocol/channel_dispatcher_base.cc
@@ -15,17 +15,19 @@ namespace protocol {
 
 ChannelDispatcherBase::ChannelDispatcherBase(const char* channel_name)
     : channel_name_(channel_name),
-      channel_factory_(nullptr) {
+      channel_factory_(nullptr),
+      event_handler_(nullptr) {
 }
 
 ChannelDispatcherBase::~ChannelDispatcherBase() {
+  writer()->Close();
   if (channel_factory_)
     channel_factory_->CancelChannelCreation(channel_name_);
 }
 
 void ChannelDispatcherBase::Init(Session* session,
                                  const ChannelConfig& config,
-                                 const InitializedCallback& callback) {
+                                 EventHandler* event_handler) {
   DCHECK(session);
   switch (config.transport) {
     case ChannelConfig::TRANSPORT_MUX_STREAM:
@@ -37,12 +39,10 @@ void ChannelDispatcherBase::Init(Session* session,
       break;
 
     default:
-      NOTREACHED();
-      callback.Run(false);
-      return;
+      LOG(FATAL) << "Unknown transport type: " << config.transport;
   }
 
-  initialized_callback_ = callback;
+  event_handler_ = event_handler;
 
   channel_factory_->CreateChannel(channel_name_, base::Bind(
       &ChannelDispatcherBase::OnChannelReady, base::Unretained(this)));
@@ -51,16 +51,21 @@ void ChannelDispatcherBase::Init(Session* session,
 void ChannelDispatcherBase::OnChannelReady(
     scoped_ptr<net::StreamSocket> socket) {
   if (!socket.get()) {
-    initialized_callback_.Run(false);
+    event_handler_->OnChannelError(this, CHANNEL_CONNECTION_ERROR);
     return;
   }
 
   channel_factory_ = nullptr;
   channel_ = socket.Pass();
+  writer_.Init(channel_.get(), base::Bind(&ChannelDispatcherBase::OnWriteFailed,

[rmsousa, 2015/01/08 02:22:31]

This means that every child of ChannelDispatcherBase will now start both a
reader (that discards everything because the callback is null) and a writer.

Questions:
* Is there such a thing as a read-only/write-only channel, and would this cause
a problem in that case?
* As far as I can tell, each ChannelDispatcherBase creates its own channel -
but, just to confirm, there's no case where we create separate "reader" and
"writer" ChannelDispatchers for hte same channel, right? (i.e. there's no chance
that a "writer" ChannelDispatcherBase would read and discard messages that
another "reader" ChannelDispatcherBase would be interested in)?
* Any other unintended consequences of running a reader in channels wehre we
didn't previously run one? (I assume running hte writer is fine as long as we
don't write anything).

[Sergey Ulanov, 2015/01/08 19:20:32]

Video and Audio channels are currently used in only one direction
Right. ChannelDispatcherBase owns the channel, and there is no way to access it
from somewhere else.
The main difference is that if for some reason a peer sends some data where it's
not expected the receiver will now read and discard the data instead of just
holding it the TCP buffers as it was before. This behavior actually makes
handling of unknown messages more consistent - dispatchers normally drop
messages they do not understand.

+                                          base::Unretained(this)));
+  reader_.StartReading(channel_.get());
 
-  OnInitialized();
+  event_handler_->OnChannelInitialized(this);
+}
 
-  initialized_callback_.Run(true);
+void ChannelDispatcherBase::OnWriteFailed(int error) {
+  event_handler_->OnChannelError(this, CHANNEL_CONNECTION_ERROR);
 }
 
 }  // namespace protocol