Independently tick Assistant timers.

We will maintain a distinct "ticker" for each timer to workaround
synchronization issues that exist with ticking multiple timers in unison
due to differences in their respective fractional seconds remaining.

Bug: b:149570650
Change-Id: Ie5e4d1aa53b9ae2189514ad626b21168ae668a92
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2271521Reviewed-by: David Black <dmblack@google.com>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Reviewed-by: Jeroen Dhollander <jeroendh@google.com>
Commit-Queue: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/master@{#783780}

ash/assistant/assistant_alarm_timer_controller_impl.cc

@@ -4,8 +4,7 @@
 
 #include "ash/assistant/assistant_alarm_timer_controller_impl.h"
 
-#include <map>
-#include <string>
+#include <cmath>
 #include <utility>
 
 #include "ash/assistant/assistant_controller_impl.h"
@@ -14,6 +13,7 @@
 #include "ash/public/mojom/assistant_controller.mojom.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "base/i18n/message_formatter.h"
+#include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
@@ -43,9 +43,6 @@ using chromeos::assistant::mojom::AssistantNotificationPtr;
 constexpr char kTimerNotificationGroupingKey[] = "assistant/timer";
 constexpr char kTimerNotificationIdPrefix[] = "assistant/timer";
 
-// Interval at which alarms/timers are ticked.
-constexpr base::TimeDelta kTickInterval = base::TimeDelta::FromSeconds(1);
-
 // Helpers ---------------------------------------------------------------------
 
 std::string ToFormattedTimeString(base::TimeDelta time,
@@ -374,11 +371,8 @@ void AssistantAlarmTimerControllerImpl::OnAssistantStatusChanged(
 
 void AssistantAlarmTimerControllerImpl::OnTimerAdded(
     const AssistantTimer& timer) {
-  // Schedule a repeating timer to tick the tracked timers.
-  if (!ticker_.IsRunning()) {
-    ticker_.Start(FROM_HERE, kTickInterval, &model_,
-                  &AssistantAlarmTimerModel::Tick);
-  }
+  // Schedule the next tick of |timer|.
+  ScheduleNextTick(timer);
 
   // Create a notification for the added alarm/timer.
   assistant_controller_->notification_controller()->AddOrUpdateNotification(
@@ -387,6 +381,9 @@ void AssistantAlarmTimerControllerImpl::OnTimerAdded(
 
 void AssistantAlarmTimerControllerImpl::OnTimerUpdated(
     const AssistantTimer& timer) {
+  // Schedule the next tick of |timer|.
+  ScheduleNextTick(timer);
+
   // When a |timer| is updated we need to update the corresponding notification
   // unless it has already been dismissed by the user.
   auto* notification_controller =
@@ -400,9 +397,8 @@ void AssistantAlarmTimerControllerImpl::OnTimerUpdated(
 
 void AssistantAlarmTimerControllerImpl::OnTimerRemoved(
     const AssistantTimer& timer) {
-  // If our model is empty, we no longer need tick updates.
-  if (model_.empty())
-    ticker_.Stop();
+  // Clean up the ticker for |timer|, if one exists.
+  tickers_.erase(timer.id);
 
   // Remove any notification associated w/ |timer|.
   assistant_controller_->notification_controller()->RemoveNotificationById(
@@ -438,4 +434,45 @@ void AssistantAlarmTimerControllerImpl::PerformAlarmTimerAction(
   }
 }
 
+void AssistantAlarmTimerControllerImpl::ScheduleNextTick(
+    const AssistantTimer& timer) {
+  auto& ticker = tickers_[timer.id];
+  if (ticker.IsRunning())
+    return;
+
+  // The next tick of |timer| should occur at its next full second of remaining
+  // time. Here we are calculating the number of milliseconds to that next full
+  // second.
+  int millis_to_next_full_sec = timer.remaining_time.InMilliseconds() % 1000;
+
+  // If |timer| has already fired, |millis_to_next_full_sec| will be negative.
+  // In this case, we take the inverse of the value to get the correct number of
+  // milliseconds to the next full second of remaining time.
+  if (millis_to_next_full_sec < 0)
+    millis_to_next_full_sec = 1000 + millis_to_next_full_sec;
+
+  // NOTE: We pass a copy of |timer.id| here as |timer| may no longer exist
+  // when Tick() is called due to the possibility of the |model_| being updated
+  // via a call to OnTimerStateChanged(), such as might happen if a timer is
+  // created, paused, resumed, or removed by LibAssistant.
+  ticker.Start(FROM_HERE,
+               base::TimeDelta::FromMilliseconds(millis_to_next_full_sec),
+               base::BindOnce(&AssistantAlarmTimerControllerImpl::Tick,
+                              base::Unretained(this), timer.id));
+}
+
+void AssistantAlarmTimerControllerImpl::Tick(const std::string& timer_id) {
+  const auto* timer = model_.GetTimerById(timer_id);
+  DCHECK(timer);
+
+  // We don't tick paused timers. Once the |timer| resumes, ticking will resume.
+  if (timer->state == AssistantTimerState::kPaused)
+    return;
+
+  // Update |timer| to reflect the new amount of |remaining_time|.
+  AssistantTimerPtr updated_timer = std::make_unique<AssistantTimer>(*timer);
+  updated_timer->remaining_time = updated_timer->fire_time - base::Time::Now();
+  model_.AddOrUpdateTimer(std::move(updated_timer));
+}
+
 }  // namespace ash

ash/assistant/assistant_alarm_timer_controller_impl.h

@@ -72,11 +72,16 @@ class AssistantAlarmTimerControllerImpl
                                const std::string& alarm_timer_id,
                                const base::Optional<base::TimeDelta>& duration);
 
+  void ScheduleNextTick(const AssistantTimer& timer);
+  void Tick(const std::string& timer_id);
+
   AssistantControllerImpl* const assistant_controller_;  // Owned by Shell.
 
   AssistantAlarmTimerModel model_;
 
-  base::RepeatingTimer ticker_;
+  // We independently tick timers in our |model_| to update their respective
+  // remaining times. This map contains these tickers, keyed by timer id.
+  std::map<std::string, base::OneShotTimer> tickers_;
 
   // Owned by AssistantService.
   chromeos::assistant::Assistant* assistant_;

ash/assistant/assistant_alarm_timer_controller_unittest.cc

@@ -14,11 +14,11 @@
 #include "ash/assistant/model/assistant_alarm_timer_model_observer.h"
 #include "ash/assistant/model/assistant_notification_model.h"
 #include "ash/assistant/model/assistant_notification_model_observer.h"
+#include "ash/assistant/test/assistant_ash_test_base.h"
 #include "ash/assistant/util/deep_link_util.h"
 #include "ash/public/mojom/assistant_controller.mojom.h"
 #include "ash/shell.h"
 #include "ash/strings/grit/ash_strings.h"
-#include "ash/test/ash_test_base.h"
 #include "base/macros.h"
 #include "base/test/icu_test_util.h"
 #include "base/test/scoped_feature_list.h"
@@ -40,8 +40,8 @@ using chromeos::assistant::mojom::AssistantNotificationPriority;
 using chromeos::assistant::mojom::AssistantNotificationPtr;
 
 // Constants.
-constexpr char kTimerId[] = "1";
-constexpr char kClientId[] = "assistant/timer1";
+constexpr char kClientId[] = "assistant/timer<timer-id>";
+constexpr char kTimerId[] = "<timer-id>";
 
 // Mocks -----------------------------------------------------------------------
 
@@ -154,6 +154,8 @@ class ExpectedNotification {
       os << "\npriority: '" << notif.priority_.value() << "'";
     if (notif.remove_on_click_.has_value())
       os << "\nremove_on_click: '" << notif.remove_on_click_.value() << "'";
+    if (notif.title_.has_value())
+      os << "\ntitle: '" << notif.title_.value() << "'";
     return os;
   }
 
@@ -311,28 +313,45 @@ class ScopedNotificationModelObserver
 
 // AssistantAlarmTimerControllerTest -------------------------------------------
 
-class AssistantAlarmTimerControllerTest : public AshTestBase {
+class AssistantAlarmTimerControllerTest : public AssistantAshTestBase {
  protected:
   AssistantAlarmTimerControllerTest()
-      : AshTestBase(base::test::TaskEnvironment::TimeSource::MOCK_TIME) {}
+      : AssistantAshTestBase(
+            base::test::TaskEnvironment::TimeSource::MOCK_TIME) {}
 
   ~AssistantAlarmTimerControllerTest() override = default;
 
   // AshTestBase:
   void SetUp() override {
-    AshTestBase::SetUp();
-
+    AssistantAshTestBase::SetUp();
     feature_list_.InitAndDisableFeature(
         chromeos::assistant::features::kAssistantTimersV2);
   }
 
-  // Advances the clock by |time_delta|, running any sequenced tasks in the
-  // queue. Note that we don't use |TaskEnvironment::FastForwardBy| because that
-  // API will hang when |time_delta| is sufficiently large, ultimately resulting
-  // in unittest timeout.
-  void AdvanceClock(base::TimeDelta time_delta) {
+  void TearDown() override {
+    controller()->OnTimerStateChanged({});
+    AssistantAshTestBase::TearDown();
+  }
+
+  // Advances the clock by |time_delta| and waits for the next timer update.
+  // NOTE: If |time_delta| is zero, this method is a no-op.
+  void AdvanceClockAndWaitForTimerUpdate(base::TimeDelta time_delta) {
+    if (time_delta.is_zero())
+      return;
+
     task_environment()->AdvanceClock(time_delta);
-    task_environment()->RunUntilIdle();
+
+    MockAssistantAlarmTimerModelObserver observer;
+    controller()->GetModel()->AddObserver(&observer);
+
+    base::RunLoop run_loop;
+    EXPECT_CALL(observer, OnTimerUpdated)
+        .WillOnce(testing::Invoke([&](const AssistantTimer& timer) {
+          run_loop.QuitClosure().Run();
+        }));
+    run_loop.Run();
+
+    controller()->GetModel()->RemoveObserver(&observer);
   }
 
   AssistantAlarmTimerController* controller() {
@@ -391,7 +410,7 @@ TEST_F(AssistantAlarmTimerControllerTest, UpdatedTimersShouldHaveCreationTime) {
   ScheduleTimer(kTimerId).WithCreationTime(creation_time);
 
   // Advance clock.
-  AdvanceClock(base::TimeDelta::FromMinutes(1));
+  AdvanceClockAndWaitForTimerUpdate(base::TimeDelta::FromMinutes(1));
 
   // If unspecified, |creation_time| should carry forward on update.
   EXPECT_CALL(mock, OnTimerUpdated)
@@ -494,7 +513,7 @@ TEST_F(AssistantAlarmTimerControllerTest, TimerNotificationHasExpectedTitleV2) {
     // Run each tick of the clock in the test.
     for (auto& tick : i18n_test_case.ticks) {
       // Advance clock to next tick.
-      AdvanceClock(tick.advance_clock);
+      AdvanceClockAndWaitForTimerUpdate(tick.advance_clock);
 
       // Make assertions about the notification.
       EXPECT_EQ(ExpectedNotification().WithClientId(kClientId).WithTitle(
@@ -550,7 +569,7 @@ TEST_F(AssistantAlarmTimerControllerTest, TimerNotificationHasExpectedMessage) {
     // Run each tick of the clock in the test.
     for (auto& tick : i18n_test_case.ticks) {
       // Advance clock to next tick.
-      AdvanceClock(tick.advance_clock);
+      AdvanceClockAndWaitForTimerUpdate(tick.advance_clock);
 
       // Make assertions about the notification.
       EXPECT_EQ(ExpectedNotification().WithClientId(kClientId).WithMessage(
@@ -909,7 +928,7 @@ TEST_F(AssistantAlarmTimerControllerTest,
 
   // Advance the clock.
   // NOTE: Six seconds is the threshold for popping up our notification.
-  AdvanceClock(base::TimeDelta::FromSeconds(6));
+  AdvanceClockAndWaitForTimerUpdate(base::TimeDelta::FromSeconds(6));
 
   // Make assertions about the notification.
   EXPECT_EQ(ExpectedNotification().WithClientId(kClientId).WithPriority(

ash/assistant/model/assistant_alarm_timer_model.cc

@@ -76,20 +76,6 @@ const AssistantTimer* AssistantAlarmTimerModel::GetTimerById(
   return it != timers_.end() ? it->second.get() : nullptr;
 }
 
-void AssistantAlarmTimerModel::Tick() {
-  if (timers_.empty())
-    return;
-
-  for (auto& pair : timers_) {
-    AssistantTimer* timer = pair.second.get();
-    if (timer->state == AssistantTimerState::kPaused)
-      continue;
-
-    timer->remaining_time = timer->fire_time - base::Time::Now();
-    NotifyTimerUpdated(*timer);
-  }
-}
-
 void AssistantAlarmTimerModel::NotifyTimerAdded(const AssistantTimer& timer) {
   for (auto& observer : observers_)
     observer.OnTimerAdded(timer);

ash/assistant/model/assistant_alarm_timer_model.h

@@ -44,10 +44,6 @@ class COMPONENT_EXPORT(ASSISTANT_MODEL) AssistantAlarmTimerModel {
   // Returns the timer uniquely identified by |id|.
   const AssistantTimer* GetTimerById(const std::string& id) const;
 
-  // Invoke to tick any timers. Note that this will update the |remaining_time|
-  // for all timers in the model and trigger an OnTimerUpdated() event.
-  void Tick();
-
   // Returns |true| if the model contains no timers, |false| otherwise.
   bool empty() const { return timers_.empty(); }
 

ash/public/cpp/assistant/controller/assistant_alarm_timer_controller.cc

@@ -17,6 +17,8 @@ AssistantAlarmTimerController* g_instance = nullptr;
 // AssistantTimer --------------------------------------------------------------
 
 AssistantTimer::AssistantTimer() = default;
+AssistantTimer::AssistantTimer(const AssistantTimer&) = default;
+AssistantTimer& AssistantTimer::operator=(const AssistantTimer&) = default;
 AssistantTimer::~AssistantTimer() = default;
 
 // AssistantAlarmTimerController -----------------------------------------------

ash/public/cpp/assistant/controller/assistant_alarm_timer_controller.h

@@ -36,8 +36,8 @@ enum class AssistantTimerState {
 // Models an Assistant timer.
 struct ASH_PUBLIC_EXPORT AssistantTimer {
   AssistantTimer();
-  AssistantTimer(const AssistantTimer&) = delete;
-  AssistantTimer& operator=(const AssistantTimer&) = delete;
+  AssistantTimer(const AssistantTimer&);
+  AssistantTimer& operator=(const AssistantTimer&);
   ~AssistantTimer();
 
   std::string id;