[Chromoting] Add DataChannelManager to manage optional incoming data channels

remoting/protocol/data_channel_handler.cc

+// 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;
+
+void DataChannelHandler::Close() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (connected()) {
+    OnDisconnecting();
+    is_connected_ = false;
+  }
+  if (!closing()) {
+    pipe_.reset();
+    base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);

[joedow, 2017/06/06 23:33:25]

Why not just delete now instead of doing it async?  You reset the pipe already
so I'm not sure what else would block here.

Close should never be called twice as you don't know how quickly the instance
will be deleted and once it is, your member methods above won't protect against
a crash.

[Hzj_jie, 2017/06/07 17:31:24]

If the DataChannelHandler works with a timer and the timer can only be stopped
in OnDisconnecting, deleting immediately here is not safe. We can ensure posting
task after OnDisconnecting() is disallowed, but any tasks posted before it can
be executed safely.

+  }
+}
+
+bool DataChannelHandler::Send(google::protobuf::MessageLite* message,
+                              const base::Closure& done) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (connected()) {
+    pipe_->Send(message, done);
+    return true;
+  }
+
+  return false;

[joedow, 2017/06/06 23:33:25]

Why don't you want to DCHECK if someone tries to send before the pipe is
connected?  Returning a bool is safe, but I would probably end up DCHECK'ING in
the calling code to prevent bugs (which kicks the can down the road).

When is it ok to send when the pipe is not connected?

[Hzj_jie, 2017/06/07 17:31:24]

It also relates to the potential usage of timer: the derived classes do not need
to check connected() in the timer callback before Send() if it does not care
about whether the data can be sent.
In another side, if we are using DCHECK() here, we force the derived classes to
check connected() before calling Send().

+}
+
+void DataChannelHandler::OnIncomingMessage(
+    std::unique_ptr<CompoundBuffer> message) {}
+
+void DataChannelHandler::OnConnected() {}
+
+void DataChannelHandler::OnDisconnecting() {}
+
+void DataChannelHandler::OnMessagePipeOpen() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(!is_connected_);
+  is_connected_ = true;
+  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