[Chromoting] Use device::PowerSaveBlocker to block screen saver

remoting/host/host_power_save_blocker_unittest.cc

+// Copyright 2016 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/host_power_save_blocker.h"
+
+#include <memory>
+
+#include "base/logging.h"
+#include "base/threading/thread.h"
+#include "remoting/host/fake_host_status_monitor.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace remoting {
+
+class HostPowerSaveBlockerTest : public testing::Test {
+ public:
+  HostPowerSaveBlockerTest();
+
+ protected:
+  void AssertActivated();
+  void AssertDeactivated();
+
+ private:

[Sergey Ulanov, 2016/07/14 19:52:48]

All of this variables can be protected. There is no reason to hide them from the
tests.

[Hzj_jie, 2016/07/14 23:20:04]

Done.

+  base::Thread ui_thread_;

[Sergey Ulanov, 2016/07/14 19:52:48]

On UI thread must be the main application thread. Create MessageLoopForUI for
the main thread here

[Hzj_jie, 2016/07/14 23:20:03]

Done.

+  base::Thread block_thread_;

[Sergey Ulanov, 2016/07/14 19:52:48]

blocking_io_thread_ or file_thread_?

[Hzj_jie, 2016/07/14 23:20:03]

Follow PowerSaveBlocker definition. It's a little bit confusion, but I believe
it should be a blocking io thread.

[Sergey Ulanov, 2016/07/14 23:30:57]

Call it blocking_thread_? block_thread_ is confusing.

[Hzj_jie, 2016/07/15 02:12:00]

Oh, sorry, I missed ing.

+  FakeHostStatusMonitor monitor_;
+
+ protected:
+  std::unique_ptr<HostPowerSaveBlocker> blocker_;
+};
+
+HostPowerSaveBlockerTest::HostPowerSaveBlockerTest()
+    : ui_thread_("ui-thread"),
+      block_thread_("block-thread") {
+  DCHECK(ui_thread_.Start());

[Sergey Ulanov, 2016/07/14 19:52:48]

this won't start the thread in release builds. Never put an expression with
side-effects inside DCHECK(). Give that this is a unittest, ASSERT_TRUE() would
be more appropriate here.

[Hzj_jie, 2016/07/14 23:20:03]

ASSERT_TRUE won't work in constructor (It returns an error message.)

[Sergey Ulanov, 2016/07/14 23:30:57]

It should work. It doesn't return any result. It will simply return from the
constructor when assertion fails.

[Hzj_jie, 2016/07/15 02:12:00]

Emmm... Here is the error message,
../../remoting/host/host_power_save_blocker_unittest.cc:32:3: error: constructor
'HostPowerSaveBlockerTest' must not return void expression
  ASSERT_TRUE(blocking_thread_.StartWithOptions(
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../testing/gtest/include/gtest/gtest.h:1871:23: note: expanded from macro
'ASSERT_TRUE'
                      GTEST_FATAL_FAILURE_)
                      ^~~~~~~~~~~~~~~~~~~~~
../../testing/gtest/include/gtest/internal/gtest-internal.h:1192:5: note:
expanded from macro 'GTEST_TEST_BOOLEAN_'
    fail(::testing::internal::GetBoolAssertionFailureMessage(\
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../testing/gtest/include/gtest/internal/gtest-internal.h:1110:3: note:
expanded from macro 'GTEST_FATAL_FAILURE_'
  return GTEST_MESSAGE_(message, ::testing::TestPartResult::kFatalFailure)
  ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[Sergey Ulanov, 2016/07/15 06:15:33]

"must not return void expression" gah, C++, this is so dumb!

Then I guess you can move initialization to SetUp() method, which actually
returns void? 

+  DCHECK(ui_thread_.WaitUntilThreadStarted());

[Sergey Ulanov, 2016/07/14 19:52:48]

Why?
If you really need it then use StartAndWaitForTesting()

[Hzj_jie, 2016/07/14 23:20:03]

Done.

+  DCHECK(block_thread_.StartWithOptions(
+      base::Thread::Options(base::MessageLoop::TYPE_IO, 0)));
+  DCHECK(block_thread_.WaitUntilThreadStarted());
+  blocker_.reset(new HostPowerSaveBlocker(monitor_.AsWeakPtr(),
+                                          ui_thread_.task_runner(),
+                                          block_thread_.task_runner()));
+}
+
+void HostPowerSaveBlockerTest::AssertActivated() {
+  ASSERT_TRUE(blocker_->blocker_);
+}
+
+void HostPowerSaveBlockerTest::AssertDeactivated() {

[Sergey Ulanov, 2016/07/14 19:52:48]

You can replaced these two methods with bool is_activated() and ASSERT in the
test.

[Hzj_jie, 2016/07/14 23:20:04]

Done.

+  ASSERT_FALSE(blocker_->blocker_);
+}
+
+TEST_F(HostPowerSaveBlockerTest, Activated) {
+  blocker_->OnClientConnected("jid/jid1@jid2.org");
+  AssertActivated();
+  blocker_->OnClientDisconnected("jid/jid3@jid4.org");
+  AssertDeactivated();
+}
+
+}  // namespace remoting