chromeos: power: Refactor RendererFreezer and add tests

chrome/browser/chromeos/power/renderer_freezer_unittest.cc

Index: chrome/browser/chromeos/power/renderer_freezer_unittest.cc
diff --git a/chrome/browser/chromeos/power/renderer_freezer_unittest.cc b/chrome/browser/chromeos/power/renderer_freezer_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..6889290c76362140f008acb72d13943f2fc6a600
--- /dev/null
+++ b/chrome/browser/chromeos/power/renderer_freezer_unittest.cc
@@ -0,0 +1,262 @@
+// Copyright 2014 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 "chrome/browser/chromeos/power/renderer_freezer.h"
+
+#include <string>
+
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/fake_power_manager_client.h"
+#include "testing/gtest/include/gtest/gtest-death-test.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromeos {
+
+namespace {
+// Class that delegates used in testing can inherit from to record calls that
+// are made by the code being tested.
+class ActionRecorder {
+ public:
+  ActionRecorder();
+  virtual ~ActionRecorder();
+
+  // Returns a comma-separated string describing the actions that were
+  // requested since the previous call to GetActions() (i.e. results are
+  // non-repeatable).
+  std::string GetActions();
+
+ protected:
+  // Appends |new_action| to |actions_|, using a comma as a separator if
+  // other actions are already listed.
+  void AppendAction(const std::string& new_action);
+
+ private:
+  // Comma-separated list of actions that have been performed.
+  std::string actions_;
+
+  DISALLOW_COPY_AND_ASSIGN(ActionRecorder);
+};
+
+ActionRecorder::ActionRecorder() {
+}
+
+ActionRecorder::~ActionRecorder() {
+}
+
+std::string ActionRecorder::GetActions() {
+  std::string actions = actions_;
+  actions_.clear();
+  return actions;
+}
+
+void ActionRecorder::AppendAction(const std::string& new_action) {
+  if (!actions_.empty())
+    actions_ += ",";
+  actions_ += new_action;
+}
+
+// Actions that can be returned by TestDelegate::GetActions().
+const char kFreezeRenderers[] = "freeze_renderers";
+const char kThawRenderers[] = "thaw_renderers";
+const char kNoActions[] = "";
+
+// Test implementation of RendererFreezer::Delegate that records the actions it
+// was asked to perform.
+class TestDelegate : public RendererFreezer::Delegate, public ActionRecorder {
+ public:
+  TestDelegate();
+  virtual ~TestDelegate();
+
+  // RendererFreezer::Delegate overrides.
+  virtual bool FreezeRenderers() OVERRIDE;
+  virtual bool ThawRenderers() OVERRIDE;
+  virtual bool CanFreezeRenderers() OVERRIDE;
+
+  void set_freeze_renderers_result(bool result);
+  void set_thaw_renderers_result(bool result);
+
+  // Sets whether the delegate is capable of freezing renderers.  This also
+  // changes |freeze_renderers_result_| and |thaw_renderers_result_|.
+  void set_can_freeze_renderers(bool can_freeze);
+
+ private:
+  bool can_freeze_renderers_;
+  bool freeze_renderers_result_;
+  bool thaw_renderers_result_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestDelegate);
+};
+
+TestDelegate::TestDelegate()

[Daniel Erat, 2014/09/06 02:01:25]

all of these implementations are short enough that i'd probably inline them in
the above class definition to avoid needing to duplicate all of these signatures
and make the code easier to read. (it's fine to inline in classes that are
defined in .cc files rather than headers.)

+    : can_freeze_renderers_(true),
+      freeze_renderers_result_(true),
+      thaw_renderers_result_(true) {
+}
+
+TestDelegate::~TestDelegate() {
+}
+
+bool TestDelegate::FreezeRenderers() {
+  AppendAction(kFreezeRenderers);
+
+  return freeze_renderers_result_;
+}
+
+bool TestDelegate::ThawRenderers() {
+  AppendAction(kThawRenderers);
+
+  return thaw_renderers_result_;
+}
+
+bool TestDelegate::CanFreezeRenderers() {
+  return can_freeze_renderers_;
+}
+
+void TestDelegate::set_can_freeze_renderers(bool can_freeze) {
+  can_freeze_renderers_ = can_freeze;
+
+  // If the delegate cannot freeze renderers, then the result of trying to do so
+  // will be false.
+  freeze_renderers_result_ = can_freeze;
+  thaw_renderers_result_ = can_freeze;
+}
+
+void TestDelegate::set_freeze_renderers_result(bool result) {
+  freeze_renderers_result_ = result;
+}
+
+void TestDelegate::set_thaw_renderers_result(bool result) {
+  thaw_renderers_result_ = result;
+}
+
+}  // namespace
+
+class RendererFreezerTest : public testing::Test {
+ public:
+  RendererFreezerTest();
+  virtual ~RendererFreezerTest();
+
+  void Init();
+
+ protected:
+  FakePowerManagerClient* fake_power_manager_client_;

[Daniel Erat, 2014/09/06 02:01:25]

nit: name this |client_| to avoid awkward-looking wrapping later?

+  TestDelegate* test_delegate_;
+
+  scoped_ptr<RendererFreezer> renderer_freezer_;

[Daniel Erat, 2014/09/06 02:01:25]

nit: name this |freezer_|? (feel free to ignore if it doesn't help with
wrapping)

+
+ private:
+  base::MessageLoop message_loop_;
+  DISALLOW_COPY_AND_ASSIGN(RendererFreezerTest);
+};
+
+RendererFreezerTest::RendererFreezerTest()

