[Chromoting] Add DataChannelManager to manage optional incoming data channels

remoting/protocol/data_channel_manager_unittest.cc

Index: remoting/protocol/data_channel_manager_unittest.cc
diff --git a/remoting/protocol/data_channel_manager_unittest.cc b/remoting/protocol/data_channel_manager_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..3e9add3a9287bd74e12f5ed259427c52706980fe
--- /dev/null
+++ b/remoting/protocol/data_channel_manager_unittest.cc
@@ -0,0 +1,183 @@
+// 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 <map>
+#include <utility>
+
+#include "base/bind.h"
+#include "base/memory/ptr_util.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "remoting/base/compound_buffer.h"
+#include "remoting/proto/process_stats.pb.h"
+#include "remoting/protocol/data_channel_handler.h"
+#include "remoting/protocol/fake_message_pipe.h"
+#include "remoting/protocol/fake_message_pipe_wrapper.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace remoting {
+namespace protocol {
+
+namespace {
+
+class FakeDataChannelHandler final : public DataChannelHandler {
+ public:
+  FakeDataChannelHandler(const std::string& name,
+                         std::unique_ptr<MessagePipe> pipe,
+                         const std::string& expected_data)
+      : DataChannelHandler(name, std::move(pipe)),
+        expected_data_(expected_data) {
+    handlers_[name] = this;
+  }
+
+  void OnIncomingMessage(std::unique_ptr<CompoundBuffer> message) override {

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

nit: I think it is cleaner to break the impls out from the class declaration for
test code.  It can be in the same file still but does improve readability.

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

Done.

+    ASSERT_TRUE(message != nullptr);
+    std::string content;
+    content.resize(expected_data_.size());
+    message->CopyTo(&(content[0]), content.size());
+    ASSERT_EQ(content, expected_data_);
+    received_message_count_++;
+  }
+
+  int received_message_count() const { return received_message_count_; }
+
+  static FakeDataChannelHandler* Find(const std::string& name) {
+    auto it = handlers_.find(name);
+    if (it == handlers_.end()) {
+      return nullptr;
+    }
+    return it->second;
+  }
+
+  static int instance_count() { return handlers_.size(); }
+
+ protected:
+  ~FakeDataChannelHandler() override {
+    EXPECT_EQ(handlers_.erase(pipe_name()), 1U);
+  }
+
+ private:
+  static std::map<std::string, FakeDataChannelHandler*> handlers_;
+
+  const std::string expected_data_;
+  int received_message_count_ = 0;
+};
+
+// static
+std::map<std::string, FakeDataChannelHandler*>
+FakeDataChannelHandler::handlers_;
+
+void TestDataChannelManager(bool asynchronous) {

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

This is a *big* test method that does a lot of stuff.  It would be better to set
up specific tests for each feature you are testing (i.e.):
- test partial match
- test full match
- test full / partial match
- test duplicate regex

etc.  This would make the tests more maintainable and easier to fix/understand.

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

Done.

+  base::MessageLoop message_loop;
+  DataChannelManager manager;
+  ASSERT_TRUE(manager.RegisterHandlerFactory("FullMatch", base::Bind(
+      [](const std::string& expected_data,
+         const std::string& name,
+         std::unique_ptr<MessagePipe> pipe) -> void {
+        new FakeDataChannelHandler(name, std::move(pipe), expected_data);
+      },
+      "FullMatchContent")));
+
+  ASSERT_TRUE(manager.RegisterHandlerFactory("Prefix.*", base::Bind(
+      [](const std::string& expected_data,
+         const std::string& name,
+         std::unique_ptr<MessagePipe> pipe) -> void {
+        new FakeDataChannelHandler(name, std::move(pipe), expected_data);
+      },
+      "PrefixMatchContent")));
+
+  FakeMessagePipe pipe1(asynchronous);
+  FakeMessagePipe pipe2(asynchronous);
+  FakeMessagePipe pipe3(asynchronous);
+  FakeMessagePipe pipe4(asynchronous);
+  FakeMessagePipe pipe5(asynchronous);
+  ASSERT_TRUE(manager.OnIncomingDataChannel("FullMatch", pipe1.Wrap()));
+  ASSERT_TRUE(manager.OnIncomingDataChannel("Prefix1", pipe2.Wrap()));
+  ASSERT_TRUE(manager.OnIncomingDataChannel("Prefix2", pipe3.Wrap()));
+  ASSERT_TRUE(manager.OnIncomingDataChannel("Prefix3", pipe4.Wrap()));
+  ASSERT_FALSE(manager.OnIncomingDataChannel("Unknown", pipe5.Wrap()));
+  pipe1.OpenPipe();
+  pipe2.OpenPipe();
+  pipe3.OpenPipe();
+
+  base::RunLoop().RunUntilIdle();
+
+  FakeDataChannelHandler* handler1 = FakeDataChannelHandler::Find("FullMatch");
+  FakeDataChannelHandler* handler2 = FakeDataChannelHandler::Find("Prefix1");
+  FakeDataChannelHandler* handler3 = FakeDataChannelHandler::Find("Prefix2");
+  FakeDataChannelHandler* handler4 = FakeDataChannelHandler::Find("Prefix3");
+  FakeDataChannelHandler* handler5 = FakeDataChannelHandler::Find("Unknown");
+  ASSERT_TRUE(handler1 != nullptr);
+  ASSERT_TRUE(handler2 != nullptr);
+  ASSERT_TRUE(handler3 != nullptr);
+  ASSERT_TRUE(handler4 != nullptr);
+  ASSERT_TRUE(handler5 == nullptr);
+
+  {
+    ProcessResourceUsage usage;

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

Can you avoid bringing in external dependencies into the test?  Can we just
define a test message type at the top of this file and use that instead?

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

This is not doable since proto is automatically generated from .proto file. I
will add a new proto to avoid the dependency.

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

Thanks!

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

Done.

+    int sent = 0;
+    base::Closure sent_callback = base::Bind([](int* sent) {
+          (*sent)++;
+        },
+        base::Unretained(&sent));
+    ASSERT_TRUE(handler1->Send(&usage, sent_callback));
+    ASSERT_TRUE(handler2->Send(&usage, sent_callback));
+    ASSERT_TRUE(handler3->Send(&usage, sent_callback));
+    ASSERT_FALSE(handler4->Send(&usage, sent_callback));
+
+    base::RunLoop().RunUntilIdle();
+    ASSERT_EQ(sent, 3);
+  }
+
+  {
+    std::string content;
+    auto message = base::MakeUnique<CompoundBuffer>();
+    content = "FullMatchContent";
+    message->AppendCopyOf(&(content[0]), content.size());
+    pipe1.Receive(std::move(message));
+
+    message = base::MakeUnique<CompoundBuffer>();
+    content = "PrefixMatchContent";
+    message->AppendCopyOf(&(content[0]), content.size());
+    pipe2.Receive(std::move(message));
+
+    message = base::MakeUnique<CompoundBuffer>();
+    content = "PrefixMatchContent";
+    message->AppendCopyOf(&(content[0]), content.size());
+    pipe3.Receive(std::move(message));
+
+    base::RunLoop().RunUntilIdle();
+    ASSERT_EQ(handler1->received_message_count(), 1);
+    ASSERT_EQ(handler2->received_message_count(), 1);
+    ASSERT_EQ(handler3->received_message_count(), 1);
+  }
+
+  pipe2.ClosePipe();
+  base::RunLoop().RunUntilIdle();
+  ASSERT_TRUE(FakeDataChannelHandler::Find("Prefix1") == nullptr);
+
+  handler3->Close();
+  base::RunLoop().RunUntilIdle();
+  ASSERT_TRUE(FakeDataChannelHandler::Find("Prefix2") == nullptr);
+
+  pipe1.ClosePipe();
+  handler4->Close();
+  base::RunLoop().RunUntilIdle();
+  ASSERT_EQ(FakeDataChannelHandler::instance_count(), 0);
+}
+
+}  // namespace
+
+TEST(DataChannelManagerTest, SynchronousPipe) {
+  TestDataChannelManager(false);
+}
+
+TEST(DataChannelManagerTest, AsynchronousPipe) {
+  TestDataChannelManager(true);
+}
+
+}  // namespace protocol
+}  // namespace remoting