[Chromoting] Add DataChannelManager to manage optional incoming data channels

remoting/protocol/data_channel_handler.cc

Index: remoting/protocol/data_channel_handler.cc
diff --git a/remoting/protocol/data_channel_handler.cc b/remoting/protocol/data_channel_handler.cc
new file mode 100644
index 0000000000000000000000000000000000000000..cf525cdf5d72f8bfa834d1113934a09c7122927d
--- /dev/null
+++ b/remoting/protocol/data_channel_handler.cc
@@ -0,0 +1,74 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/protocol/data_channel_handler.h"
+
+#include <utility>
+
+#include "base/location.h"
+#include "base/logging.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "remoting/base/compound_buffer.h"
+
+namespace remoting {
+namespace protocol {
+
+DataChannelHandler::DataChannelHandler(const std::string& name,
+                                       std::unique_ptr<MessagePipe> pipe)
+    : name_(name),
+      pipe_(std::move(pipe)) {
+  DCHECK(pipe_);
+  pipe_->Start(this);
+}
+
+DataChannelHandler::~DataChannelHandler() = default;

[joedow, 2017/05/30 16:24:17]

Don't you want to call Close() in the d'tor?

[Hzj_jie, 2017/05/31 00:11:51]

No, instead, destructor is called by Close() function. So a derived class would
either actively call Close() or rely on the MessagePipe to be closed.

[Sergey Ulanov, 2017/05/31 19:49:02]

This is a rather bug-prone approach, we should be careful when using it.
Particularly if an object owns can delete itself, then it shouldn't have any
public methods, see my next comment

[Hzj_jie, 2017/06/01 15:43:34]

only
The public methods are only public for tests. I can move them into protected
section and public these methods in FakeDataChannelHandler.

The only concern I have is that the possibility of lacking OnMessagePipeClosed()
function call from MessagePipe. Our current implementations do not have this
issue, but it's still worthy to be called out.

+
+void DataChannelHandler::Close() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (connected()) {
+    OnDisconnected();

[joedow, 2017/05/30 16:24:17]

Should this be called 'OnDisconnecting()'?  You are notifiying listeners of the
disconnect but the connection hasn't been disconnected yet (that happens on line
34).

[Hzj_jie, 2017/05/31 00:11:51]

Since this is the only callback regarding to the disconnection, I have not
opinion on Disconnecting or Disconnected.
Updated.

[joedow, 2017/06/01 17:25:25]

It is important as they have very different meanings.  Thanks for changing it.

[Hzj_jie, 2017/06/01 19:32:08]

Done.

+    is_connected_ = false;
+  }
+  if (!closed()) {
+    pipe_.reset();
+    base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);

[joedow, 2017/05/30 16:24:17]

This feels weird, I'm not sure why the owner should control the handler's
lifetime.

[Hzj_jie, 2017/05/31 00:11:51]

Because the handler should be deleted once the MessagePipe is closed, so
ensuring the consistent lifetime of DataChannelHandler and MessagePipe would be
the safest way to avoid any unexpected referring of destructed objects.

[Sergey Ulanov, 2017/05/31 19:49:02]

In order to send outgoing messages some other class will need keep a pointer to
DataChannelHandler. How would that class be notified when the channel is closed?
If this class is intended to be a base class for channel handlers that call
Send(), then Send() should be protected instead of public.

[Hzj_jie, 2017/06/01 15:43:34]

Yes, Send() is not intentionally used by any other object, but
DataChannelHandler itself.
IMO, a typical implementation of DataChannelHandler should handle everything
itself.

+  }
+}
+
+bool DataChannelHandler::Send(google::protobuf::MessageLite* message,
+                              const base::Closure& done) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (is_connected_) {

[joedow, 2017/05/30 16:24:17]

You use your helper method above (connected()), it would be good to be
consistent and use that here as well.

[Hzj_jie, 2017/05/31 00:11:51]

Done.

+    pipe_->Send(message, done);
+    return true;
+  }
+
+  return false;
+}
+
+void DataChannelHandler::OnIncomingMessage(
+    std::unique_ptr<CompoundBuffer> message) {}

[joedow, 2017/05/30 16:24:17]

Do these need to be implemented?  If they are optional, I think going with an
observer pattern is better.  If they are required to implement the virtual
methods, then you should add 'NOTIMPLEMENTED()' here to let them know they need
to override the methods.

[Hzj_jie, 2017/05/31 00:11:51]

Observer pattern seems not fitting this class: the DataChannelHandler implements
receiving the incoming message instead of observing them.

Using sending process statistic as an example: it does not expect to receive any
data from OnIncomingMessage(), but only uses Send() to send the data to the
peer, so it can simply ignore the override of this function.

If you have strong opinion, I can make this function as pure virtual, but I do
not think this is really necessary.

+
+void DataChannelHandler::OnConnected() {}
+
+void DataChannelHandler::OnDisconnected() {}
+
+void DataChannelHandler::OnMessagePipeOpen() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  is_connected_ = true;

[joedow, 2017/05/30 16:24:17]

DCHECK(!is_connected); to protect against multiple open calls?  It shouldn't
happen but would cause odd problems if it did.

[Hzj_jie, 2017/05/31 00:11:51]

Done.

+  OnConnected();
+}
+
+void DataChannelHandler::OnMessageReceived(
+    std::unique_ptr<CompoundBuffer> message) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  OnIncomingMessage(std::move(message));
+}
+
+void DataChannelHandler::OnMessagePipeClosed() {
+  Close();
+}
+
+}  // namespace protocol
+}  // namespace remoting