STOP button in timer notification should remove timer.

Previously, the STOP button would cause the timer to stop ringing. This
was sufficient because we only showed a notification for ringing timers.
Since we will now show notifications for ticking timers, the STOP button
needs to actually remove the associated timer.

This CL:
- Removes the StopRinging API as it is no longer needed, and there is no
  need for it anytime soon (to my knowledge).
- Adds a RemoveAlarmTimer API.
- Updates DeepLinkUtil to reflect changes in possible AlarmTimerActions.
- Updates unittests and creates a new browsertest verifying changes.

Note that this CL adds assistant_timers_browsertest as
assistant_browsertest is growing large. A follow up CL will migrate
existing timer browsertests to the new location.

Also note that this CL removes support for in-Assistant notification
types since it was the sole user of the StopRinging API and there is no
plan to launch the feature following standalone UI deprecation. The
pipeline to support notifications of multiple types has *not* been
removed at this time as it may prove useful later.

Bug: b:149570650
Change-Id: I4cbf0da246237f8402b075ace0f0e1c98348e296
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2132622
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#758981}

ash/assistant/assistant_alarm_timer_controller.cc

@@ -31,6 +31,8 @@ namespace ash {
 
 namespace {
 
+using assistant::util::AlarmTimerAction;
+
 // Grouping key and ID prefix for timer notifications.
 constexpr char kTimerNotificationGroupingKey[] = "assistant/timer";
 constexpr char kTimerNotificationIdPrefix[] = "assistant/timer";
@@ -116,26 +118,15 @@ chromeos::assistant::mojom::AssistantNotificationPtr CreateTimerNotification(
   const std::string message = CreateTimerNotificationMessage(timer);
 
   base::Optional<GURL> stop_alarm_timer_action_url =
-      assistant::util::CreateAlarmTimerDeepLink(
-          assistant::util::AlarmTimerAction::kStopRinging,
-          /*alarm_timer_id=*/base::nullopt,
-          /*duration=*/base::nullopt);
+      assistant::util::CreateAlarmTimerDeepLink(AlarmTimerAction::kRemove,
+                                                timer.id);
 
   base::Optional<GURL> add_time_to_timer_action_url =
       assistant::util::CreateAlarmTimerDeepLink(
-          assistant::util::AlarmTimerAction::kAddTimeToTimer, timer.id,
-          kOneMin);
+          AlarmTimerAction::kAddTimeToTimer, timer.id, kOneMin);
 
   AssistantNotificationPtr notification = AssistantNotification::New();
-
-  // If in-Assistant notifications are supported, we'll allow alarm/timer
-  // notifications to show in either Assistant UI or the Message Center.
-  // Otherwise, we'll only allow the notification to show in the Message Center.
-  notification->type =
-      chromeos::assistant::features::IsInAssistantNotificationsEnabled()
-          ? AssistantNotificationType::kPreferInAssistant
-          : AssistantNotificationType::kSystem;
-
+  notification->type = AssistantNotificationType::kSystem;
   notification->title = title;
   notification->message = message;
   notification->client_id = CreateTimerNotificationId(timer);
@@ -209,11 +200,9 @@ void AssistantAlarmTimerController::SetAssistant(
 
 void AssistantAlarmTimerController::OnAssistantControllerConstructed() {
   assistant_controller_->state_controller()->AddObserver(this);
-  assistant_controller_->ui_controller()->AddModelObserver(this);
 }
 
 void AssistantAlarmTimerController::OnAssistantControllerDestroying() {
-  assistant_controller_->ui_controller()->RemoveModelObserver(this);
   assistant_controller_->state_controller()->RemoveObserver(this);
 }
 
@@ -226,21 +215,22 @@ void AssistantAlarmTimerController::OnDeepLinkReceived(
   if (type != DeepLinkType::kAlarmTimer)
     return;
 
-  const base::Optional<assistant::util::AlarmTimerAction>& action =
+  const base::Optional<AlarmTimerAction>& action =
       assistant::util::GetDeepLinkParamAsAlarmTimerAction(params);
   if (!action.has_value())
     return;
 
-  // Timer ID is optional. Only used for adding time to timer.
   const base::Optional<std::string>& alarm_timer_id =
       assistant::util::GetDeepLinkParam(params, DeepLinkParam::kId);
+  if (!alarm_timer_id.has_value())
+    return;
 
   // Duration is optional. Only used for adding time to timer.
   const base::Optional<base::TimeDelta>& duration =
       assistant::util::GetDeepLinkParamAsTimeDelta(params,
                                                    DeepLinkParam::kDurationMs);
 
-  PerformAlarmTimerAction(action.value(), alarm_timer_id, duration);
+  PerformAlarmTimerAction(action.value(), alarm_timer_id.value(), duration);
 }
 
 void AssistantAlarmTimerController::OnAssistantStatusChanged(
@@ -320,43 +310,22 @@ void AssistantAlarmTimerController::OnAllTimersRemoved() {
                                         /*from_server=*/false);
 }
 
-void AssistantAlarmTimerController::OnUiVisibilityChanged(
-    AssistantVisibility new_visibility,
-    AssistantVisibility old_visibility,
-    base::Optional<AssistantEntryPoint> entry_point,
-    base::Optional<AssistantExitPoint> exit_point) {
-  // When the Assistant UI transitions from a visible state, we'll dismiss any
-  // ringing alarms or timers (assuming certain conditions have been met).
-  if (old_visibility != AssistantVisibility::kVisible)
-    return;
-
-  // We only do this if in-Assistant notifications are enabled, as in-Assistant
-  // alarm/timer notifications only live as long the Assistant UI. Per UX
-  // requirement, when Assistant UI dismisses with an in-Assistant timer
-  // notification showing, any ringing alarms/timers should be stopped.
-  if (!chromeos::assistant::features::IsInAssistantNotificationsEnabled())
-    return;
-
-  assistant_->StopAlarmTimerRinging();
-}
-
 void AssistantAlarmTimerController::PerformAlarmTimerAction(
-    const assistant::util::AlarmTimerAction& action,
-    const base::Optional<std::string>& alarm_timer_id,
+    const AlarmTimerAction& action,
+    const std::string& alarm_timer_id,
     const base::Optional<base::TimeDelta>& duration) {
   DCHECK(assistant_);
 
   switch (action) {
-    case assistant::util::AlarmTimerAction::kAddTimeToTimer:
-      if (!alarm_timer_id.has_value() || !duration.has_value()) {
-        LOG(ERROR) << "Ignore add time to timer action without timer ID or "
-                   << "duration.";
+    case AlarmTimerAction::kAddTimeToTimer:
+      if (!duration.has_value()) {
+        LOG(ERROR) << "Ignoring add time to timer action duration.";
         return;
       }
-      assistant_->AddTimeToTimer(alarm_timer_id.value(), duration.value());
+      assistant_->AddTimeToTimer(alarm_timer_id, duration.value());
       break;
-    case assistant::util::AlarmTimerAction::kStopRinging:
-      assistant_->StopAlarmTimerRinging();
+    case AlarmTimerAction::kRemove:
+      assistant_->RemoveAlarmTimer(alarm_timer_id);
       break;
   }
 }

ash/assistant/assistant_alarm_timer_controller.h

@@ -12,7 +12,6 @@
 #include "ash/assistant/assistant_controller_observer.h"
 #include "ash/assistant/model/assistant_alarm_timer_model.h"
 #include "ash/assistant/model/assistant_alarm_timer_model_observer.h"
-#include "ash/assistant/model/assistant_ui_model_observer.h"
 #include "ash/public/cpp/assistant/assistant_state.h"
 #include "ash/public/mojom/assistant_controller.mojom.h"
 #include "base/macros.h"
@@ -37,8 +36,7 @@ class AssistantAlarmTimerController
     : public mojom::AssistantAlarmTimerController,
       public AssistantControllerObserver,
       public AssistantStateObserver,
-      public AssistantAlarmTimerModelObserver,
-      public AssistantUiModelObserver {
+      public AssistantAlarmTimerModelObserver {
  public:
   explicit AssistantAlarmTimerController(
       AssistantController* assistant_controller);
@@ -77,18 +75,10 @@ class AssistantAlarmTimerController
   void OnTimerRemoved(const mojom::AssistantTimer& timer) override;
   void OnAllTimersRemoved() override;
 
-  // AssistantUiModelObserver:
-  void OnUiVisibilityChanged(
-      AssistantVisibility new_visibility,
-      AssistantVisibility old_visibility,
-      base::Optional<AssistantEntryPoint> entry_point,
-      base::Optional<AssistantExitPoint> exit_point) override;
-
  private:
-  void PerformAlarmTimerAction(
-      const assistant::util::AlarmTimerAction& action,
-      const base::Optional<std::string>& alarm_timer_id,
-      const base::Optional<base::TimeDelta>& duration);
+  void PerformAlarmTimerAction(const assistant::util::AlarmTimerAction& action,
+                               const std::string& alarm_timer_id,
+                               const base::Optional<base::TimeDelta>& duration);
 
   AssistantController* const assistant_controller_;  // Owned by Shell.
 

ash/assistant/assistant_notification_controller.cc

@@ -65,9 +65,8 @@ message_center::NotifierId GetNotifierId() {
 
 bool IsSystemNotification(
     const chromeos::assistant::mojom::AssistantNotification* notification) {
-  using chromeos::assistant::mojom::AssistantNotificationType;
-  return notification->type == AssistantNotificationType::kPreferInAssistant ||
-         notification->type == AssistantNotificationType::kSystem;
+  return notification->type ==
+         chromeos::assistant::mojom::AssistantNotificationType::kSystem;
 }
 
 bool IsValidActionUrl(const GURL& action_url) {
@@ -85,13 +84,11 @@ AssistantNotificationController::AssistantNotificationController(
       expiry_monitor_(this),
       notifier_id_(GetNotifierId()) {
   AddModelObserver(this);
-  assistant_controller_->AddObserver(this);
   message_center::MessageCenter::Get()->AddObserver(this);
 }
 
 AssistantNotificationController::~AssistantNotificationController() {
   message_center::MessageCenter::Get()->RemoveObserver(this);
-  assistant_controller_->RemoveObserver(this);
   RemoveModelObserver(this);
 }
 
@@ -115,68 +112,10 @@ void AssistantNotificationController::SetAssistant(
   assistant_ = assistant;
 }
 
-// AssistantControllerObserver -------------------------------------------------
-
-void AssistantNotificationController::OnAssistantControllerConstructed() {
-  assistant_controller_->ui_controller()->AddModelObserver(this);
-}
-
-void AssistantNotificationController::OnAssistantControllerDestroying() {
-  assistant_controller_->ui_controller()->RemoveModelObserver(this);
-}
-
-// AssistantUiModelObserver ----------------------------------------------------
-
-void AssistantNotificationController::OnUiVisibilityChanged(
-    AssistantVisibility new_visibility,
-    AssistantVisibility old_visibility,
-    base::Optional<AssistantEntryPoint> entry_point,
-    base::Optional<AssistantExitPoint> exit_point) {
-  switch (new_visibility) {
-    case AssistantVisibility::kVisible:
-      // When the Assistant UI becomes visible we convert any notifications of
-      // type |kPreferInAssistant| to type |kInAssistant|. This will cause them
-      // to be removed from the Message Center (if they had previously been
-      // added) and to finish out their lifetimes as in-Assistant notifications.
-      for (const auto* notification : model_.GetNotificationsByType(
-               AssistantNotificationType::kPreferInAssistant)) {
-        auto update = notification->Clone();
-        update->type = AssistantNotificationType::kInAssistant;
-        model_.AddOrUpdateNotification(std::move(update));
-      }
-      break;
-    case AssistantVisibility::kClosed:
-      // When the Assistant UI is no longer visible to the user we remove any
-      // notifications of type |kInAssistant| as this type of notification does
-      // not outlive the Assistant view hierarchy.
-      if (old_visibility == AssistantVisibility::kVisible) {
-        for (const auto* notification : model_.GetNotificationsByType(
-                 AssistantNotificationType::kInAssistant)) {
-          model_.RemoveNotificationById(notification->client_id,
-                                        /*from_server=*/false);
-        }
-      }
-      break;
-  }
-}
-
 // mojom::AssistantNotificationController --------------------------------------
 
 void AssistantNotificationController::AddOrUpdateNotification(
     AssistantNotificationPtr notification) {
-  const AssistantVisibility visibility =
-      assistant_controller_->ui_controller()->model()->visibility();
-
-  // If Assistant UI is visible and |notification| is of type
-  // |kPreferInAssistant|, we convert it to a notification of type
-  // |kInAssistant|. This will cause the notification to be removed from the
-  // Message Center (if it had previously been added) and it will finish out its
-  // lifetime as an in-Assistant notification.
-  if (visibility == AssistantVisibility::kVisible &&
-      notification->type == AssistantNotificationType::kPreferInAssistant) {
-    notification->type = AssistantNotificationType::kInAssistant;
-  }
-
   model_.AddOrUpdateNotification(std::move(notification));
 }
 

ash/assistant/assistant_notification_controller.h

@@ -8,11 +8,9 @@
 #include <string>
 
 #include "ash/ash_export.h"
-#include "ash/assistant/assistant_controller_observer.h"
 #include "ash/assistant/assistant_notification_expiry_monitor.h"
 #include "ash/assistant/model/assistant_notification_model.h"
 #include "ash/assistant/model/assistant_notification_model_observer.h"
-#include "ash/assistant/model/assistant_ui_model_observer.h"
 #include "ash/public/mojom/assistant_controller.mojom.h"
 #include "base/macros.h"
 #include "chromeos/services/assistant/public/mojom/assistant.mojom.h"
@@ -28,8 +26,6 @@ class AssistantController;
 // The class to manage Assistant notifications.
 class ASH_EXPORT AssistantNotificationController
     : public mojom::AssistantNotificationController,
-      public AssistantControllerObserver,
-      public AssistantUiModelObserver,
       public AssistantNotificationModelObserver,
       public message_center::MessageCenterObserver {
  public:
@@ -57,17 +53,6 @@ class ASH_EXPORT AssistantNotificationController
   // Provides a pointer to the |assistant| owned by AssistantController.
   void SetAssistant(chromeos::assistant::mojom::Assistant* assistant);
 
-  // AssistantControllerObserver:
-  void OnAssistantControllerConstructed() override;
-  void OnAssistantControllerDestroying() override;
-
-  // AssistantUiModelObserver:
-  void OnUiVisibilityChanged(
-      AssistantVisibility new_visibility,
-      AssistantVisibility old_visibility,
-      base::Optional<AssistantEntryPoint> entry_point,
-      base::Optional<AssistantExitPoint> exit_point) override;
-
   // mojom::AssistantNotificationController:
   void AddOrUpdateNotification(AssistantNotificationPtr notification) override;
   void RemoveNotificationById(const std::string& id, bool from_server) override;

ash/assistant/test/test_assistant_service.cc

@@ -283,8 +283,7 @@ void TestAssistantService::AddTimeToTimer(const std::string& id,
                                           base::TimeDelta duration) {
 }
 
-void TestAssistantService::StopAlarmTimerRinging() {
-}
+void TestAssistantService::RemoveAlarmTimer(const std::string& id) {}
 
 void TestAssistantService::StartInteraction(
     chromeos::assistant::mojom::AssistantInteractionType type,

ash/assistant/test/test_assistant_service.h

@@ -112,7 +112,7 @@ class TestAssistantService : public chromeos::assistant::mojom::Assistant {
   void NotifyEntryIntoAssistantUi(
       chromeos::assistant::mojom::AssistantEntryPoint entry_point) override;
   void AddTimeToTimer(const std::string& id, base::TimeDelta duration) override;
-  void StopAlarmTimerRinging() override;
+  void RemoveAlarmTimer(const std::string& id) override;
 
  private:
   void StartInteraction(

ash/assistant/util/deep_link_util.cc

@@ -40,7 +40,7 @@ constexpr char kVeIdParamKey[] = "veId";
 
 // Supported alarm/timer action deep link param values.
 constexpr char kAddTimeToTimer[] = "addTimeToTimer";
-constexpr char kStopAlarmTimerRinging[] = "stopAlarmTimerRinging";
+constexpr char kRemoveAlarmTimer[] = "removeAlarmTimer";
 
 // Supported proactive suggestions action deep link param values.
 constexpr char kCardClick[] = "cardClick";
@@ -89,12 +89,12 @@ base::Optional<GURL> CreateAlarmTimerDeepLink(
       url = net::AppendOrReplaceQueryParameter(url, kActionParamKey,
                                                kAddTimeToTimer);
       break;
-    case assistant::util::AlarmTimerAction::kStopRinging:
-      DCHECK(!alarm_timer_id.has_value() && !duration.has_value());
-      if (alarm_timer_id.has_value() || duration.has_value())
+    case assistant::util::AlarmTimerAction::kRemove:
+      DCHECK(alarm_timer_id.has_value() && !duration.has_value());
+      if (!alarm_timer_id.has_value() || duration.has_value())
         return base::nullopt;
       url = net::AppendOrReplaceQueryParameter(url, kActionParamKey,
-                                               kStopAlarmTimerRinging);
+                                               kRemoveAlarmTimer);
       break;
   }
 
@@ -108,6 +108,7 @@ base::Optional<GURL> CreateAlarmTimerDeepLink(
         url, kDurationMsParamKey,
         base::NumberToString(duration->InMilliseconds()));
   }
+
   return url;
 }
 
@@ -184,8 +185,8 @@ base::Optional<AlarmTimerAction> GetDeepLinkParamAsAlarmTimerAction(
   if (action_string_value.value() == kAddTimeToTimer)
     return AlarmTimerAction::kAddTimeToTimer;
 
-  if (action_string_value.value() == kStopAlarmTimerRinging)
-    return AlarmTimerAction::kStopRinging;
+  if (action_string_value.value() == kRemoveAlarmTimer)
+    return AlarmTimerAction::kRemove;
 
   return base::nullopt;
 }

ash/assistant/util/deep_link_util.h

@@ -58,7 +58,7 @@ enum class DeepLinkParam {
 // Enumeration of alarm/timer deep link actions.
 enum class AlarmTimerAction {
   kAddTimeToTimer,
-  kStopRinging,
+  kRemove,
 };
 
 // Enumeration of proactive suggestions deep link actions.
@@ -80,7 +80,7 @@ COMPONENT_EXPORT(ASSISTANT_UTIL)
 base::Optional<GURL> CreateAlarmTimerDeepLink(
     AlarmTimerAction action,
     base::Optional<std::string> alarm_timer_id,
-    base::Optional<base::TimeDelta> duration);
+    base::Optional<base::TimeDelta> duration = base::nullopt);
 
 // Returns a deep link to send an Assistant query.
 COMPONENT_EXPORT(ASSISTANT_UTIL)

ash/assistant/util/deep_link_util_unittest.cc

@@ -27,10 +27,10 @@ TEST_F(DeepLinkUtilTest, CreateAlarmTimerDeeplink) {
       CreateAlarmTimerDeepLink(AlarmTimerAction::kAddTimeToTimer, "1",
                                base::TimeDelta::FromMinutes(1))
           .value());
-  ASSERT_EQ("googleassistant://alarm-timer?action=stopAlarmTimerRinging",
-            CreateAlarmTimerDeepLink(AlarmTimerAction::kStopRinging,
-                                     base::nullopt, base::nullopt)
-                .value());
+  ASSERT_EQ(
+      "googleassistant://alarm-timer?action=removeAlarmTimer&id=1",
+      CreateAlarmTimerDeepLink(AlarmTimerAction::kRemove, "1", base::nullopt)
+          .value());
 
   // For invalid deeplink params, we will hit DCHECK since this API isn't meant
   // to be used in such cases. We'll use a |ScopedLogAssertHandler| to safely
@@ -40,14 +40,15 @@ TEST_F(DeepLinkUtilTest, CreateAlarmTimerDeeplink) {
          const base::StringPiece stack_trace) {}));
 
   ASSERT_EQ(base::nullopt,
-            CreateAlarmTimerDeepLink(AlarmTimerAction::kStopRinging, "1",
+            CreateAlarmTimerDeepLink(AlarmTimerAction::kRemove, base::nullopt,
                                      base::nullopt));
-  ASSERT_EQ(base::nullopt, CreateAlarmTimerDeepLink(
-                               AlarmTimerAction::kStopRinging, base::nullopt,
-                               base::TimeDelta::FromMinutes(1)));
   ASSERT_EQ(base::nullopt,
-            CreateAlarmTimerDeepLink(AlarmTimerAction::kStopRinging, "1",
+            CreateAlarmTimerDeepLink(AlarmTimerAction::kRemove, base::nullopt,
                                      base::TimeDelta::FromMinutes(1)));
+  ASSERT_EQ(base::nullopt,
+            CreateAlarmTimerDeepLink(AlarmTimerAction::kRemove, "1",
+                                     base::TimeDelta::FromMinutes(1)));
+
   ASSERT_EQ(base::nullopt,
             CreateAlarmTimerDeepLink(AlarmTimerAction::kAddTimeToTimer, "1",
                                      base::nullopt));
@@ -175,8 +176,8 @@ TEST_F(DeepLinkUtilTest, GetDeepLinkParamAsAlarmTimerAction) {
   // Case: Deep link parameter present, well formed.
   params["action"] = "addTimeToTimer";
   AssertDeepLinkParamEq(AlarmTimerAction::kAddTimeToTimer);
-  params["action"] = "stopAlarmTimerRinging";
-  AssertDeepLinkParamEq(AlarmTimerAction::kStopRinging);
+  params["action"] = "removeAlarmTimer";
+  AssertDeepLinkParamEq(AlarmTimerAction::kRemove);
 
   // Case: Deep link parameter present, non AlarmTimerAction value.
   params["action"] = "true";

chrome/browser/ui/ash/DEPS

@@ -26,6 +26,12 @@ specific_include_rules = {
   "ash_util\.cc": [
     "+ash/shell.h",
   ],
+  "assistant_timers_browsertest\.cc": [
+    "+ash/shelf",
+    "+ash/shell.h",
+    "+ash/system",
+    "+ui/message_center",
+  ],
   # https://crbug.com/756054
   "chrome_accessibility_delegate.*": [
     "+ash/accessibility/accessibility_delegate.h",

chrome/browser/ui/ash/assistant/assistant_timers_browsertest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <string>
+#include <vector>
+
+#include "ash/shelf/shelf.h"
+#include "ash/shelf/shelf_widget.h"
+#include "ash/shell.h"
+#include "ash/system/message_center/unified_message_center_bubble.h"
+#include "ash/system/message_center/unified_message_center_view.h"
+#include "ash/system/status_area_widget.h"
+#include "ash/system/unified/unified_system_tray.h"
+#include "base/strings/string_util.h"
+#include "base/test/icu_test_util.h"
+#include "chrome/browser/ui/ash/assistant/assistant_test_mixin.h"
+#include "chrome/test/base/mixin_based_in_process_browser_test.h"
+#include "chromeos/services/assistant/public/features.h"
+#include "ui/aura/window.h"
+#include "ui/events/test/event_generator.h"
+#include "ui/message_center/message_center.h"
+#include "ui/message_center/public/cpp/notification.h"
+#include "ui/message_center/views/notification_view_md.h"
+
+namespace chromeos {
+namespace assistant {
+
+namespace {
+
+// TODO(b:153496343): Move generic helpers to a more generic location for reuse.
+// Helpers ---------------------------------------------------------------------
+
+// Finds any descendents of |parent| with the desired |class_name| and pushes
+// them onto the strongly typed |result| vector.
+// NOTE: Callers are expected to ensure that casting to <T> makes sense.
+template <typename T>
+void FindDescendentsOfClass(views::View* parent,
+                            std::string class_name,
+                            std::vector<T*>* result) {
+  for (auto* child : parent->children()) {
+    if (child->GetClassName() == class_name)
+      result->push_back(static_cast<T*>(child));
+    FindDescendentsOfClass(child, class_name, result);
+  }
+}
+
+// Finds any descendents of |parent| with class name equal to the static class
+// variable |kViewClassName| and pushes them onto the strongly typed |result|
+// vector.
+// NOTE: This variant of FindDescendentsOfClass is safer than the three argument
+// variant and its usage should be preferred where possible.
+template <typename T>
+void FindDescendentsOfClass(views::View* parent, std::vector<T*>* result) {
+  FindDescendentsOfClass(parent, T::kViewClassName, result);
+}
+
+// Returns the status area widget.
+ash::StatusAreaWidget* FindStatusAreaWidget() {
+  return ash::Shelf::ForWindow(ash::Shell::GetRootWindowForNewWindows())
+      ->shelf_widget()
+      ->status_area_widget();
+}
+
+// Returns the set of Assistant notifications (as indicated by application id).
+message_center::NotificationList::Notifications FindAssistantNotifications() {
+  return message_center::MessageCenter::Get()->FindNotificationsByAppId(
+      "assistant");
+}
+
+// Returns visible notifications having id starting with |prefix|.
+std::vector<message_center::Notification*> FindVisibleNotificationsByPrefixedId(
+    const std::string& prefix) {
+  std::vector<message_center::Notification*> notifications;
+  for (auto* notification :
+       message_center::MessageCenter::Get()->GetVisibleNotifications()) {
+    if (base::StartsWith(notification->id(), prefix,
+                         base::CompareCase::SENSITIVE)) {
+      notifications.push_back(notification);
+    }
+  }
+  return notifications;
+}
+
+// Returns the view for the specified |notification|.
+message_center::MessageView* FindViewForNotification(
+    const message_center::Notification* notification) {
+  ash::UnifiedMessageCenterView* unified_message_center_view =
+      FindStatusAreaWidget()
+          ->unified_system_tray()
+          ->message_center_bubble()
+          ->message_center_view();
+
+  std::vector<message_center::MessageView*> message_views;
+  FindDescendentsOfClass(unified_message_center_view, &message_views);
+
+  for (message_center::MessageView* message_view : message_views) {
+    if (message_view->notification_id() == notification->id())
+      return message_view;
+  }
+
+  return nullptr;
+}
+
+// Returns the action buttons for the specified |notification|.
+std::vector<message_center::NotificationButtonMD*>
+FindActionButtonsForNotification(
+    const message_center::Notification* notification) {
+  auto* notification_view = FindViewForNotification(notification);
+
+  std::vector<message_center::NotificationButtonMD*> action_buttons;
+  FindDescendentsOfClass(notification_view, "NotificationButtonMD",
+                         &action_buttons);
+
+  return action_buttons;
+}
+
+// Performs a tap of the specified |view| and waits until the RunLoop idles.
+void TapOnAndWait(const views::View* view) {
+  auto* root_window = view->GetWidget()->GetNativeWindow()->GetRootWindow();
+  ui::test::EventGenerator event_generator(root_window);
+  event_generator.MoveTouch(view->GetBoundsInScreen().CenterPoint());
+  event_generator.PressTouch();
+  event_generator.ReleaseTouch();
+  base::RunLoop().RunUntilIdle();
+}
+
+// Performs a tap of the specified |widget| and waits until the RunLoop idles.
+void TapOnAndWait(const views::Widget* widget) {
+  aura::Window* root_window = widget->GetNativeWindow()->GetRootWindow();
+  ui::test::EventGenerator event_generator(root_window);
+  event_generator.MoveTouch(widget->GetWindowBoundsInScreen().CenterPoint());
+  event_generator.PressTouch();
+  event_generator.ReleaseTouch();
+  base::RunLoop().RunUntilIdle();
+}
+
+}  // namespace
+
+// AssistantTimersBrowserTest --------------------------------------------------
+
+class AssistantTimersBrowserTest : public MixinBasedInProcessBrowserTest {
+ public:
+  AssistantTimersBrowserTest() {
+    feature_list_.InitAndEnableFeature(features::kAssistantTimersV2);
+  }
+
+  AssistantTimersBrowserTest(const AssistantTimersBrowserTest&) = delete;
+  AssistantTimersBrowserTest& operator=(const AssistantTimersBrowserTest&) =
+      delete;
+
+  ~AssistantTimersBrowserTest() override = default;
+
+  void ShowAssistantUi() {
+    if (!tester()->IsVisible())
+      tester()->PressAssistantKey();
+  }
+
+  AssistantTestMixin* tester() { return &tester_; }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+  base::test::ScopedRestoreICUDefaultLocale locale_{"en_US"};
+  AssistantTestMixin tester_{&mixin_host_, this, embedded_test_server(),
+                             FakeS3Mode::kReplay};
+};
+
+// Tests -----------------------------------------------------------------------
+
+// Pressing the "STOP" action button in a timer notification should result in
+// the timer being removed.
+IN_PROC_BROWSER_TEST_F(AssistantTimersBrowserTest,
+                       ShouldRemoveTimerWhenStoppingViaNotification) {
+  tester()->StartAssistantAndWaitForReady();
+
+  ShowAssistantUi();
+  EXPECT_TRUE(tester()->IsVisible());
+
+  // Confirm no Assistant notifications are currently being shown.
+  EXPECT_TRUE(FindAssistantNotifications().empty());
+
+  // Start a timer for five minutes.
+  tester()->SendTextQuery("Set a timer for 5 minutes");
+  tester()->ExpectAnyOfTheseTextResponses({
+      "Alright, 5 min. Starting… now.",
+      "OK, 5 min. And we're starting… now.",
+      "OK, 5 min. Starting… now.",
+      "Sure, 5 min. And that's starting… now.",
+      "Sure, 5 min. Starting now.",
+  });
+
+  // Tap status area widget (to show notifications in the Message Center).
+  TapOnAndWait(FindStatusAreaWidget());
+
+  // Confirm that an Assistant timer notification is now showing.
+  auto notifications = FindVisibleNotificationsByPrefixedId("assistant/timer");
+  ASSERT_EQ(1u, notifications.size());
+
+  // Find the action buttons for our notification.
+  // NOTE: We expect action buttons for "STOP" and "ADD 1 MIN".
+  auto action_buttons = FindActionButtonsForNotification(notifications.at(0));
+  EXPECT_EQ(2u, action_buttons.size());
+
+  // Tap the "STOP" action button in the notification.
+  EXPECT_EQ(base::UTF8ToUTF16("STOP"), action_buttons.at(0)->GetText());
+  TapOnAndWait(action_buttons.at(0));
+
+  ShowAssistantUi();
+  EXPECT_TRUE(tester()->IsVisible());
+
+  // Confirm that no timers exist anymore.
+  tester()->SendTextQuery("Show my timers");
+  tester()->ExpectAnyOfTheseTextResponses({
+      "It looks like you don't have any timers set at the moment.",
+  });
+}
+
+}  // namespace assistant
+}  // namespace chromeos

chrome/test/BUILD.gn

@@ -2478,6 +2478,7 @@ if (!is_android) {
           "../browser/ui/ash/assistant/assistant_browsertest.cc",
           "../browser/ui/ash/assistant/assistant_test_mixin.cc",
           "../browser/ui/ash/assistant/assistant_test_mixin.h",
+          "../browser/ui/ash/assistant/assistant_timers_browsertest.cc",
           "../browser/ui/ash/assistant/test/fake_s3_server.cc",
           "../browser/ui/ash/assistant/test/fake_s3_server.h",
         ]

chromeos/services/assistant/assistant_manager_service_impl.cc

@@ -1464,13 +1464,6 @@ void AssistantManagerServiceImpl::OnDeviceAppsEnabled(bool enabled) {
       assistant::features::IsAppSupportEnabled() && enabled);
 }
 
-void AssistantManagerServiceImpl::StopAlarmTimerRinging() {
-  if (!assistant_manager_internal_)
-    return;
-
-  assistant_manager_internal_->GetAlarmTimerManager()->StopRinging();
-}
-
 void AssistantManagerServiceImpl::AddTimeToTimer(const std::string& id,
                                                  base::TimeDelta duration) {
   if (!assistant_manager_internal_)
@@ -1480,6 +1473,11 @@ void AssistantManagerServiceImpl::AddTimeToTimer(const std::string& id,
       id, duration.InSeconds());
 }
 
+void AssistantManagerServiceImpl::RemoveAlarmTimer(const std::string& id) {
+  if (assistant_manager_internal_)
+    assistant_manager_internal_->GetAlarmTimerManager()->RemoveEvent(id);
+}
+
 void AssistantManagerServiceImpl::NotifyEntryIntoAssistantUi(
     mojom::AssistantEntryPoint entry_point) {
   base::AutoLock lock(last_trigger_source_lock_);

chromeos/services/assistant/assistant_manager_service_impl.h

@@ -86,6 +86,9 @@ enum class AssistantQueryResponseType {
 // Since LibAssistant is a standalone library, all callbacks come from it
 // running on threads not owned by Chrome. Thus we need to post the callbacks
 // onto the main thread.
+// NOTE: this class may start/stop LibAssistant multiple times throughout its
+// lifetime. This may occur, for example, if the user manually toggles Assistant
+// enabled/disabled in settings or switches to a non-primary profile.
 class COMPONENT_EXPORT(ASSISTANT_SERVICE) AssistantManagerServiceImpl
     : public AssistantManagerService,
       public ::chromeos::assistant::action::AssistantActionObserver,
@@ -154,7 +157,7 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AssistantManagerServiceImpl
   void NotifyEntryIntoAssistantUi(
       mojom::AssistantEntryPoint entry_point) override;
   void AddTimeToTimer(const std::string& id, base::TimeDelta duration) override;
-  void StopAlarmTimerRinging() override;
+  void RemoveAlarmTimer(const std::string& id) override;
 
   // AssistantActionObserver overrides:
   void OnScheduleWait(int id, int time_ms) override;

chromeos/services/assistant/fake_assistant_manager_service_impl.cc

@@ -117,7 +117,7 @@ void FakeAssistantManagerServiceImpl::AddTimeToTimer(const std::string& id,
                                                      base::TimeDelta duration) {
 }
 
-void FakeAssistantManagerServiceImpl::StopAlarmTimerRinging() {}
+void FakeAssistantManagerServiceImpl::RemoveAlarmTimer(const std::string& id) {}
 
 void FakeAssistantManagerServiceImpl::SetStateAndInformObservers(
     State new_state) {

chromeos/services/assistant/fake_assistant_manager_service_impl.h

@@ -75,7 +75,7 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) FakeAssistantManagerServiceImpl
   void NotifyEntryIntoAssistantUi(
       mojom::AssistantEntryPoint entry_point) override;
   void AddTimeToTimer(const std::string& id, base::TimeDelta duration) override;
-  void StopAlarmTimerRinging() override;
+  void RemoveAlarmTimer(const std::string& id) override;
 
   // Update the state to the corresponding value, and inform the
   // |AssistantStateObserver| of the change.

chromeos/services/assistant/public/features.cc

@@ -82,9 +82,6 @@ const base::Feature kAssistantRoutines{"AssistantRoutines",
 const base::Feature kAssistantTimersV2{"AssistantTimersV2",
                                        base::FEATURE_DISABLED_BY_DEFAULT};
 
-const base::Feature kInAssistantNotifications{
-    "InAssistantNotifications", base::FEATURE_DISABLED_BY_DEFAULT};
-
 const base::Feature kEnableClearCutLog{"EnableClearCutLog",
                                        base::FEATURE_DISABLED_BY_DEFAULT};
 
@@ -151,10 +148,6 @@ bool IsFeedbackUiEnabled() {
   return base::FeatureList::IsEnabled(kAssistantFeedbackUi);
 }
 
-bool IsInAssistantNotificationsEnabled() {
-  return base::FeatureList::IsEnabled(kInAssistantNotifications);
-}
-
 bool IsMediaSessionIntegrationEnabled() {
   return base::FeatureList::IsEnabled(kEnableMediaSessionIntegration);
 }

chromeos/services/assistant/public/features.h

@@ -62,10 +62,6 @@ extern const base::Feature kAssistantRoutines;
 COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC)
 extern const base::Feature kAssistantTimersV2;
 
-// Enables in-Assistant notifications.
-COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC)
-extern const base::Feature kInAssistantNotifications;
-
 // Enables clear cut logging.
 COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC)
 extern const base::Feature kEnableClearCutLog;
@@ -114,9 +110,6 @@ COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC) bool IsDspHotwordEnabled();
 
 COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC) bool IsFeedbackUiEnabled();
 
-COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC)
-bool IsInAssistantNotificationsEnabled();
-
 COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC)
 bool IsMediaSessionIntegrationEnabled();
 

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

@@ -142,8 +142,9 @@ interface Assistant {
   // this method is a no-op if there is no existing timer identified by |id|.
   AddTimeToTimer(string id, mojo_base.mojom.TimeDelta duration);
 
-  // Stops timer or alarm ringing.
-  StopAlarmTimerRinging();
+  // Remove the alarm/timer specified by |id|.
+  // NOTE: this is a no-op if no such alarm/timer exists.
+  RemoveAlarmTimer(string id);
 };
 
 // Enumeration of Assistant entry points. These values are persisted to logs.

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

@@ -7,25 +7,9 @@ module chromeos.assistant.mojom;
 import "mojo/public/mojom/base/time.mojom";
 import "url/mojom/url.mojom";
 
+// TODO(b:153495778): Remove notification types.
 // Enumeration of notification types.
 enum AssistantNotificationType {
-  // A notification of type |kInAssistant| will only be displayed within
-  // Assistant UI. If Assistant UI is not visible at the time of notification
-  // creation, the notification will be queued up until Assistant UI becomes
-  // visible. When Assistant UI dismisses, notifications of this type are
-  // dismissed.
-  kInAssistant,
-
-  // A notification of type |kPreferInAssistant| may be shown in either
-  // Assistant UI or the Message Center, depending on Assistant visibility
-  // state. If Assistant UI is not visible, notifications of this type will
-  // show immediately in the Message Center unless the user has opted out of
-  // seeing system notifications. Once Assistant UI becomes visible, or if it
-  // is already visible at the time of notification creation, notifications of
-  // this type are converted to notifications of type |kInAssistant| and they
-  // will finish out their lifetimes as in-Assistant notifications.
-  kPreferInAssistant,
-
   // A notification of type |kSystem| will only be displayed within the Message
   // Center. It is immediately added to the Message Center regardless of
   // Assistant UI visibility state, although it may not be added if the user has