[Chromoting] Add DataChannelManager to manage optional incoming data channels

remoting/protocol/data_channel_handler.h

+// 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.
+
+#ifndef REMOTING_PROTOCOL_DATA_CHANNEL_HANDLER_H_
+#define REMOTING_PROTOCOL_DATA_CHANNEL_HANDLER_H_
+
+#include <memory>
+#include <string>
+
+#include "base/callback.h"
+#include "base/sequenced_task_runner_helpers.h"
+#include "base/threading/thread_checker.h"
+#include "remoting/protocol/message_pipe.h"
+
+namespace google {
+namespace protobuf {
+class MessageLite;
+}  // namespace protobuf
+}  // namespace google
+
+namespace remoting {
+
+class CompoundBuffer;
+
+namespace protocol {
+
+// A base class to handle data from a named data channel. This class manages the

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

nit: Replace 'data channel' with 'MessagePipe'.  Otherwise we should define what
a DataChannel is somewhere (or call it WebRtcDataChannelHandler)

I'd like to be specific and consistent with the terms used so we don't have to
update the comments if we decide to use it elsewhere.

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

Done.

+// lifetime itself: it deletes itself once the data channel is closed or the
+// derived instance actively calls Close() function.
+class DataChannelHandler : public MessagePipe::EventHandler {

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

Consider renaming to something like MessagePipeHandlerBase since the class isn't
specific to a Data Channel.  That way it will also be listed next to the
MessagePipe class in the directory structure which will show that they are
strongly related.

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

I think NamedMessagePipeHandler would be better, as there is not a name for a
typical MessagePipe.

+ protected:
+  // Creates a DataChannelHandler to handle data from |pipe|.
+  // This class is not virtual, but it is an no-op instance. So hide the

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

This class isn't a no-op instance though, it does work and handles instance
lifetime.  Could this comment be updated to state that callers should create
instances of the derived types instead of the base class which is why the c'tor
is hidden?

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

Done.

+  // constructor.
+  DataChannelHandler(const std::string& name,
+                     std::unique_ptr<MessagePipe> pipe);

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

Would it makes sense to use a zero-param c'tor and instead add an 'Initialize()'
method which is passed the params?

Using this means the derived class needs to have these values passed in as well,
however if you used a non-virtual init() method, then the derived class would
never see the actual values here and would need to use the protected methods to
access the pipe name.

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

Keeping a copy of |name| in derived classes is not that bad: not encouraged, but
without significant harmfulness.
Keeping a raw pointer of |pipe| is definitely wrong and can be simply found.

The concern of two-phase construction is that this pattern is not supported by
language. So there is not guarantee that Initialize() function can be called
only once. (Calling it twice is definitely not the right thing to do.)

+
+  ~DataChannelHandler() override;
+
+  // Closes the channel and eventually destructs this instance. No operations
+  // should be performed after executing this function.
+  void Close();
+
+  const std::string& pipe_name() const { return name_; }
+
+  // Returns true if |this| object has been scheduled to be deleted. This
+  // function always indicates the lifetime of |this| even the |pipe_| has never
+  // been opened.
+  bool closing() const { return pipe_ == nullptr; }
+
+  // Whether |pipe_| has been connected.
+  bool connected() const { return is_connected_; }
+
+  // Returns false if the message is not sent, usually it happens before the
+  // |pipe_| is opened.
+  bool Send(google::protobuf::MessageLite* message, const base::Closure& done);
+
+  // Derived classes can override these functions to receive data from the
+  // connection or observe the connection state. These functions will not be
+  // called unless |pipe_| has been opened.
+

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

nit: remove newline

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

Done.

+  virtual void OnIncomingMessage(std::unique_ptr<CompoundBuffer> message);
+  virtual void OnConnected();
+  virtual void OnDisconnecting();

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

Can these be pure virtual?  Is the only benefit that you can create instances
for testing?

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

I believe in most the case, only one or two of them are useful.
- If the implementation sends data regularly: OnConnected() and
OnDisconnecting() are useful.
- If the implementation receives data: OnIncomingMessage() and OnDisconnecting()
are useful.
- If the implementation sends after receives: OnIncomingMessage() is useful.
- If the implementation receives after sends: OnConnected() and
OnIncomingMessage() is useful.

So indeed I cannot find a case to use all of these three functions.

+
+ private:
+  friend class base::DeleteHelper<DataChannelHandler>;

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

Why is DeleteHelper needed here?  I haven't used it before so curious why it is
required.

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

Because the destructor is protected (to avoid unexpected "delete object;"),
TaskRunner::DeleteSoon cannot access it. The DeleteHelper is the functor to do
the real destruction work, so we need to give the friendship to it.

+
+  // MessagePipe::EventHandler implementation.
+  void OnMessagePipeOpen() override;
+  void OnMessageReceived(std::unique_ptr<CompoundBuffer> message) override;
+  void OnMessagePipeClosed() override;
+
+  const std::string name_;
+  std::unique_ptr<MessagePipe> pipe_;
+  base::ThreadChecker thread_checker_;
+  bool is_connected_ = false;
+};
+
+}  // namespace protocol
+}  // namespace remoting
+
+#endif  // REMOTING_PROTOCOL_DATA_CHANNEL_HANDLER_H_