[Daniel Erat, 2014/09/06 02:01:25]

i'd recommend inlining these too

+    : fake_power_manager_client_(new FakePowerManagerClient()),
+      test_delegate_(new TestDelegate()) {
+}
+
+RendererFreezerTest::~RendererFreezerTest() {
+  renderer_freezer_.reset();
+
+  // This should never be false since all tests should be calling Init() but it
+  // doesn't hurt to check.
+  if (DBusThreadManager::IsInitialized())
+    DBusThreadManager::Shutdown();
+}
+
+void RendererFreezerTest::Init() {
+  DBusThreadManager::GetSetterForTesting()->SetPowerManagerClient(

[Daniel Erat, 2014/09/06 02:01:25]

nit: probably a bit cleaner to do this in the c'tor and just leave the
RendererFreezer creation in Init()

+      scoped_ptr<PowerManagerClient>(fake_power_manager_client_));
+
+  renderer_freezer_.reset(new RendererFreezer(
+      scoped_ptr<RendererFreezer::Delegate>(test_delegate_)));
+}
+
+// Tests that the RendererFreezer freezes renderers on suspend and thaws them on
+// resume.
+TEST_F(RendererFreezerTest, SuspendResume) {
+  Init();
+
+  fake_power_manager_client_->SendSuspendImminent();
+
+  // The RendererFreezer should have grabbed an asynchronous callback and done
+  // nothing else.
+  EXPECT_EQ(
+      1, fake_power_manager_client_->GetNumPendingSuspendReadinessCallbacks());
+  EXPECT_EQ(kNoActions, test_delegate_->GetActions());
+
+  // The RendererFreezer should eventually freeze the renderers and run the
+  // callback.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(
+      0, fake_power_manager_client_->GetNumPendingSuspendReadinessCallbacks());
+  EXPECT_EQ(kFreezeRenderers, test_delegate_->GetActions());
+
+  // The renderers should be thawed when we resume.
+  fake_power_manager_client_->SendSuspendDone();
+  EXPECT_EQ(kThawRenderers, test_delegate_->GetActions());
+}
+
+// Tests that the renderer freezer does nothing if the delegate cannot freeze
+// renderers.
+TEST_F(RendererFreezerTest, DelegateCannotFreezeRenderers) {
+  test_delegate_->set_can_freeze_renderers(false);
+  Init();
+
+  fake_power_manager_client_->SendSuspendImminent();
+
+  // The RendererFreezer should not have grabbed a callback or done anything
+  // else.
+  EXPECT_EQ(
+      0, fake_power_manager_client_->GetNumPendingSuspendReadinessCallbacks());
+  EXPECT_EQ(kNoActions, test_delegate_->GetActions());
+
+  // There should be nothing in the message loop.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(kNoActions, test_delegate_->GetActions());
+
+  // Nothing happens on resume.
+  fake_power_manager_client_->SendSuspendDone();
+  EXPECT_EQ(kNoActions, test_delegate_->GetActions());
+}
+
+// Tests that the RendererFreezer does nothing on resume if the freezing
+// operation was unsuccessful.
+TEST_F(RendererFreezerTest, ErrorFreezingRenderers) {
+  Init();
+  test_delegate_->set_freeze_renderers_result(false);
+
+  fake_power_manager_client_->SendSuspendImminent();
+  EXPECT_EQ(
+      1, fake_power_manager_client_->GetNumPendingSuspendReadinessCallbacks());
+
+  // The freezing operation should fail, but we should still report readiness.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(kFreezeRenderers, test_delegate_->GetActions());
+  EXPECT_EQ(
+      0, fake_power_manager_client_->GetNumPendingSuspendReadinessCallbacks());
+
+  // Since the delegate reported that the freezing was unsuccessful, don't do
+  // anything on resume.
+  fake_power_manager_client_->SendSuspendDone();
+  EXPECT_EQ(kNoActions, test_delegate_->GetActions());
+}
+
+#if defined(GTEST_HAS_DEATH_TEST)
+// Tests that the RendererFreezer crashes the browser if the freezing operation
+// was successful but the thawing operation failed.
+TEST_F(RendererFreezerTest, ErrorThawingRenderers) {
+  Init();
+  test_delegate_->set_thaw_renderers_result(false);
+
+  fake_power_manager_client_->SendSuspendImminent();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(kFreezeRenderers, test_delegate_->GetActions());
+
+  EXPECT_DEATH(fake_power_manager_client_->SendSuspendDone(), "Unable to thaw");
+}
+#endif  // GTEST_HAS_DEATH_TEST
+
+}  // namespace chromeos