[Chromoting] Add DataChannelManager to manage optional incoming data channels

remoting/protocol/data_channel_manager.h

Index: remoting/protocol/data_channel_manager.h
diff --git a/remoting/protocol/data_channel_manager.h b/remoting/protocol/data_channel_manager.h
new file mode 100644
index 0000000000000000000000000000000000000000..0f65e6801b2bfd8cc51940773809e7058124034c
--- /dev/null
+++ b/remoting/protocol/data_channel_manager.h
@@ -0,0 +1,53 @@
+// 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_MANAGER_H_
+#define REMOTING_PROTOCOL_DATA_CHANNEL_MANAGER_H_
+
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "base/callback.h"
+
+namespace remoting {
+namespace protocol {
+
+class DataChannelHandler;

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

remove this fwd declare since it isn't used.

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

Done.

+class MessagePipe;
+
+// DataChannelManager helps to manage optional data channels. Each
+// DataChannelHandler implementation registers a factory function to create an
+// instance of itself to handle data from a named data channel. All handles are
+// closed when the associated MessagePipe is closed. Clients can also close a
+// data channel manually by calling DataChannelHandler::Close() function.

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

I don't think the DCM should include any impl details about the DCH.  For
instance the bit about calling Close() is already out of date as that method is
protected so a client can't call it directly.

Can you update the class description to be specific to the DCM?

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

The client means "the client of DataChannelHandler". But anyway, this comment is
do out of date. I have updated it.

+class DataChannelManager final {
+ public:
+  using CreateHandlerCallback = base::Callback<void(
+      const std::string& name,
+      std::unique_ptr<MessagePipe> pipe)>;
+
+  DataChannelManager();
+  ~DataChannelManager();

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

Would it make sense to expose a static function and return an instance to a
singleton here?  You should never have more than one of these per session / host
lifetime.

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

The lifetime of a DataChannelManager should be consistent with ClientSession.
But the later one is not a singleton object in the lifetime of the host.

+
+  // Registers a factory function to create a DataChannelHandler to handle a new

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

As discussed, it may not create a DataChannelHandler.  Can you update the
comments in the header to be specific to the DCM class?

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

Done.

+  // incoming data channel with a name matching |prefix|. Returns false if the
+  // registration fails, usually it happens when |prefix| or |constructor| are
+  // empty.
+  bool RegisterCreateHandlerCallback(const std::string& prefix,
+                                     CreateHandlerCallback constructor);
+
+  // Creates a DataChannelHandler to handle the new incoming data channel.
+  // Returns true if a handler exists for the new data channel.

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

nit: Doesn't the function return true if a handler was created for the incoming
request (not just that one exists)?

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

Done.

+  bool OnIncomingDataChannel(const std::string& name,
+                             std::unique_ptr<MessagePipe> pipe);
+
+ private:
+  std::vector<std::pair<std::string, CreateHandlerCallback>> constructors_;
+};
+
+}  // namespace protocol
+}  // namespace remoting
+
+#endif  // REMOTING_PROTOCOL_DATA_CHANNEL_MANAGER_H_