Don't always remove Assistant notifications on click.

Previously, Assistant notifications would always be removed when clicked
or when clicking their buttons. Now, Assistant notifications may opt to
*not* be removed when clicked or when clicking their buttons.

This is used by Assistant timers v2, as Assistant timer notifications
should no longer be removed on some click events moving forward.

Bug: b:157930832
Change-Id: Ifb11186b89d7514258306d26bbd5f79e2c93f9c9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2225580Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Commit-Queue: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/master@{#774324}

ash/assistant/assistant_alarm_timer_controller_impl.cc

@@ -67,15 +67,18 @@ std::string ToFormattedTimeString(base::TimeDelta time,
 
   // Create our distinct |measures| to be formatted.
   std::vector<icu::Measure> measures;
-  if (hours) {
-    // We only show |hours| if necessary.
+
+  // We only show |hours| if necessary.
+  if (hours)
     measures.push_back(icu::Measure(hours, createHour(status), status));
-  }
-  if (minutes || width == UMEASFMT_WIDTH_NUMERIC) {
-    // We only show |minutes| if necessary or if using numeric format width.
+
+  // We only show |minutes| if necessary or if using numeric format |width|.
+  if (minutes || width == UMEASFMT_WIDTH_NUMERIC)
     measures.push_back(icu::Measure(minutes, createMinute(status), status));
-  }
-  measures.push_back(icu::Measure(seconds, createSecond(status), status));
+
+  // We only show |seconds| if necessary or if using numeric format |width|.
+  if (seconds || width == UMEASFMT_WIDTH_NUMERIC)
+    measures.push_back(icu::Measure(seconds, createSecond(status), status));
 
   // Format our |measures| into a |unicode_message|.
   icu::UnicodeString unicode_message;
@@ -172,7 +175,8 @@ std::vector<AssistantNotificationButtonPtr> CreateTimerNotificationButtons(
         l10n_util::GetStringUTF8(IDS_ASSISTANT_TIMER_NOTIFICATION_STOP_BUTTON),
         assistant::util::CreateAlarmTimerDeepLink(
             AlarmTimerAction::kRemoveAlarmOrTimer, timer.id)
-            .value()));
+            .value(),
+        /*remove_notification_on_click=*/true));
 
     // "ADD 1 MIN" button.
     buttons.push_back(AssistantNotificationButton::New(
@@ -181,7 +185,8 @@ std::vector<AssistantNotificationButtonPtr> CreateTimerNotificationButtons(
         assistant::util::CreateAlarmTimerDeepLink(
             AlarmTimerAction::kAddTimeToTimer, timer.id,
             base::TimeDelta::FromMinutes(1))
-            .value()));
+            .value(),
+        /*remove_notification_on_click=*/true));
 
     return buttons;
   }
@@ -196,7 +201,8 @@ std::vector<AssistantNotificationButtonPtr> CreateTimerNotificationButtons(
               IDS_ASSISTANT_TIMER_NOTIFICATION_RESUME_BUTTON),
           assistant::util::CreateAlarmTimerDeepLink(
               AlarmTimerAction::kResumeTimer, timer.id)
-              .value()));
+              .value(),
+          /*remove_notification_on_click=*/false));
     } else {
       // "PAUSE" button.
       buttons.push_back(AssistantNotificationButton::New(
@@ -204,7 +210,8 @@ std::vector<AssistantNotificationButtonPtr> CreateTimerNotificationButtons(
               IDS_ASSISTANT_TIMER_NOTIFICATION_PAUSE_BUTTON),
           assistant::util::CreateAlarmTimerDeepLink(
               AlarmTimerAction::kPauseTimer, timer.id)
-              .value()));
+              .value(),
+          /*remove_notification_on_click=*/false));
     }
   }
 
@@ -213,7 +220,8 @@ std::vector<AssistantNotificationButtonPtr> CreateTimerNotificationButtons(
       l10n_util::GetStringUTF8(IDS_ASSISTANT_TIMER_NOTIFICATION_CANCEL_BUTTON),
       assistant::util::CreateAlarmTimerDeepLink(
           AlarmTimerAction::kRemoveAlarmOrTimer, timer.id)
-          .value()));
+          .value(),
+      /*remove_notification_on_click=*/true));
 
   if (timer.state == AssistantTimerState::kFired) {
     // "ADD 1 MIN" button.
@@ -223,7 +231,8 @@ std::vector<AssistantNotificationButtonPtr> CreateTimerNotificationButtons(
         assistant::util::CreateAlarmTimerDeepLink(
             AlarmTimerAction::kAddTimeToTimer, timer.id,
             base::TimeDelta::FromMinutes(1))
-            .value()));
+            .value(),
+        /*remove_notification_on_click=*/false));
   }
 
   return buttons;

