Night Light: Fix Timezone changes bug

Timezone changes should refresh the schedule for both sunset-to-sunrise and custom schedule types.

Bug: 1010981
Change-Id: Iec3ad0292cd016aac9dcf88fcdedd234f276311d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1849425Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Commit-Queue: Jun Liu <yjliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#705331}

ash/system/night_light/night_light_controller_impl.cc

@@ -481,10 +481,10 @@ void NightLightControllerImpl::SetCurrentGeoposition(
   StoreCachedGeoposition(position);
   delegate_->SetGeoposition(position);
 
-  // If the schedule type is sunset to sunrise, then a potential change in the
-  // geoposition might mean a change in the start and end times. In this case,
-  // we must trigger a refresh.
-  if (GetScheduleType() == ScheduleType::kSunsetToSunrise)
+  // If the schedule type is sunset to sunrise or custom, a potential change in
+  // the geoposition might mean timezone change as well as a change in the start
+  // and end times. In these cases, we must trigger a refresh.
+  if (GetScheduleType() != ScheduleType::kNone)
     Refresh(true /* did_schedule_change */);
 }
 

ash/system/night_light/night_light_controller_unittest.cc

@@ -585,7 +585,7 @@ TEST_F(NightLightTest, TestSunsetSunriseGeoposition) {
   //       now        sunset            sunrise
   //
   NightLightControllerImpl* controller = GetController();
-  delegate()->SetFakeNow(TimeOfDay(16 * 60));  // 4:00 PM.
+  delegate()->SetFakeNow(TimeOfDay(16 * 60));  // 4:00PM.
   controller->SetCurrentGeoposition(NightLightController::SimpleGeoposition{
       kFakePosition1_Latitude, kFakePosition1_Longitude});
 
@@ -642,6 +642,86 @@ TEST_F(NightLightTest, TestSunsetSunriseGeoposition) {
             controller->timer()->GetCurrentDelay());
 }
 
