[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.
+class DataChannelHandler : public MessagePipe::EventHandler {
+ public:
+  // Creates a DataChannelHandler to handle data from |pipe|.
+  DataChannelHandler(const std::string& name,
+                     std::unique_ptr<MessagePipe> pipe);
+
+  // Closes the channel and eventually destructs this instance. Any operations
+  // should not be performed after executing this function.

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

The last sentence could be more clear:
"No operations should be performed after executing this function."

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

Done.

+  void Close();
+
+  const std::string& pipe_name() const { return name_; }
+
+  bool closed() const { return pipe_ == nullptr; }
+
+  bool connected() const { return is_connected_; }

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

How are 'connected()' and 'closed()' different?  This might be confusing for
callers as I think your intention is that a closed pipe could be destroyed soon
but a caller could see that and try to connect again.

Can you use one connection state and then use DCHECKS to ensure the data channel
isn't re-connected as you try to delete it?  Another option would be to not
delete it and allow the owner to Close/Destroy it as needed.  I'm not sure why
this object needs to destroy itself.

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

I may rename "closed()" to "finialized()" to avoid any confusion.
The goal of this function is to ensure the derived classes have a way to track
the lifetime of itself even the pipe has never been opened.
Say an implementation uses a timer, it needs to stop the timer once the "this"
will be deleted later. This usually can be done by OnDisconnecting(). But once
the pipe has not been connected at all, the OnDisconnecting() function won't be
called.

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

I think this is more confusing as finalized() is a C#/Java term and doesn't
apply here (there is no finalization queue).

Providing a method to check the state of a soon to be deleted instance is
dangerous.  This should be fixed by changing the design at a higher level I
think.

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

I have changed the function name into closing().

This is helpful when using DataChannelHandler with Timer.
==============
Implementation::Implementation() {
  repeat_timer_.Start(FROM_HERE, interval, this, &Implementation::DoSomething);
}

void Implementation::DoSomething() {
  if (closing()) {
    repeat_timer_.Stop();
    return;
  }

  Send(...);
}
==============
Because the DeleteSoon() happens in the end of the thread task runner, but
repeat_timer_ posts task before it, this implementation is always safe. i.e.
PostTask() in current thread task runner once closing() is true is disallowed.

+
+  // Returns false before the channel is connected.

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

I'd make this comment more generic, "Returns false if the message is not sent".

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

Updated, but I still prefer to explicitly mention the reason of failure.
Otherwise the caller may not know what should be done once the function returns
false. i.e.
==========================
if (connected()) {
  bool r = Send();
  DCHECK(r);
}
==========================
should be perfectly fine.

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

I think it is better to avoid impl details in the header comment.  If you want
to prevent people from calling Send before connecting, then DCHECK the state.

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

DCHECK() the state does not match the design of "an optional data channel". i.e.
Failed to send one or more data packets should not impact the result.

And we have bunch of cases to describe the situations once the function fails.
E.g.
https://cs.chromium.org/chromium/src/tools/gn/scope.h?q=bool.*%5C(.%2B%5C)&l=...
and
https://cs.chromium.org/chromium/src/third_party/webrtc/base/stream.h?dr=C&l=169.

It seems not like an implementation detail, but the explanation of the failure
reason.

+  bool Send(google::protobuf::MessageLite* message, const base::Closure& done);

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

Why is message a pointer and not a const ref, are you planning on changing it?

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

Because MessagePipe::Send() receives a pointer. It's questionable to use pointer
in MessagePipe::Send(): it may be helpful to merge the message with others,
though it's not likely to happen. But to avoid a copy, I need to use pointer
here.

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

Potentially MessagePipe::Send() can be changed to use const-ref.
MessageLite is an interface, and interfaces are normally passed by pointer.
AFAIR that's the reason it's currently passed by pointer. In most cases
const-ref args can be copied, but MessageLite can't.

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

MessageLite is not copyable anyway, so using const ref seems still reasonable. I
can try it in a different change.

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

I didn't realize MessageLite was an interface, thanks Sergey.

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

Done.

+
+ protected:
+  ~DataChannelHandler() override;
+
+  // Derived classes can override these functions to receive data from the
+  // connection or observe the connection state.

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

Why use inheritance here instead of an observer pattern (which is more common in
our codebase)?

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

A derived class of DataChannelHandler handles data instead of observing data, a
typical implementation should both receive and send data. Observer pattern seems
not match the requirement of this class.

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

I missed that the first time around.  A better approach would be to invert this
relationship then.  Create an interface which is passed into an instance of the
DataChannelHandler.

That Interface will expose:
virtual void void OnIncomingMessage(std::unique_ptr<CompoundBuffer> message);
void OnConnected();
void OnDisconnecting();

The wrapper will own the instance of the class implementing the interface and
control its lifetime.

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

I still do not see the difference,
1. who will control the lifetime of the wrapper?
2. How to send data through the interface?
3. DataChannelHandler itself handles the data, instead of sending and receiving
data: it's a MessagePipe::EventHandler, but not a MessagePipe. So I cannot quite
tell why we need to delegate the implementation to an interface.

+  virtual void OnIncomingMessage(std::unique_ptr<CompoundBuffer> message);
+  virtual void OnConnected();
+  virtual void OnDisconnected();
+
+ private:
+  friend class base::DeleteHelper<DataChannelHandler>;
+
+  // 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_