ash/assistant/assistant_alarm_timer_controller_unittest.cc

@@ -34,6 +34,9 @@ using chromeos::assistant::mojom::AssistantNotificationButton;
 using chromeos::assistant::mojom::AssistantNotificationButtonPtr;
 using chromeos::assistant::mojom::AssistantNotificationPtr;
 
+// Constants.
+constexpr char kTimerId[] = "1";
+
 // Test Structs ----------------------------------------------------------------
 
 // Represents a test instruction to advance the tick of the mock clock and
@@ -128,6 +131,13 @@ class ExpectButton {
     return *this;
   }
 
+  const ExpectButton& HasRemoveNotificationOnClick(
+      bool remove_notification_on_click) const {
+    EXPECT_EQ(remove_notification_on_click,
+              button_->remove_notification_on_click);
+    return *this;
+  }
+
  private:
   const AssistantNotificationButton* button_;
 };
@@ -435,8 +445,6 @@ TEST_F(AssistantAlarmTimerControllerTest, TimerNotificationHasExpectedButtons) {
   // Observe notifications.
   ScopedNotificationModelObserver notification_model_observer;
 
-  constexpr char kTimerId[] = "1";
-
   // Fire a timer.
   FireTimer(std::string(kTimerId));
 
@@ -450,7 +458,8 @@ TEST_F(AssistantAlarmTimerControllerTest, TimerNotificationHasExpectedButtons) {
       .HasActionUrl(
           assistant::util::CreateAlarmTimerDeepLink(
               assistant::util::AlarmTimerAction::kRemoveAlarmOrTimer, kTimerId)
-              .value());
+              .value())
+      .HasRemoveNotificationOnClick(true);
 
   // We expect an "ADD 1 MIN" button which will add time to the timer.
   ExpectButton(last_notification->buttons.at(1))
@@ -458,7 +467,8 @@ TEST_F(AssistantAlarmTimerControllerTest, TimerNotificationHasExpectedButtons) {
       .HasActionUrl(assistant::util::CreateAlarmTimerDeepLink(
                         assistant::util::AlarmTimerAction::kAddTimeToTimer,
                         kTimerId, base::TimeDelta::FromMinutes(1))
-                        .value());
+                        .value())
+      .HasRemoveNotificationOnClick(true);
 }
 
 // Tests that a notification is added for a timer and has the expected buttons
