Use WallClockTimer in DeviceOffHoursController

DeviceOffHours policy operates with a "real" clock: it has to know the
exact time (day, hours, minutes) to make a decision. One of things which
affect real clock is suspend: from software's perspective it's a jump
into future. Therefore DeviceOffHoursController caught OnSuspendDone
event from the power manager to reschedule its timer.

WallClockTimer is the class which automatically reschedules after
suspend, therefore instead of duplicating this logic in
DeviceOffHoursController we can just reuse WallClockTimer.

Also this CL makes a number of changes in tests:
 * An access method for fake power manager client is removed from
DeviceSettingsTestBase, as tests for DeviceOffHoursController were the
ones which used it.
 * DeviceSettingsTestBase now has an additional constructor to override
time source (mocked or system) for task environment it creates. This is
needed because we have to mock time to test suspend, but not all other
tests support mocked time. More on that in https://crbug.com/1129894.
 * CheckSendSuspendDone test for DeviceOffHoursController is renamed to
CheckUnderSuspend, since it simulates suspend, not only checks that
sending OnSuspentEvent to DeviceOffHoursController may trigger the
update.

Bug: b:160682243
Change-Id: I403964452e5eeb7470fd731e6760b7b0dec27456
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2418816Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Sergey Poromov <poromov@chromium.org>
Commit-Queue: Oleg Davydov <burunduk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809873}

chrome/browser/chromeos/policy/off_hours/device_off_hours_controller.cc

