[Chromoting] Add DataChannelManager to manage optional incoming data channels

remoting/protocol/data_channel_manager.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_MANAGER_H_
+#define REMOTING_PROTOCOL_DATA_CHANNEL_MANAGER_H_
+
+#include <map>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "base/callback.h"
+
+namespace remoting {
+namespace protocol {
+
+class DataChannelHandler;
+class MessagePipe;
+
+// DataChannelManager helps to manage optional data channels. Each
+// DataChannelHandler implementation register a factory function to create an

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

s/register/registers

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

Done.

Also the comment does not reflect the latest change: lifetime of
DataChannelHandler instances are not managed by DataChannelManager.

+// instance of itself to handle data from a named data channel. Lifetime of all
+// DataChannelHandler instances are mananged by DataChannelManager instance.

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

s/mananged/managed

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

Done.

+// Besides releasing when session closing, clients can also actively close the
+// channel by calling DataChannelHandler::Close() function.

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

re-write lines 25/26 -> All handlers are closed when the session ends.  Clients
can also close a data channel manually by calling the close method.

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

Done.

+class DataChannelManager final {
+ public:
+  using HandlerConstructor = base::Callback<void(

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

maybe rename HandlerConstructor to DataChannelHandlerFactory?

The function passed in might not construct a new instance, it would be useful to
be able to provide a singleton as well.

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

Term "DataChannel" seems a little bit redundant, I have updated the name into
HandlerFactory.

+      const std::string& name,
+      std::unique_ptr<MessagePipe> pipe)>;
+
+  DataChannelManager();
+  ~DataChannelManager();
+
+  // Registers a factory function to create a DataChannelHandler to handle a new
+  // incoming data channel with a name matching |regex|. This function fails and
+  // returns false when |regex| or |constructor| are empty.

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

fix the last sentence:
Returns true if the registration succeeds.

This is simpler and requires no maintenance if the param names change.

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

Same as DataChannelHandler::Send(), I still prefer to explicitly explain the
failure cases.

+  bool RegisterHandlerFactory(const std::string& regex,

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

A regex isn't needed is it?  Data channels have a standard format
({prefix}-{unique_pipe_id}).  Can't we register a strict substring for the
prefix (aka message type)?  I don't think we would want to register a handler
based on the pipe_id suffix right?

I think it would be less error prone to allow registration for the message_type
and use a starts_with match.

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

I do not think there is any restriction of the naming of data channels.
IMO, using regex does not increase the complexity (most of the clients can still
use a pure string for a full match) and ensure the safety (for the prefix-id
case, prefix-\d+ should be safer to avoid unexpected matches to prefix-ABC or
prefix-.)

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

Can you think of a scenario where a regex is required?

Using something like 'channel-name' is simple and difficult to introduce bugs. 
Using regex for the channel name when the flexibility is not required just
overcomplicates usage of the class.

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

By using regex, you can still use "channel-name" without any impact. But for
advanced usage, say "prefix-\d+" case I have mentioned in the previous comment,
it can provide a more stable match mechanism.

+                              HandlerConstructor constructor);
+
+  // Creates a DataChannelHandler to handle the new incoming data channel. This
+  // function fails and returns false if no registered factory function matches
+  // |name|.

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

The last sentence is a bit awkward.  It is totally reasonable that no factory is
registered for a message (consider having a older host connecting to a newer
client).

Reword to:
Returns true if a handler exists for the new data channel.

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

Done.

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