+// Tests the behavior when the client sets the geoposition while in custom
+// schedule setting. Current time is simulated to be updated accordingly. The
+// current time change should bring the controller into or take it out of the
+// night light mode accordingly if necessary, based on the settings.
+TEST_F(NightLightTest, TestCustomScheduleGeopositionChanges) {
+  constexpr int kCustom_Start = 19 * 60;
+  constexpr int kCustom_End = 2 * 60;
+
+  // Returns the positive difference in minutes given t1 and t2 in minutes
+  auto time_diff = [](int t1, int t2) {
+    int t = t2 - t1;
+    return t < 0 ? 24 * 60 + t : t;
+  };
+
+  NightLightControllerImpl* controller = GetController();
+  controller->SetCustomStartTime(TimeOfDay(kCustom_Start));
+  controller->SetCustomEndTime(TimeOfDay(kCustom_End));
+
+  // Position 1 current time and custom start and end time.
+  //
+  //      16:00       19:00             2:00
+  // <----- + --------- + --------------- + ------------->
+  //        |           |                 |
+  //       now     custom start      custom end
+  //
+
+  int fake_now = 16 * 60;
+  delegate()->SetFakeNow(TimeOfDay(fake_now));
+  controller->SetCurrentGeoposition(NightLightController::SimpleGeoposition{
+      kFakePosition1_Latitude, kFakePosition1_Longitude});
+
+  // Expect that timer is running and is scheduled at next custom start time.
+  controller->SetScheduleType(NightLightController::ScheduleType::kCustom);
+  EXPECT_FALSE(controller->GetEnabled());
+  TestCompositorsTemperature(0.0f);
+  EXPECT_TRUE(controller->timer()->IsRunning());
+  EXPECT_EQ(base::TimeDelta::FromMinutes(time_diff(fake_now, kCustom_Start)),
+            controller->timer()->GetCurrentDelay());
+
+  // Simulate a timezone change by changing geoposition.
+  // Current time updates to 9PM.
+  //      19:00       21:00       2:00
+  // <----- + --------- + -------- + --------------------->
+  //        |           |          |
+  //   custom start    now      custom end
+  //
+  fake_now = 21 * 60;
+  delegate()->SetFakeNow(TimeOfDay(fake_now));
+  controller->timer()->FireNow();
+  controller->SetCurrentGeoposition(NightLightController::SimpleGeoposition{
+      kFakePosition2_Latitude, kFakePosition2_Longitude});
+
+  // Expect the controller to enter night light mode and  the scheduled end
+  // delay has been updated.
+  EXPECT_TRUE(controller->GetEnabled());
+  TestCompositorsTemperature(controller->GetColorTemperature());
+  EXPECT_EQ(NightLightControllerImpl::AnimationDuration::kShort,
+            controller->last_animation_duration());
+  EXPECT_TRUE(controller->timer()->IsRunning());
+  EXPECT_EQ(base::TimeDelta::FromMinutes(time_diff(fake_now, kCustom_End)),
+            controller->timer()->GetCurrentDelay());
+
+  // Simulate user changing position back to location 1 and current time goes
+  // back to 4PM.
+  fake_now = 16 * 60;
+  delegate()->SetFakeNow(TimeOfDay(fake_now));
+  controller->timer()->FireNow();
+
+  controller->SetCurrentGeoposition(NightLightController::SimpleGeoposition{
+      kFakePosition1_Latitude, kFakePosition1_Longitude});
+  EXPECT_FALSE(controller->GetEnabled());
+  TestCompositorsTemperature(0.0f);
+  EXPECT_EQ(NightLightControllerImpl::AnimationDuration::kShort,
+            controller->last_animation_duration());
+  // Timer is running and is scheduled at next custom start time.
+  EXPECT_TRUE(controller->timer()->IsRunning());
+  EXPECT_EQ(base::TimeDelta::FromMinutes(time_diff(fake_now, kCustom_Start)),
+            controller->timer()->GetCurrentDelay());
+}
+
 // Tests the behavior when there is no valid geoposition for example due to lack
 // of connectivity.
 TEST_F(NightLightTest, AbsentValidGeoposition) {

chrome/browser/chromeos/night_light/night_light_client.cc

@@ -53,7 +53,7 @@ void NightLightClient::Start() {
 
 void NightLightClient::OnScheduleTypeChanged(
     ash::NightLightController::ScheduleType new_type) {
-  if (new_type != ash::NightLightController::ScheduleType::kSunsetToSunrise) {
+  if (new_type == ash::NightLightController::ScheduleType::kNone) {
     using_geoposition_ = false;
     timer_->Stop();
     return;
@@ -117,7 +117,7 @@ void NightLightClient::OnGeoposition(const chromeos::Geoposition& position,
                                      const base::TimeDelta elapsed) {
   if (!using_geoposition_) {
     // A response might arrive after the schedule type is no longer "sunset to
-    // sunrise", which means we should not push any positions to the
+    // sunrise" or "custom", which means we should not push any positions to the
     // NightLightController.
     return;
   }

chrome/browser/chromeos/night_light/night_light_client.h

@@ -35,7 +35,7 @@ class NightLightClient : public ash::NightLightController::Observer,
 
   // Starts watching changes in the Night Light schedule type in order to begin
   // periodically pushing user's IP-based geoposition to NightLightController as
-  // long as the type is set to "sunset to sunrise".
+  // long as the type is set to "sunset to sunrise" or "custom".
   void Start();
 
   // ash::NightLightController::Observer:
@@ -103,8 +103,9 @@ class NightLightClient : public ash::NightLightController::Observer,
   // The ID of the current timezone in the fromat similar to "America/Chicago".
   base::string16 current_timezone_id_;
 
-  // True as long as the schedule type is set to "sunset to sunrise", which
-  // means this client will be retrieving the IP-based geoposition once per day.
+  // True as long as the schedule type is set to "sunset to sunrise" or
+  // "custom", which means this client will be retrieving the IP-based
+  // geoposition once per day.
   bool using_geoposition_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(NightLightClient);

chrome/browser/chromeos/night_light/night_light_client_unittest.cc

@@ -111,7 +111,7 @@ class FakeNightLightClient : public NightLightClient,
 };
 
 // Base test fixture.
-class NightLightClientTest : public testing::Test {
+class NightLightClientTest : public testing::TestWithParam<ScheduleType> {
  public:
   NightLightClientTest() = default;
   ~NightLightClientTest() override = default;
@@ -134,12 +134,14 @@ class NightLightClientTest : public testing::Test {
 };
 
 // Test that the client is retrieving geoposition periodically only when the
-// schedule type is "sunset to sunrise".
-TEST_F(NightLightClientTest, TestClientRunningOnlyWhenSunsetToSunriseSchedule) {
+// schedule type is "sunset to sunrise" or "custom".
+TEST_F(NightLightClientTest,
+       TestClientRunningWhenSunsetToSunriseOrCustomSchedule) {
   EXPECT_FALSE(client_.using_geoposition());
   controller_.NotifyScheduleTypeChanged(ScheduleType::kNone);
   EXPECT_FALSE(client_.using_geoposition());
   controller_.NotifyScheduleTypeChanged(ScheduleType::kCustom);
+  EXPECT_TRUE(client_.using_geoposition());
   controller_.NotifyScheduleTypeChanged(ScheduleType::kSunsetToSunrise);
   task_environment_.RunUntilIdle();
   EXPECT_TRUE(client_.using_geoposition());
@@ -215,14 +217,14 @@ TEST_F(NightLightClientTest, TestRepeatedScheduleTypeChanges) {
   EXPECT_EQ(expected_delay, client_.timer().GetCurrentDelay());
 }
 
-// Tests that timezone changes result in new geoposition requests only if the
-// schedule type is sunset to sunrise.
-TEST_F(NightLightClientTest, TestTimezoneChanges) {
+// Tests that timezone changes result in new geoposition requests if the
+// schedule type is sunset to sunrise or custom.
+TEST_P(NightLightClientTest, TestTimezoneChanges) {
   EXPECT_EQ(0, controller_.position_pushes_num());
   client_.SetCurrentTimezoneIdForTesting(
       base::ASCIIToUTF16("America/Los_Angeles"));
 
-  // When schedule type is not sunset to sunrise, timezone changes do not result
+  // When schedule type is none, timezone changes do not result
   // in geoposition requests.
   controller_.NotifyScheduleTypeChanged(ScheduleType::kNone);
   task_environment_.RunUntilIdle();
@@ -243,9 +245,9 @@ TEST_F(NightLightClientTest, TestTimezoneChanges) {
   position.timestamp = base::Time::Now();
   client_.set_position_to_send(position);
 
-  // Change the schedule type to sunset to sunrise, and expect the geoposition
-  // will be pushed.
-  controller_.NotifyScheduleTypeChanged(ScheduleType::kSunsetToSunrise);
+  // Change the schedule type to sunset to sunrise or custom, and expect the
+  // geoposition will be pushed.
+  controller_.NotifyScheduleTypeChanged(GetParam());
   task_environment_.RunUntilIdle();
   EXPECT_EQ(1, controller_.position_pushes_num());
   EXPECT_EQ(1, client_.geoposition_requests_num());
@@ -267,4 +269,8 @@ TEST_F(NightLightClientTest, TestTimezoneChanges) {
   EXPECT_EQ(GetTimezoneId(*timezone), client_.current_timezone_id());
 }
 
+INSTANTIATE_TEST_SUITE_P(,
+                         NightLightClientTest,
+                         ::testing::Values(ScheduleType::kSunsetToSunrise,
+                                           ScheduleType::kCustom));
 }  // namespace