@@ -475,7 +485,6 @@ TEST_F(AssistantAlarmTimerControllerTest,
   // Observe notifications.
   ScopedNotificationModelObserver notification_model_observer;
 
-  constexpr char kTimerId[] = "1";
   constexpr base::TimeDelta kTimeRemaining = base::TimeDelta::FromMinutes(1);
 
   // Schedule a timer.
@@ -491,7 +500,8 @@ TEST_F(AssistantAlarmTimerControllerTest,
       .HasActionUrl(
           assistant::util::CreateAlarmTimerDeepLink(
               assistant::util::AlarmTimerAction::kPauseTimer, kTimerId)
-              .value());
+              .value())
+      .HasRemoveNotificationOnClick(false);
 
   // We expect a "CANCEL" button which will remove the timer.
   ExpectButton(last_notification->buttons.at(1))
@@ -499,7 +509,8 @@ TEST_F(AssistantAlarmTimerControllerTest,
       .HasActionUrl(
           assistant::util::CreateAlarmTimerDeepLink(
               assistant::util::AlarmTimerAction::kRemoveAlarmOrTimer, kTimerId)
-              .value());
+              .value())
+      .HasRemoveNotificationOnClick(true);
 
   // Pause the timer.
   PauseTimer(kTimerId).WithRemainingTime(kTimeRemaining);
@@ -514,7 +525,8 @@ TEST_F(AssistantAlarmTimerControllerTest,
       .HasActionUrl(
           assistant::util::CreateAlarmTimerDeepLink(
               assistant::util::AlarmTimerAction::kResumeTimer, kTimerId)
-              .value());
+              .value())
+      .HasRemoveNotificationOnClick(false);
 
   // We expect a "CANCEL" button which will remove the timer.
   ExpectButton(last_notification->buttons.at(1))
@@ -522,7 +534,8 @@ TEST_F(AssistantAlarmTimerControllerTest,
       .HasActionUrl(
           assistant::util::CreateAlarmTimerDeepLink(
               assistant::util::AlarmTimerAction::kRemoveAlarmOrTimer, kTimerId)
-              .value());
+              .value())
+      .HasRemoveNotificationOnClick(true);
 
   // Fire the timer.
   FireTimer(std::string(kTimerId));
@@ -537,7 +550,8 @@ TEST_F(AssistantAlarmTimerControllerTest,
       .HasActionUrl(
           assistant::util::CreateAlarmTimerDeepLink(
               assistant::util::AlarmTimerAction::kRemoveAlarmOrTimer, kTimerId)
-              .value());
+              .value())
+      .HasRemoveNotificationOnClick(true);
 
   // We expect an "ADD 1 MIN" button which will add time to the timer.
   ExpectButton(last_notification->buttons.at(1))
@@ -545,7 +559,25 @@ TEST_F(AssistantAlarmTimerControllerTest,
       .HasActionUrl(assistant::util::CreateAlarmTimerDeepLink(
                         assistant::util::AlarmTimerAction::kAddTimeToTimer,
                         kTimerId, base::TimeDelta::FromMinutes(1))
-                        .value());
+                        .value())
+      .HasRemoveNotificationOnClick(false);
+}
+
+// Tests that a notification is added for a timer and has the expected value to
+// cause removal on click.
+TEST_F(AssistantAlarmTimerControllerTest,
+       TimerNotificationHasExpectedRemoveOnClick) {
+  // Observe notifications.
+  ScopedNotificationModelObserver notification_model_observer;
+
+  // Fire a timer.
+  FireTimer(std::string(kTimerId));
+
+  // Make assertions about the notification.
+  auto* last_notification = notification_model_observer.last_notification();
+  ASSERT_NE(nullptr, last_notification);
+  EXPECT_EQ("assistant/timer1", last_notification->client_id);
+  EXPECT_EQ(true, last_notification->remove_on_click);
 }
 
 }  // namespace ash

ash/assistant/assistant_notification_controller.cc

@@ -182,7 +182,14 @@ void AssistantNotificationController::OnNotificationClicked(
     // NOTE: We copy construct a new GURL as our |notification| may be destroyed
     // during the OpenUrl() sequence leaving |action_url| in a bad state.
     AssistantController::Get()->OpenUrl(GURL(action_url));
-    model_.RemoveNotificationById(id, /*from_server=*/false);
+
+    const bool remove_notification =
+        button_index.has_value() ? notification->buttons[button_index.value()]
+                                       ->remove_notification_on_click
+                                 : notification->remove_on_click;
+
+    if (remove_notification)
+      model_.RemoveNotificationById(id, /*from_server=*/false);
     return;
   }
 

chromeos/services/assistant/assistant_manager_service_impl.cc

@@ -806,7 +806,8 @@ void AssistantManagerServiceImpl::OnShowNotification(
 
   for (const auto& button : notification.buttons) {
     notification_ptr->buttons.push_back(mojom::AssistantNotificationButton::New(
-        button.label, GURL(button.action_url)));
+        button.label, GURL(button.action_url),
+        /*remove_notification_on_click=*/true));
   }
 
   assistant_notification_controller()->AddOrUpdateNotification(

chromeos/services/assistant/public/mojom/assistant_notification.mojom

@@ -14,6 +14,9 @@ struct AssistantNotificationButton {
 
   // Optional URL to open when the tap action is invoked on the button.
   url.mojom.Url action_url;
+
+  // If |true|, the associated notification will be removed on button click.
+  bool remove_notification_on_click = true;
 };
 
 // Models an Assistant notification.
@@ -53,4 +56,7 @@ struct AssistantNotification {
   // When the notification should expire.
   // Expressed as milliseconds since Unix Epoch.
   mojo_base.mojom.Time? expiry_time;
+
+  // If |true|, the notification will be removed on click.
+  bool remove_on_click = true;
 };