Make TimeOfDay API clearer.

This is a post-mortem AI. TimeOfDay::ToTimeToday() returned a "null"
base::Time instance on failure. This made it very easy for an
unsuspecting caller to used the return value without validating it.

It now returns an absl::optional<base::Time>, which essentially forces
the caller to check that the value is legitimate before using it.

Bug: b:294437057
Change-Id: I0b8b2dd22e4750502d2f48da33bab43379a76fd9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4791729
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Commit-Queue: Eric Sum <esum@google.com>
Cr-Commit-Position: refs/heads/main@{#1187045}

ash/BUILD.gn

@@ -4311,6 +4311,8 @@ static_library("test_support") {
     "test/test_widget_builder.h",
     "test/test_window_builder.cc",
     "test/test_window_builder.h",
+    "test/time_of_day_test_util.cc",
+    "test/time_of_day_test_util.h",
     "test/toplevel_window.cc",
     "test/toplevel_window.h",
     "test/ui_controls_ash.cc",

ash/system/geolocation/geolocation_controller.cc

@@ -295,11 +295,15 @@ base::Time GeolocationController::GetSunRiseSet(bool sunrise) const {
   if (!geoposition_) {
     VLOG(1) << "Invalid geoposition. Using default time for "
             << (sunrise ? "sunrise." : "sunset.");
-    return TimeOfDay(sunrise ? kDefaultSunriseTimeOffsetMinutes
-                             : kDefaultSunsetTimeOffsetMinutes)
-        .SetClock(clock_)
-        .SetLocalTimeConverter(local_time_converter_)
-        .ToTimeToday();
+    const absl::optional<base::Time> default_value =
+        TimeOfDay(sunrise ? kDefaultSunriseTimeOffsetMinutes
+                          : kDefaultSunsetTimeOffsetMinutes)
+            .SetClock(clock_)
+            .SetLocalTimeConverter(local_time_converter_)
+            .ToTimeToday();
+    // TODO(b/294437057): Change this method's return value to return a type
+    // that makes this failure more obvious to the caller.
+    return default_value.value_or(base::Time());
   }
 
   icu::CalendarAstronomer astro(geoposition_->longitude,
@@ -310,13 +314,18 @@ base::Time GeolocationController::GetSunRiseSet(bool sunrise) const {
   // See the documentation of icu::CalendarAstronomer::getSunRiseSet().
   // Note that the icu calendar works with milliseconds since epoch, and
   // base::Time::FromDoubleT() / ToDoubleT() work with seconds since epoch.
-  const double midday_today_sec =
+  const absl::optional<base::Time> midday_today =
       TimeOfDay(12 * 60)
           .SetClock(clock_)
           .SetLocalTimeConverter(local_time_converter_)
-          .ToTimeToday()
-          .ToDoubleT();
-  astro.setTime(midday_today_sec * 1000.0);
+          .ToTimeToday();
+  if (!midday_today) {
+    // TODO(b/294437057): Change this method's return value to return a type
+    // that makes this failure more obvious to the caller.
+    return base::Time();
+  }
+
+  astro.setTime(midday_today->ToDoubleT() * 1000.0);
   const double sun_rise_set_ms = astro.getSunRiseSet(sunrise);
   // If there is 24 hours of daylight or darkness, `CalendarAstronomer` returns
   // a very large negative value. Any timestamp before or at the epoch

ash/system/geolocation/geolocation_controller_unittest.cc

@@ -12,6 +12,7 @@
 #include "ash/system/geolocation/test_geolocation_url_loader_factory.h"
 #include "ash/system/time/time_of_day.h"
 #include "ash/test/ash_test_base.h"
+#include "ash/test/time_of_day_test_util.h"
 #include "ash/test_shell_delegate.h"
 #include "base/check.h"
 #include "base/memory/raw_ptr.h"
@@ -392,9 +393,9 @@ TEST_F(GeolocationControllerTest, SunsetSunriseDefault) {
   // If geoposition is unset, the controller should return the default sunset
   // and sunrise time .
   EXPECT_EQ(controller()->GetSunsetTime(),
-            TimeOfDay(kDefaultSunsetTimeOffsetMinutes).ToTimeToday());
+            ToTimeToday(TimeOfDay(kDefaultSunsetTimeOffsetMinutes)));
   EXPECT_EQ(controller()->GetSunriseTime(),
-            TimeOfDay(kDefaultSunriseTimeOffsetMinutes).ToTimeToday());
+            ToTimeToday(TimeOfDay(kDefaultSunriseTimeOffsetMinutes)));
 }
 
 // Tests the behavior when there is a valid geoposition, sunrise and sunset
@@ -531,14 +532,14 @@ TEST_F(GeolocationControllerTest, AbsentValidGeoposition) {
   // Switching to user 1 should ignore the current geoposition since it's
   // a cached value from user 2's prefs rather than a newly-updated value.
   SwitchActiveUser(kUser1Email);
-  EXPECT_EQ(controller()->GetSunsetTime(),
-            TimeOfDay(kDefaultSunsetTimeOffsetMinutes)
-                .SetClock(test_clock())
-                .ToTimeToday());
-  EXPECT_EQ(controller()->GetSunriseTime(),
-            TimeOfDay(kDefaultSunriseTimeOffsetMinutes)
-                .SetClock(test_clock())
-                .ToTimeToday());
+  EXPECT_EQ(
+      controller()->GetSunsetTime(),
+      ToTimeToday(
+          TimeOfDay(kDefaultSunsetTimeOffsetMinutes).SetClock(test_clock())));
+  EXPECT_EQ(
+      controller()->GetSunriseTime(),
+      ToTimeToday(
+          TimeOfDay(kDefaultSunriseTimeOffsetMinutes).SetClock(test_clock())));
 
   // Now simulate receiving a live geoposition update.
   Geoposition position;

ash/system/night_light/night_light_controller_impl.cc

@@ -138,8 +138,15 @@ class NightLightControllerDelegateImpl
     if (!HasGeoposition()) {
       LOG(ERROR) << "Invalid geoposition. Using default time for "
                  << (sunrise ? "sunrise." : "sunset.");
-      return sunrise ? TimeOfDay(kDefaultEndTimeOffsetMinutes).ToTimeToday()
-                     : TimeOfDay(kDefaultStartTimeOffsetMinutes).ToTimeToday();
+      return sunrise ? TimeOfDay(kDefaultEndTimeOffsetMinutes)
+                           .ToTimeToday()
+                           // TODO(b/289276024): `ToTimeToday()` failures will
+                           // be handled properly when night light has migrated
+                           // to use `GeolocationController`.
+                           .value_or(base::Time())
+                     : TimeOfDay(kDefaultStartTimeOffsetMinutes)
+                           .ToTimeToday()
+                           .value_or(base::Time());
     }
 
     icu::CalendarAstronomer astro(geoposition_->longitude,
@@ -151,7 +158,7 @@ class NightLightControllerDelegateImpl
     // Note that the icu calendar works with milliseconds since epoch, and
     // base::Time::FromDoubleT() / ToDoubleT() work with seconds since epoch.
     const double midday_today_sec =
-        TimeOfDay(12 * 60).ToTimeToday().ToDoubleT();
+        TimeOfDay(12 * 60).ToTimeToday().value_or(base::Time()).ToDoubleT();
     astro.setTime(midday_today_sec * 1000.0);
     const double sun_rise_set_ms = astro.getSunRiseSet(sunrise);
     return base::Time::FromDoubleT(sun_rise_set_ms / 1000.0);
@@ -1064,7 +1071,8 @@ void NightLightControllerImpl::Refresh(
 
     case ScheduleType::kCustom:
       RefreshScheduleTimer(
-          GetCustomStartTime().ToTimeToday(), GetCustomEndTime().ToTimeToday(),
+          GetCustomStartTime().ToTimeToday().value_or(base::Time()),
+          GetCustomEndTime().ToTimeToday().value_or(base::Time()),
           did_schedule_change, keep_manual_toggles_during_schedules);
       return;
   }

ash/system/night_light/night_light_controller_unittest.cc

@@ -20,6 +20,7 @@
 #include "ash/system/time/time_of_day.h"
 #include "ash/test/ash_test_base.h"
 #include "ash/test/ash_test_helper.h"
+#include "ash/test/time_of_day_test_util.h"
 #include "ash/test_shell_delegate.h"
 #include "base/command_line.h"
 #include "base/functional/bind.h"
@@ -154,9 +155,9 @@ class TestDelegate : public NightLightControllerImpl::Delegate {
   ~TestDelegate() override = default;
 
   void SetFakeNow(base::Time time) { fake_now_ = time; }
-  void SetFakeNow(TimeOfDay time) { fake_now_ = time.ToTimeToday(); }
-  void SetFakeSunset(TimeOfDay time) { fake_sunset_ = time.ToTimeToday(); }
-  void SetFakeSunrise(TimeOfDay time) { fake_sunrise_ = time.ToTimeToday(); }
+  void SetFakeNow(TimeOfDay time) { fake_now_ = ToTimeToday(time); }
+  void SetFakeSunset(TimeOfDay time) { fake_sunset_ = ToTimeToday(time); }
+  void SetFakeSunrise(TimeOfDay time) { fake_sunrise_ = ToTimeToday(time); }
 
   // ash::NightLightControllerImpl::Delegate
   base::Time GetNow() const override { return fake_now_; }
@@ -814,8 +815,8 @@ TEST_F(NightLightTest, AbsentValidGeoposition) {
   EXPECT_TRUE(controller->is_current_geoposition_from_cache());
   const TimeOfDay kSunset2{kFakePosition2_SunsetOffset};
   const TimeOfDay kSunrise2{kFakePosition2_SunriseOffset};
-  EXPECT_EQ(delegate()->GetSunsetTime(), kSunset2.ToTimeToday());
-  EXPECT_EQ(delegate()->GetSunriseTime(), kSunrise2.ToTimeToday());
+  EXPECT_EQ(delegate()->GetSunsetTime(), ToTimeToday(kSunset2));
+  EXPECT_EQ(delegate()->GetSunriseTime(), ToTimeToday(kSunrise2));
 
   // Store fake geoposition 1 in user 1's prefs.
   user1_pref_service()->SetDouble(prefs::kNightLightCachedLatitude,
@@ -831,16 +832,16 @@ TEST_F(NightLightTest, AbsentValidGeoposition) {
   EXPECT_TRUE(controller->is_current_geoposition_from_cache());
   const TimeOfDay kSunset1{kFakePosition1_SunsetOffset};
   const TimeOfDay kSunrise1{kFakePosition1_SunriseOffset};
-  EXPECT_EQ(delegate()->GetSunsetTime(), kSunset1.ToTimeToday());
-  EXPECT_EQ(delegate()->GetSunriseTime(), kSunrise1.ToTimeToday());
+  EXPECT_EQ(delegate()->GetSunsetTime(), ToTimeToday(kSunset1));
+  EXPECT_EQ(delegate()->GetSunriseTime(), ToTimeToday(kSunrise1));
 
   // Now simulate receiving a geoposition update of fake geoposition 2.
   controller->SetCurrentGeoposition(NightLightController::SimpleGeoposition{
       kFakePosition2_Latitude, kFakePosition2_Longitude});
   EXPECT_TRUE(delegate()->HasGeoposition());
   EXPECT_FALSE(controller->is_current_geoposition_from_cache());
-  EXPECT_EQ(delegate()->GetSunsetTime(), kSunset2.ToTimeToday());
-  EXPECT_EQ(delegate()->GetSunriseTime(), kSunrise2.ToTimeToday());
+  EXPECT_EQ(delegate()->GetSunsetTime(), ToTimeToday(kSunset2));
+  EXPECT_EQ(delegate()->GetSunriseTime(), ToTimeToday(kSunrise2));
 
   // Update user 2's prefs with fake geoposition 1.
   user2_pref_service()->SetDouble(prefs::kNightLightCachedLatitude,
@@ -853,8 +854,8 @@ TEST_F(NightLightTest, AbsentValidGeoposition) {
   SwitchActiveUser(kUser2Email);
   EXPECT_TRUE(delegate()->HasGeoposition());
   EXPECT_FALSE(controller->is_current_geoposition_from_cache());
-  EXPECT_EQ(delegate()->GetSunsetTime(), kSunset2.ToTimeToday());
-  EXPECT_EQ(delegate()->GetSunriseTime(), kSunrise2.ToTimeToday());
+  EXPECT_EQ(delegate()->GetSunsetTime(), ToTimeToday(kSunset2));
+  EXPECT_EQ(delegate()->GetSunriseTime(), ToTimeToday(kSunrise2));
 
   // Clear all cached geoposition prefs for all users, just to make sure getting
   // a new geoposition with persist it for all users not just the active one.
@@ -868,8 +869,8 @@ TEST_F(NightLightTest, AbsentValidGeoposition) {
       kFakePosition1_Latitude, kFakePosition1_Longitude});
   EXPECT_TRUE(delegate()->HasGeoposition());
   EXPECT_FALSE(controller->is_current_geoposition_from_cache());
-  EXPECT_EQ(delegate()->GetSunsetTime(), kSunset1.ToTimeToday());
-  EXPECT_EQ(delegate()->GetSunriseTime(), kSunrise1.ToTimeToday());
+  EXPECT_EQ(delegate()->GetSunsetTime(), ToTimeToday(kSunset1));
+  EXPECT_EQ(delegate()->GetSunriseTime(), ToTimeToday(kSunrise1));
   EXPECT_EQ(kFakePosition1_Latitude,
             user1_pref_service()->GetDouble(prefs::kNightLightCachedLatitude));
   EXPECT_EQ(kFakePosition1_Longitude,
@@ -1101,12 +1102,11 @@ TEST_F(NightLightTest, MultiUserManualStatusToggleWithSchedules) {
     bool user_1_expected_status;
     bool user_2_expected_status;
   } kTestCases[] = {
-      {MakeTimeOfDay(2, kPM).ToTimeToday(), false, false},
-      {MakeTimeOfDay(4, kPM).ToTimeToday(), true, false},
-      {MakeTimeOfDay(7, kPM).ToTimeToday(), true, true},
-      {MakeTimeOfDay(10, kPM).ToTimeToday(), false, true},
-      {MakeTimeOfDay(9, kAM).ToTimeToday() +
-           base::Days(1),  // 9:00 AM tomorrow.
+      {ToTimeToday(MakeTimeOfDay(2, kPM)), false, false},
+      {ToTimeToday(MakeTimeOfDay(4, kPM)), true, false},
+      {ToTimeToday(MakeTimeOfDay(7, kPM)), true, true},
+      {ToTimeToday(MakeTimeOfDay(10, kPM)), false, true},
+      {ToTimeToday(MakeTimeOfDay(9, kAM)) + base::Days(1),  // 9:00 AM tomorrow.
        false, false},
   };
 

ash/system/night_light/night_light_feature_pod_controller.cc

@@ -146,11 +146,16 @@ const std::u16string NightLightFeaturePodController::GetPodSubLabel() {
               ? IDS_ASH_STATUS_TRAY_NIGHT_LIGHT_ON_STATE_SUNSET_TO_SUNRISE_SCHEDULED
               : IDS_ASH_STATUS_TRAY_NIGHT_LIGHT_OFF_STATE_SUNSET_TO_SUNRISE_SCHEDULED);
     case NightLightController::ScheduleType::kCustom:
-      const TimeOfDay time = is_enabled ? controller->GetCustomEndTime()
+      const TimeOfDay time_of_day = is_enabled
+                                        ? controller->GetCustomEndTime()
                                         : controller->GetCustomStartTime();
+      const absl::optional<base::Time> time = time_of_day.ToTimeToday();
+      if (!time) {
+        return std::u16string();
+      }
       const std::u16string time_str =
           base::TimeFormatTimeOfDayWithHourClockType(
-              time.ToTimeToday(),
+              *time,
               Shell::Get()->system_tray_model()->clock()->hour_clock_type(),
               base::kKeepAmPm);
       return is_enabled

ash/system/night_light/night_light_feature_pod_controller_unittest.cc

@@ -16,6 +16,7 @@
 #include "ash/system/unified/unified_system_tray.h"
 #include "ash/system/unified/unified_system_tray_bubble.h"
 #include "ash/test/ash_test_base.h"
+#include "ash/test/time_of_day_test_util.h"
 #include "base/memory/raw_ptr.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
@@ -184,11 +185,11 @@ TEST_P(NightLightFeaturePodControllerTest, Custom) {
   auto* clock_model = Shell::Get()->system_tray_model()->clock();
   const std::u16string start_time_str =
       base::TimeFormatTimeOfDayWithHourClockType(
-          controller->GetCustomStartTime().ToTimeToday(),
+          ToTimeToday(controller->GetCustomStartTime()),
           clock_model->hour_clock_type(), base::kKeepAmPm);
   const std::u16string end_time_str =
       base::TimeFormatTimeOfDayWithHourClockType(
-          controller->GetCustomEndTime().ToTimeToday(),
+          ToTimeToday(controller->GetCustomEndTime()),
           clock_model->hour_clock_type(), base::kKeepAmPm);
   const std::u16string sublabel_on = l10n_util::GetStringFUTF16(
       IDS_ASH_STATUS_TRAY_NIGHT_LIGHT_ON_STATE_CUSTOM_SCHEDULED, end_time_str);

ash/system/scheduled_feature/scheduled_feature.cc

@@ -337,33 +337,55 @@ void ScheduledFeature::OnCustomSchedulePrefsChanged() {
 
 void ScheduledFeature::Refresh(bool did_schedule_change,
                                bool keep_manual_toggles_during_schedules) {
-  switch (GetScheduleType()) {
+  absl::optional<base::Time> start_time;
+  absl::optional<base::Time> end_time;
+  const ScheduleType schedule_type = GetScheduleType();
+  switch (schedule_type) {
     case ScheduleType::kNone:
       timer_->Stop();
       RefreshFeatureState();
       SetCurrentCheckpoint(
           GetCheckpointForEnabledState(GetEnabled(), ScheduleType::kNone));
       return;
     case ScheduleType::kSunsetToSunrise: {
-      base::Time sunrise_time = geolocation_controller_->GetSunriseTime();
-      base::Time sunset_time = geolocation_controller_->GetSunsetTime();
+      const base::Time sunrise_time = geolocation_controller_->GetSunriseTime();
+      const base::Time sunset_time = geolocation_controller_->GetSunsetTime();
       if (sunrise_time == GeolocationController::kNoSunRiseSet ||
           sunset_time == GeolocationController::kNoSunRiseSet) {
         // Simply disable the feature in this corner case. Since sunset and
         // sunrise are exactly the same, there is no time for it to be enabled.
-        sunrise_time = clock_->Now();
-        sunset_time = sunrise_time;
+        start_time = clock_->Now();
+        end_time = start_time;
+      } else if (!sunrise_time.is_null() && !sunset_time.is_null()) {
+        start_time = sunset_time;
+        end_time = sunrise_time;
+      } else {
+        // Sunrise or sunset is temporarily unavailable. Leave `start_time` and
+        // `end_time` unset.
       }
-      RefreshScheduleTimer(sunset_time, sunrise_time, did_schedule_change,
-                           keep_manual_toggles_during_schedules);
-      return;
+      break;
     }
     case ScheduleType::kCustom:
-      RefreshScheduleTimer(
-          GetCustomStartTime().ToTimeToday(), GetCustomEndTime().ToTimeToday(),
-          did_schedule_change, keep_manual_toggles_during_schedules);
-      return;
+      start_time = GetCustomStartTime().ToTimeToday();
+      end_time = GetCustomEndTime().ToTimeToday();
+      break;
   }
+
+  // b/285187343: Timestamps can legitimately be null if getting local time
+  // fails.
+  if (!start_time || !end_time) {
+    LOG(ERROR) << "Received null start/end times at " << clock_->Now();
+    ScheduleNextRefreshRetry(keep_manual_toggles_during_schedules);
+    // Best effort to still make `current_checkpoint_` as accurate as possible
+    // before exiting and not be in an inconsistent state. The next successful
+    // `Refresh()` will make `current_checkpoint_` 100% accurate again.
+    SetCurrentCheckpoint(
+        GetCheckpointForEnabledState(GetEnabled(), schedule_type));
+    return;
+  }
+
+  RefreshScheduleTimer(*start_time, *end_time, did_schedule_change,
+                       keep_manual_toggles_during_schedules);
 }
 
 // The `ScheduleCheckpoint` usage in this method does not directly apply
@@ -384,20 +406,6 @@ void ScheduledFeature::RefreshScheduleTimer(
   }
 
   const base::Time now = clock_->Now();
-  // b/285187343: Timestamps can legitimately be null if getting local time
-  // fails.
-  if (start_time.is_null() || end_time.is_null()) {
-    LOG(ERROR) << "Received null timestamps. start_time=" << start_time
-               << "end_time=" << end_time << " now=" << now;
-    ScheduleNextRefreshRetry(keep_manual_toggles_during_schedules);
-    // Best effort to still make `current_checkpoint_` as accurate as possible
-    // before exiting and not be in an inconsistent state. The next successful
-    // `Refresh()` will make `current_checkpoint_` 100% accurate again.
-    SetCurrentCheckpoint(
-        GetCheckpointForEnabledState(GetEnabled(), schedule_type));
-    return;
-  }
-
   const schedule_utils::Position schedule_position =
       schedule_utils::GetCurrentPosition(now, start_time, end_time,
                                          schedule_type);

ash/system/scheduled_feature/scheduled_feature_unittest.cc

@@ -27,6 +27,7 @@
 #include "ash/test/ash_test_base.h"
 #include "ash/test/ash_test_helper.h"
 #include "ash/test/failing_local_time_converter.h"
+#include "ash/test/time_of_day_test_util.h"
 #include "ash/test_shell_delegate.h"
 #include "base/command_line.h"
 #include "base/memory/ptr_util.h"
@@ -350,7 +351,7 @@ class ScheduledFeatureTest : public NoSessionAshTestBase,
   // 1) now = 2 PM time_of_day = 5 PM, advances 3 hours
   // 2) now = 7 PM time_of_day = 5 PM, advances 22 hours (the next day)
   void FastForwardTo(TimeOfDay time_of_day) {
-    base::Time target_time = time_of_day.SetClock(this).ToTimeToday();
+    base::Time target_time = ToTimeToday(time_of_day.SetClock(this));
     const base::Time now = Now();
     if (target_time < now) {
       target_time += base::Days(1);