@@ -30,7 +30,7 @@ namespace policy {
 namespace off_hours {
 
 DeviceOffHoursController::DeviceOffHoursController()
-    : timer_(std::make_unique<base::OneShotTimer>()),
+    : timer_(std::make_unique<util::WallClockTimer>()),
       clock_(base::DefaultClock::GetInstance()) {
   auto* system_clock_client = chromeos::SystemClockClient::Get();
   if (system_clock_client) {
@@ -39,17 +39,11 @@ DeviceOffHoursController::DeviceOffHoursController()
         base::BindOnce(&DeviceOffHoursController::SystemClockInitiallyAvailable,
                        weak_ptr_factory_.GetWeakPtr()));
   }
-
-  if (chromeos::PowerManagerClient::Get())
-    chromeos::PowerManagerClient::Get()->AddObserver(this);
 }
 
 DeviceOffHoursController::~DeviceOffHoursController() {
   if (chromeos::SystemClockClient::Get())
     chromeos::SystemClockClient::Get()->RemoveObserver(this);
-
-  if (chromeos::PowerManagerClient::Get())
-    chromeos::PowerManagerClient::Get()->RemoveObserver(this);
 }
 
 void DeviceOffHoursController::AddObserver(Observer* observer) {
@@ -64,7 +58,7 @@ void DeviceOffHoursController::SetClockForTesting(
     base::Clock* clock,
     const base::TickClock* timer_clock) {
   clock_ = clock;
-  timer_ = std::make_unique<base::OneShotTimer>(timer_clock);
+  timer_ = std::make_unique<util::WallClockTimer>(clock, timer_clock);
 }
 
 bool DeviceOffHoursController::IsCurrentSessionAllowedOnlyForOffHours() const {
@@ -111,13 +105,6 @@ void DeviceOffHoursController::UpdateOffHoursPolicy(
   UpdateOffHoursMode();
 }
 
-void DeviceOffHoursController::SuspendDone(
-    const base::TimeDelta& sleep_duration) {
-  // Triggered when device wakes up. "OffHours" state could be changed during
-  // sleep mode and should be updated after that.
-  UpdateOffHoursMode();
-}
-
 void DeviceOffHoursController::NotifyOffHoursEndTimeChanged() const {
   VLOG(1) << "OffHours end time is changed to " << off_hours_end_time_;
   for (auto& observer : observers_)
@@ -179,7 +166,7 @@ void DeviceOffHoursController::SetOffHoursMode(bool off_hours_enabled) {
 void DeviceOffHoursController::StartOffHoursTimer(base::TimeDelta delay) {
   DCHECK_GT(delay, base::TimeDelta());
   DVLOG(1) << "OffHours mode timer starts for " << delay;
-  timer_->Start(FROM_HERE, delay,
+  timer_->Start(FROM_HERE, clock_->Now() + delay,
                 base::BindOnce(&DeviceOffHoursController::UpdateOffHoursMode,
                                weak_ptr_factory_.GetWeakPtr()));
 }

chrome/browser/chromeos/policy/off_hours/device_off_hours_controller.h

@@ -13,8 +13,7 @@
 #include "base/observer_list.h"
 #include "base/time/clock.h"
 #include "base/time/time.h"
-#include "base/timer/timer.h"
-#include "chromeos/dbus/power/power_manager_client.h"
+#include "base/util/timer/wall_clock_timer.h"
 #include "chromeos/dbus/system_clock/system_clock_client.h"
 #include "chromeos/policy/weekly_time/weekly_time_interval.h"
 #include "components/policy/proto/chrome_device_policy.pb.h"
@@ -37,8 +36,7 @@ namespace off_hours {
 //
 // "OffHours" mode is never on until device time is synchronized with
 // network time because in this case device time could be incorrect.
-class DeviceOffHoursController : public chromeos::SystemClockClient::Observer,
-                                 public chromeos::PowerManagerClient::Observer {
+class DeviceOffHoursController : public chromeos::SystemClockClient::Observer {
  public:
   // Observer interface.
   class Observer {
@@ -75,9 +73,6 @@ class DeviceOffHoursController : public chromeos::SystemClockClient::Observer,
   // when "OffHours" mode is off.
   base::Time GetOffHoursEndTime() const { return off_hours_end_time_; }
 
-  // chromeos::PowerManagerClient::Observer:
-  void SuspendDone(const base::TimeDelta& sleep_duration) override;
-
   // chromeos::SystemClockClient::Observer:
   void SystemClockUpdated() override;
 
@@ -130,7 +125,7 @@ class DeviceOffHoursController : public chromeos::SystemClockClient::Observer,
 
   // Timer for updating device settings at the begin of next “OffHours” interval
   // or at the end of current "OffHours" interval.
-  std::unique_ptr<base::OneShotTimer> timer_;
+  std::unique_ptr<util::WallClockTimer> timer_;
 
   // Used for testing purposes, otherwise it's an instance of
   // base::DefaultClock.

chrome/browser/chromeos/policy/off_hours/device_off_hours_controller_unittest.cc

@@ -9,13 +9,14 @@
 
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
+#include "base/power_monitor/test/fake_power_monitor_source.h"
 #include "base/test/simple_test_clock.h"
 #include "base/test/simple_test_tick_clock.h"
+#include "base/test/task_environment.h"
 #include "base/time/tick_clock.h"
 #include "base/time/time.h"
 #include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
 #include "chrome/browser/chromeos/settings/device_settings_test_helper.h"
-#include "chromeos/dbus/power/fake_power_manager_client.h"
 #include "chromeos/dbus/system_clock/system_clock_client.h"
 #include "components/policy/proto/chrome_device_policy.pb.h"
 
@@ -102,7 +103,9 @@ void SetOffHoursPolicyToProto(em::ChromeDeviceSettingsProto* proto,
 class DeviceOffHoursControllerSimpleTest
     : public chromeos::DeviceSettingsTestBase {
  protected:
-  DeviceOffHoursControllerSimpleTest() = default;
+  DeviceOffHoursControllerSimpleTest()
+      : chromeos::DeviceSettingsTestBase(
+            base::test::TaskEnvironment::TimeSource::MOCK_TIME) {}
   ~DeviceOffHoursControllerSimpleTest() override = default;
 
   void SetUp() override {
@@ -323,21 +326,34 @@ class DeviceOffHoursControllerFakeClockTest
     system_clock_client()->NotifyObserversSystemClockUpdated();
     // Clocks are set to 1970-01-01 00:00:00 UTC, Thursday.
     test_clock_.SetNow(base::Time::UnixEpoch());
-    test_tick_clock_.SetNowTicks(base::TimeTicks::UnixEpoch());
-    device_off_hours_controller()->SetClockForTesting(&test_clock_,
-                                                      &test_tick_clock_);
+    device_off_hours_controller()->SetClockForTesting(
+        &test_clock_, task_environment_.GetMockTickClock());
   }
 
   void AdvanceTestClock(TimeDelta duration) {
     test_clock_.Advance(duration);
-    test_tick_clock_.Advance(duration);
+    task_environment_.FastForwardBy(duration);
+
+    task_environment_.RunUntilIdle();
+    base::RunLoop().RunUntilIdle();
+  }
+
+  void SuspendFor(TimeDelta duration) {
+    fake_power_monitor_source_.Suspend();
+
+    test_clock_.Advance(duration);
+
+    fake_power_monitor_source_.Resume();
+
+    task_environment_.RunUntilIdle();
+    base::RunLoop().RunUntilIdle();
   }
 
   base::Clock* clock() { return &test_clock_; }
 
  private:
   base::SimpleTestClock test_clock_;
-  base::SimpleTestTickClock test_tick_clock_;
+  base::test::ScopedFakePowerMonitorSource fake_power_monitor_source_;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceOffHoursControllerFakeClockTest);
 };
@@ -364,7 +380,7 @@ TEST_F(DeviceOffHoursControllerFakeClockTest, FakeClock) {
   EXPECT_FALSE(device_off_hours_controller()->is_off_hours_mode());
 }
 
-TEST_F(DeviceOffHoursControllerFakeClockTest, CheckSendSuspendDone) {
+TEST_F(DeviceOffHoursControllerFakeClockTest, CheckUnderSuspend) {
   system_clock_client()->SetServiceIsAvailable(true);
   int current_day_of_week = ExtractDayOfWeek(clock()->Now());
   LOG(ERROR) << "day " << current_day_of_week;
@@ -379,12 +395,10 @@ TEST_F(DeviceOffHoursControllerFakeClockTest, CheckSendSuspendDone) {
   UpdateDeviceSettings();
   EXPECT_FALSE(device_off_hours_controller()->is_off_hours_mode());
 
-  AdvanceTestClock(kDay);
-  power_manager_client()->SendSuspendDone();
+  SuspendFor(kDay);
   EXPECT_TRUE(device_off_hours_controller()->is_off_hours_mode());
 
   AdvanceTestClock(kHour);
-  power_manager_client()->SendSuspendDone();
   EXPECT_FALSE(device_off_hours_controller()->is_off_hours_mode());
 }
 

chrome/browser/chromeos/settings/device_settings_test_helper.cc

@@ -38,6 +38,10 @@ ScopedDeviceSettingsTestHelper::~ScopedDeviceSettingsTestHelper() {
 DeviceSettingsTestBase::DeviceSettingsTestBase()
     : task_environment_(content::BrowserTaskEnvironment::IO_MAINLOOP) {}
 
+DeviceSettingsTestBase::DeviceSettingsTestBase(
+    base::test::TaskEnvironment::TimeSource time)
+    : task_environment_(content::BrowserTaskEnvironment::IO_MAINLOOP, time) {}
+
 DeviceSettingsTestBase::~DeviceSettingsTestBase() {
   CHECK(teardown_called_);
 }
@@ -116,8 +120,4 @@ void DeviceSettingsTestBase::InitOwner(const AccountId& account_id,
     service->OnTPMTokenReady(true /* token is enabled */);
 }
 
-FakePowerManagerClient* DeviceSettingsTestBase::power_manager_client() {
-  return FakePowerManagerClient::Get();
-}
-
 }  // namespace chromeos

chrome/browser/chromeos/settings/device_settings_test_helper.h

@@ -11,6 +11,7 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/strings/string_util.h"
+#include "base/test/task_environment.h"
 #include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
 #include "chrome/browser/chromeos/policy/device_policy_builder.h"
 #include "chrome/browser/chromeos/settings/device_settings_service.h"
@@ -25,7 +26,6 @@ class TestingProfile;
 namespace chromeos {
 
 class DBusThreadManagerSetter;
-class FakePowerManagerClient;
 
 // Wraps the singleton device settings and initializes it to the point where it
 // reports OWNERSHIP_NONE for the ownership status.
@@ -46,6 +46,7 @@ class ScopedDeviceSettingsTestHelper {
 class DeviceSettingsTestBase : public testing::Test {
  protected:
   DeviceSettingsTestBase();
+  explicit DeviceSettingsTestBase(base::test::TaskEnvironment::TimeSource time);
   ~DeviceSettingsTestBase() override;
 
   // testing::Test:
@@ -64,8 +65,6 @@ class DeviceSettingsTestBase : public testing::Test {
 
   void InitOwner(const AccountId& account_id, bool tpm_is_ready);
 
-  FakePowerManagerClient* power_manager_client();
-
   content::BrowserTaskEnvironment task_environment_;
 
   std::unique_ptr<policy::DevicePolicyBuilder> device_policy_;