[Chromoting] Add DataChannelManager to manage optional incoming data channels

remoting/protocol/data_channel_manager.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_manager.h"
+
+#include <utility>
+
+#include "base/bind.h"
+#include "base/location.h"
+#include "remoting/protocol/data_channel_handler.h"
+#include "third_party/re2/src/re2/re2.h"
+
+namespace remoting {
+namespace protocol {
+
+DataChannelManager::DataChannelManager() = default;
+DataChannelManager::~DataChannelManager() = default;
+
+bool DataChannelManager::RegisterHandlerFactory(
+    const std::string& regex,
+    HandlerConstructor constructor) {

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

I think factory is a better term than constructor since you aren't registering a
constructor but rather a function that returns an instance of a handler.

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

Done.

+  if (regex.empty() || !constructor) {
+    return false;
+  }
+
+  constructors_.push_back(std::make_pair(regex, constructor));

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

Do you want to check for double-registration (i.e. if two callers register the
same regex, only one will get called, the other will not).

Perhaps using a Dictionary would be good here to check for message handler
existence to prevent this type of problem.

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

Yes, I considered this issue before, but since regex is using here and the pipe
name is coming from the peer, we cannot guarantee no pipe name can match two or
more regexes.
i.e. both ".*" and ".+" match an unempty string.
I can add a DEBUG section in OnIncomingDataChannel to ensure only one match
happens.

Using prefix matching reduces the flexibility, so I prefer regex here.

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

Flexibility doesn't necessarily lead to a good API.  Can you think of a use case
where a regex is required?

Image searching for the handler for a certain message type.  In our existing
codebase, I know the name of the channel/message type and can search for the
string to see who handles it.  If it is a regex, that may not be true since they
can take any form.

If there is a solid use case, then a regex is OK, but otherwise I don't
personally see the value.

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

As I have mentioned in another comment, using regex won't increase the
complexity for regular full match, but provide a more stable matching mechanism
then using prefix.

+  return true;
+}
+
+bool DataChannelManager::OnIncomingDataChannel(
+    const std::string& name,
+    std::unique_ptr<MessagePipe> pipe) {
+  for (auto& constructor : constructors_) {

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

Using a dictionary would be cleaner here.  You could retrieve the factory
instance using the message-type portion of the name and then use the factory
directly (instead of using the second member of a pair).

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

A dictionary cannot handle prefix matches.

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

Sure it can.  The prefix is the name of the channel.  If we define a format for
channel names, say {name}::{instance_id} then the user of this interface passes
in "name".  Name is then a key in the Dict and you can check for uniqueness.

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

Then it will introduce another magic character or string (no matter - or ::) .
This seems not right to me because we are using an existing label()
(https://cs.chromium.org/chromium/src/third_party/webrtc/api/datachannelinterf...)
instead of a newly defined string. What if the magic string we are using is
disallowed in other data channel implementations?

+    if (re2::RE2::FullMatch(name, constructor.first)) {
+      constructor.second.Run(name, std::move(pipe));
+      return true;
+    }
+  }
+  return false;
+}
+
+}  // namespace protocol
+}  // namespace remoting