[Chromoting] Use ProcessStatsSender in DesktopSessionAgent (desktop process)

remoting/host/desktop_session_agent_unittest.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/host/desktop_session_agent.h"
+
+#include <memory>
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "base/single_thread_task_runner.h"
+#include "ipc/ipc_channel_proxy.h"
+#include "ipc/ipc_listener.h"
+#include "remoting/base/auto_thread_task_runner.h"
+#include "remoting/host/chromoting_messages.h"
+#include "remoting/host/desktop_environment_options.h"
+#include "remoting/host/fake_desktop_environment.h"
+#include "remoting/host/screen_resolution.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace remoting {
+
+namespace {
+
+class MockDelegate : public DesktopSessionAgent::Delegate {

[joedow, 2017/06/15 22:50:44]

Sorry I missed this in the first iteration, but this isn't a Mock, FakeDelegate
or TestDelegate is a more accurate name.

[Hzj_jie, 2017/06/16 00:39:28]

It looks like I still cannot choose the right name between Mock and Fake. :(

+ public:
+  MockDelegate(scoped_refptr<base::SingleThreadTaskRunner> runner);
+  ~MockDelegate() override = default;
+
+  DesktopEnvironmentFactory& desktop_environment_factory() override {
+    return factory_;
+  }
+
+  void OnNetworkProcessDisconnected() override {}
+
+  base::WeakPtr<Delegate> GetWeakPtr() { return weak_ptr_.GetWeakPtr(); }
+
+ private:
+  FakeDesktopEnvironmentFactory factory_;
+
+  base::WeakPtrFactory<MockDelegate> weak_ptr_;
+};
+
+MockDelegate::MockDelegate(scoped_refptr<base::SingleThreadTaskRunner> runner)
+    : factory_(runner),
+      weak_ptr_(this) {}
+
+class ProcessStatsListener : public IPC::Listener {
+ public:
+  ProcessStatsListener(base::Closure action_after_received)
+      : action_after_received_(action_after_received) {}
+
+  ~ProcessStatsListener() override = default;
+
+ private:
+  // IPC::Listener implementation.
+  bool OnMessageReceived(const IPC::Message& message) override;
+
+  void OnProcessResourceUsage(
+      const remoting::protocol::AggregatedProcessResourceUsage& usage);
+
+  const base::Closure action_after_received_;
+};
+
+bool ProcessStatsListener::OnMessageReceived(const IPC::Message& message) {
+  bool handled = false;
+  IPC_BEGIN_MESSAGE_MAP(ProcessStatsListener, message)
+    IPC_MESSAGE_HANDLER(ChromotingAnyToNetworkMsg_ReportProcessStats,
+                        OnProcessResourceUsage);
+    IPC_MESSAGE_UNHANDLED(handled = false);
+  IPC_END_MESSAGE_MAP()
+  return handled;
+}
+
+void ProcessStatsListener::OnProcessResourceUsage(
+    const remoting::protocol::AggregatedProcessResourceUsage& usage) {
+  action_after_received_.Run();
+}
+
+}  // namespace
+
+class DesktopSessionAgentTest : public ::testing::Test {
+ public:
+  DesktopSessionAgentTest();
+  ~DesktopSessionAgentTest() override = default;
+
+  void Shutdown();
+
+ protected:
+  base::MessageLoop message_loop_;
+  base::RunLoop run_loop_;
+  scoped_refptr<AutoThreadTaskRunner> task_runner_;
+  scoped_refptr<DesktopSessionAgent> agent_;
+};
+
+DesktopSessionAgentTest::DesktopSessionAgentTest()
+    : task_runner_(new AutoThreadTaskRunner(
+          message_loop_.task_runner(), run_loop_.QuitClosure())),
+      agent_(new DesktopSessionAgent(
+          task_runner_, task_runner_, task_runner_, task_runner_)) {}
+
+void DesktopSessionAgentTest::Shutdown() {
+  task_runner_ = nullptr;
+  agent_->Stop();
+  agent_ = nullptr;
+  ASSERT_FALSE(static_cast<bool>(task_runner_));

[joedow, 2017/06/15 22:50:44]

I might be missing something, but you assign nullptr to task_runner_ and agent_
above, how would these two ASSERTS ever fire?

[Hzj_jie, 2017/06/16 00:39:28]

No, it's my fault. They should be removed.

+  ASSERT_FALSE(static_cast<bool>(agent_));

[joedow, 2017/06/15 22:50:44]

If you do need to keep the ASSERTS, I think ASSERT_EQ(agent_, nullptr) is more
readable than trying to boolify the ptrs.

[Hzj_jie, 2017/06/16 00:39:28]

Done.

+}
+
+TEST_F(DesktopSessionAgentTest, StartProcessStatsReport) {
+  std::unique_ptr<MockDelegate> delegate(new MockDelegate(task_runner_));
+  std::unique_ptr<IPC::ChannelProxy> proxy;
+  ProcessStatsListener listener(base::Bind([](
+          DesktopSessionAgentTest* test,
+          std::unique_ptr<MockDelegate>* delegate,
+          std::unique_ptr<IPC::ChannelProxy>* proxy) {
+        test->Shutdown();
+        delegate->reset();
+        proxy->reset();
+        ASSERT_FALSE(static_cast<bool>(*delegate));
+        ASSERT_FALSE(static_cast<bool>(*proxy));
+      },
+      base::Unretained(this),
+      base::Unretained(&delegate),
+      base::Unretained(&proxy)));
+  proxy = IPC::ChannelProxy::Create(
+      agent_->Start(delegate->GetWeakPtr()).release(),
+      IPC::Channel::MODE_CLIENT,
+      &listener,
+      task_runner_);
+  ASSERT_TRUE(proxy->Send(new ChromotingNetworkDesktopMsg_StartSessionAgent(
+      "jid", ScreenResolution(), DesktopEnvironmentOptions())));
+  ASSERT_TRUE(proxy->Send(
+      new ChromotingNetworkToAnyMsg_StartProcessStatsReport(1)));
+  run_loop_.Run();

[joedow, 2017/06/15 22:50:44]

Can you add a test where you send a stop MSG?

[Hzj_jie, 2017/06/16 00:39:28]

Done.

+}
+
+}  // namespace remoting