[Privacy Hub] Add Privacy Hub button to combined notification

Remove the behavior of clicking the notification text to open Privacy
Hub as only the combined notification should have the interaction to
open Privacy Hub. This interaction is supposed to be obvious to the user
through the newly added button.

Bug: b:262832229
Change-Id: I0d8f7125625f64df0332b54f6762c6d6cc1aa173
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4270640
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Auto-Submit: Christoph Schlosser <cschlosser@chromium.org>
Commit-Queue: Christoph Schlosser <cschlosser@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1108825}

ash/ash_strings.grd

@@ -5018,6 +5018,9 @@ Here are some things you can try to get started.
       <message name="IDS_PRIVACY_HUB_MICROPHONE_AND_CAMERA_OFF_NOTIFICATION_BUTTON" desc="Label for an action button that enables microphone and camera.">
         Turn on access
       </message>
+      <message name="IDS_PRIVACY_HUB_OPEN_SETTINGS_PAGE_BUTTON" desc="Label for an action button that opens the Privacy Hub settings page.">
+        Privacy controls
+      </message>
 
       <!-- Strings for camera privacy hub switch notifications -->
       <message name="IDS_PRIVACY_HUB_WANT_TO_TURN_OFF_CAMERA_NOTIFICATION_TITLE" desc="Title for a notification shown to the users when they disable camera via the hardware switch.">

ash/ash_strings_grd/IDS_PRIVACY_HUB_OPEN_SETTINGS_PAGE_BUTTON.png.sha1

@@ -0,0 +1 @@
+0015cb1b94d4edf0d0482d84f89112e719ce98a3

ash/system/privacy_hub/camera_privacy_switch_controller_unittest.cc

@@ -9,7 +9,6 @@
 
 #include "ash/constants/ash_pref_names.h"
 #include "ash/public/cpp/sensor_disabled_notification_delegate.h"
-#include "ash/public/cpp/test/test_system_tray_client.h"
 #include "ash/session/session_controller_impl.h"
 #include "ash/shell.h"
 #include "ash/strings/grit/ash_strings.h"
@@ -403,24 +402,13 @@ TEST_F(PrivacyHubCameraControllerTests,
   EXPECT_TRUE(FindNotificationById(kPrivacyHubCameraOffNotificationId));
   EXPECT_FALSE(GetUserPref());
 
-  EXPECT_EQ(GetSystemTrayClient()->show_os_settings_privacy_hub_count(), 0);
-  EXPECT_EQ(histogram_tester_.GetBucketCount(
-                privacy_hub_metrics::kPrivacyHubOpenedHistogram,
-                privacy_hub_metrics::PrivacyHubNavigationOrigin::kNotification),
-            0);
-
   // Enabling camera via clicking on the body should open the privacy hub
   // settings page.
   message_center->ClickOnNotification(kPrivacyHubCameraOffNotificationId);
 
-  EXPECT_EQ(GetSystemTrayClient()->show_os_settings_privacy_hub_count(), 1);
   // The user pref should not be changed.
   EXPECT_FALSE(GetUserPref());
   EXPECT_FALSE(FindNotificationById(kPrivacyHubCameraOffNotificationId));
-  EXPECT_EQ(histogram_tester_.GetBucketCount(
-                privacy_hub_metrics::kPrivacyHubOpenedHistogram,
-                privacy_hub_metrics::PrivacyHubNavigationOrigin::kNotification),
-            1);
 
   SetUserPref(true);
 
@@ -439,25 +427,14 @@ TEST_F(PrivacyHubCameraControllerTests,
       kPrivacyHubHWCameraSwitchOffSWCameraSwitchOnNotificationId));
   EXPECT_TRUE(GetUserPref());
 
-  EXPECT_EQ(GetSystemTrayClient()->show_os_settings_privacy_hub_count(), 1);
-  EXPECT_EQ(histogram_tester_.GetBucketCount(
-                privacy_hub_metrics::kPrivacyHubOpenedHistogram,
-                privacy_hub_metrics::PrivacyHubNavigationOrigin::kNotification),
-            1);
-
   // Clicking on the body should open the privacy hub settings page.
   message_center->ClickOnNotification(
       kPrivacyHubHWCameraSwitchOffSWCameraSwitchOnNotificationId);
 
-  EXPECT_EQ(GetSystemTrayClient()->show_os_settings_privacy_hub_count(), 2);
   // The user pref should not be changed.
   EXPECT_TRUE(GetUserPref());
   EXPECT_FALSE(FindNotificationById(
       kPrivacyHubHWCameraSwitchOffSWCameraSwitchOnNotificationId));
-  EXPECT_EQ(histogram_tester_.GetBucketCount(
-                privacy_hub_metrics::kPrivacyHubOpenedHistogram,
-                privacy_hub_metrics::PrivacyHubNavigationOrigin::kNotification),
-            2);
 }
 
 TEST_F(PrivacyHubCameraControllerTests,

ash/system/privacy_hub/microphone_privacy_switch_controller_unittest.cc

@@ -13,7 +13,6 @@
 #include "ash/public/cpp/privacy_hub_delegate.h"
 #include "ash/public/cpp/sensor_disabled_notification_delegate.h"
 #include "ash/public/cpp/test/test_new_window_delegate.h"
-#include "ash/public/cpp/test/test_system_tray_client.h"
 #include "ash/session/session_controller_impl.h"
 #include "ash/shell.h"
 #include "ash/strings/grit/ash_strings.h"
@@ -357,11 +356,6 @@ TEST_F(PrivacyHubMicrophoneControllerTest, SwMuteNotificationActionButton) {
   ASSERT_TRUE(notification);
   EXPECT_EQ(1u, notification->buttons().size());
 
-  EXPECT_EQ(histogram_tester().GetBucketCount(
-                privacy_hub_metrics::
-                    kPrivacyHubMicrophoneEnabledFromNotificationHistogram,
-                true),
-            0);
   // Clicking the action button should unmute device.
   ClickOnNotificationButton();
   EXPECT_FALSE(CrasAudioHandler::Get()->IsInputMuted());
@@ -383,21 +377,10 @@ TEST_F(PrivacyHubMicrophoneControllerTest, SwMuteNotificationActionBody) {
   ASSERT_TRUE(notification);
   EXPECT_EQ(1u, notification->buttons().size());
 
-  EXPECT_EQ(histogram_tester().GetBucketCount(
-                privacy_hub_metrics::kPrivacyHubOpenedHistogram,
-                privacy_hub_metrics::PrivacyHubNavigationOrigin::kNotification),
-            0);
-
   // Clicking the action button should unmute device.
   ClickOnNotificationBody();
-  EXPECT_EQ(GetSystemTrayClient()->show_os_settings_privacy_hub_count(), 1);
   EXPECT_TRUE(CrasAudioHandler::Get()->IsInputMuted());
 
-  EXPECT_EQ(histogram_tester().GetBucketCount(
-                privacy_hub_metrics::kPrivacyHubOpenedHistogram,
-                privacy_hub_metrics::PrivacyHubNavigationOrigin::kNotification),
-            1);
-
   EXPECT_FALSE(GetNotification());
 }
 

ash/system/privacy_hub/privacy_hub_notification.cc

@@ -4,7 +4,6 @@
 
 #include "ash/system/privacy_hub/privacy_hub_notification.h"
 
-#include "ash/system/privacy_hub/privacy_hub_notification_controller.h"
 #include "base/containers/contains.h"
 #include "components/vector_icons/vector_icons.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
@@ -24,22 +23,24 @@ void RemoveNotification(const std::string& id) {
 namespace ash {
 
 PrivacyHubNotificationClickDelegate::PrivacyHubNotificationClickDelegate(
-    base::RepeatingClosure button_click)
-    : button_callback_(std::move(button_click)) {}
+    base::RepeatingClosure button_click) {
+  button_callbacks_[0] = std::move(button_click);
+}
+
 PrivacyHubNotificationClickDelegate::~PrivacyHubNotificationClickDelegate() =
     default;
 
 void PrivacyHubNotificationClickDelegate::Click(
-    const absl::optional<int>& button_index,
+    const absl::optional<int>& button_index_opt,
     const absl::optional<std::u16string>& reply) {
-  if (button_index.has_value()) {
-    button_callback_.Run();
+  if (button_index_opt.has_value()) {
+    const unsigned int button_index = button_index_opt.value();
+    CHECK_GT(button_callbacks_.size(), button_index);
+    DCHECK(!button_callbacks_[button_index].is_null())
+        << "button_index=" << button_index;
+    RunCallbackIfNotNull(button_callbacks_[button_index]);
   } else {
-    if (!message_callback_.is_null()) {
-      message_callback_.Run();
-    }
-
-    PrivacyHubNotificationController::OpenPrivacyHubSettingsPage();
+    RunCallbackIfNotNull(message_callback_);
   }
 }
 
@@ -48,6 +49,18 @@ void PrivacyHubNotificationClickDelegate::SetMessageClickCallback(
   message_callback_ = std::move(callback);
 }
 
+void PrivacyHubNotificationClickDelegate::SetSecondButtonCallback(
+    base::RepeatingClosure callback) {
+  button_callbacks_[1] = std::move(callback);
+}
+
+void PrivacyHubNotificationClickDelegate::RunCallbackIfNotNull(
+    const base::RepeatingClosure& callback) {
+  if (!callback.is_null()) {
+    callback.Run();
+  }
+}
+
 PrivacyHubNotification::PrivacyHubNotification(
     const std::string& id,
     const int title_id,
@@ -56,21 +69,21 @@ PrivacyHubNotification::PrivacyHubNotification(
     const scoped_refptr<PrivacyHubNotificationClickDelegate> delegate,
     const ash::NotificationCatalogName catalog_name,
     const int button_id)
-    : id_(id), message_ids_(message_ids), sensors_for_apps_(sensors_for_apps) {
+    : id_(id),
+      message_ids_(message_ids),
+      sensors_for_apps_(sensors_for_apps),
+      delegate_(delegate),
+      button_text_(l10n_util::GetStringUTF16(button_id)) {
   DCHECK(!message_ids_.empty());
   DCHECK(message_ids_.size() < 2u || !sensors_for_apps_.Empty())
       << "Specify at least one sensor when providing more than one message ID";
   DCHECK(delegate);
 
-  message_center::RichNotificationData optional_fields;
-  optional_fields.remove_on_click = true;
-  optional_fields.buttons.emplace_back(l10n_util::GetStringUTF16(button_id));
-
   builder_.SetId(id)
       .SetCatalogName(catalog_name)
       .SetDelegate(std::move(delegate))
       .SetTitleId(title_id)
-      .SetOptionalFields(optional_fields)
+      .SetOptionalFields(MakeOptionalFields())
       .SetSmallImage(vector_icons::kSettingsIcon)
       .SetWarningLevel(message_center::SystemNotificationWarningLevel::NORMAL);
 }
@@ -123,6 +136,14 @@ void PrivacyHubNotification::Update() {
   }
 }
 
+void PrivacyHubNotification::SetSecondButton(base::RepeatingClosure callback,
+                                             int title_id) {
+  message_center::RichNotificationData optional_fields = MakeOptionalFields();
+  optional_fields.buttons.emplace_back(l10n_util::GetStringUTF16(title_id));
+  builder_.SetOptionalFields(optional_fields);
+  delegate_->SetSecondButtonCallback(std::move(callback));
+}
+
 std::vector<std::u16string> PrivacyHubNotification::GetAppsAccessingSensors()
     const {
   std::vector<std::u16string> app_names;
@@ -156,4 +177,13 @@ void PrivacyHubNotification::SetNotificationMessage() {
   }
 }
 
+message_center::RichNotificationData
+PrivacyHubNotification::MakeOptionalFields() const {
+  message_center::RichNotificationData optional_fields;
+  optional_fields.remove_on_click = true;
+  optional_fields.buttons.emplace_back(button_text_);
+
+  return optional_fields;
+}
+
 }  // namespace ash

ash/system/privacy_hub/privacy_hub_notification.h

@@ -38,10 +38,16 @@ class ASH_EXPORT PrivacyHubNotificationClickDelegate
   // When clicking on the notification message execute this `callback`.
   void SetMessageClickCallback(base::RepeatingClosure callback);
 
+  // Set the `callback` for an additional button.
+  void SetSecondButtonCallback(base::RepeatingClosure callback);
+
  private:
   ~PrivacyHubNotificationClickDelegate() override;
 
-  base::RepeatingClosure button_callback_;
+  // Run `callback` if it's not null. Do nothing otherwise.
+  void RunCallbackIfNotNull(const base::RepeatingClosure& callback);
+
+  std::array<base::RepeatingClosure, 2> button_callbacks_;
   base::RepeatingClosure message_callback_;
 };
 
@@ -93,6 +99,11 @@ class ASH_EXPORT PrivacyHubNotification {
   // again.
   void Update();
 
+  // Add an additional button to the notification. The button title will be
+  // generated from the `title_id`. Clicking the button will invoke the
+  // `callback`. Only one additional button can be active at the same time.
+  void SetSecondButton(base::RepeatingClosure callback, int title_id);
+
   // Get the underlying `SystemNotificationBuilder` to do modifications beyond
   // what this wrapper allows you to do. If you change the ID of the message
   // `Show()` and `Hide()` are not going to work reliably.
@@ -107,12 +118,18 @@ class ASH_EXPORT PrivacyHubNotification {
   // `sensors_for_apps_`.
   void SetNotificationMessage();
 
+  // Create an object of optional data fields with the defaults applying to
+  // every Privacy Hub notification.
+  message_center::RichNotificationData MakeOptionalFields() const;
+
   std::string id_;
   SystemNotificationBuilder builder_;
   MessageIds message_ids_;
   SensorSet sensors_for_apps_;
   absl::optional<base::Time> last_time_shown_;
   base::OneShotTimer remove_timer_;
+  scoped_refptr<PrivacyHubNotificationClickDelegate> delegate_;
+  std::u16string button_text_;
 };
 
 }  // namespace ash

ash/system/privacy_hub/privacy_hub_notification_controller.cc

@@ -88,6 +88,11 @@ PrivacyHubNotificationController::PrivacyHubNotificationController() {
           SensorDisabledNotificationDelegate::Sensor::kMicrophone},
       combined_delegate, NotificationCatalogName::kPrivacyHubMicAndCamera,
       IDS_PRIVACY_HUB_MICROPHONE_AND_CAMERA_OFF_NOTIFICATION_BUTTON);
+
+  combined_notification_->SetSecondButton(
+      base::BindRepeating(
+          &PrivacyHubNotificationController::OpenPrivacyHubSettingsPage),
+      IDS_PRIVACY_HUB_OPEN_SETTINGS_PAGE_BUTTON);
 }
 
 PrivacyHubNotificationController::~PrivacyHubNotificationController() = default;

ash/system/privacy_hub/privacy_hub_notification_controller_unittest.cc

@@ -113,10 +113,10 @@ class PrivacyHubNotificationControllerTest : public AshTestBase {
     return nullptr;
   }
 
-  void ClickOnNotificationButton() const {
+  void ClickOnNotificationButton(int button_index = 0) const {
     message_center::MessageCenter::Get()->ClickOnNotificationButton(
         PrivacyHubNotificationController::kCombinedNotificationId,
-        /*button_index=*/0);
+        button_index);
   }
 
   void ClickOnNotificationBody() const {
@@ -259,19 +259,8 @@ TEST_F(PrivacyHubNotificationControllerTest,
   EXPECT_FALSE(
       GetNotification(MicrophonePrivacySwitchController::kNotificationId));
 
-  EXPECT_EQ(GetSystemTrayClient()->show_os_settings_privacy_hub_count(), 0);
-  EXPECT_EQ(histogram_tester().GetBucketCount(
-                privacy_hub_metrics::kPrivacyHubOpenedHistogram,
-                privacy_hub_metrics::PrivacyHubNavigationOrigin::kNotification),
-            0);
-
   ClickOnNotificationBody();
 
-  EXPECT_EQ(GetSystemTrayClient()->show_os_settings_privacy_hub_count(), 1);
-  EXPECT_EQ(histogram_tester().GetBucketCount(
-                privacy_hub_metrics::kPrivacyHubOpenedHistogram,
-                privacy_hub_metrics::PrivacyHubNavigationOrigin::kNotification),
-            1);
   ExpectNoNotificationActive();
 
   // Go to (quick)settings and enable microphone.
@@ -330,29 +319,47 @@ TEST_F(PrivacyHubNotificationControllerTest, ClickOnNotificationButton) {
             1);
 }
 
-TEST_F(PrivacyHubNotificationControllerTest, ClickOnNotificationBody) {
+TEST_F(PrivacyHubNotificationControllerTest, ClickOnSecondNotificationButton) {
   ExpectNoNotificationActive();
   ShowCombinedNotification();
   EXPECT_TRUE(GetNotification());
 
-  EXPECT_EQ(GetSystemTrayClient()->show_os_settings_privacy_hub_count(), 0);
   EXPECT_EQ(histogram_tester().GetBucketCount(
                 privacy_hub_metrics::kPrivacyHubOpenedHistogram,
                 privacy_hub_metrics::PrivacyHubNavigationOrigin::kNotification),
             0);
+  EXPECT_EQ(GetSystemTrayClient()->show_os_settings_privacy_hub_count(), 0);
 
-  ClickOnNotificationBody();
+  ClickOnNotificationButton(1);
 
   WaitUntilNotificationRemoved(kPrivacyHubCameraOffNotificationId);
 
   ExpectNoNotificationActive();
+
   EXPECT_EQ(GetSystemTrayClient()->show_os_settings_privacy_hub_count(), 1);
   EXPECT_EQ(histogram_tester().GetBucketCount(
                 privacy_hub_metrics::kPrivacyHubOpenedHistogram,
                 privacy_hub_metrics::PrivacyHubNavigationOrigin::kNotification),
             1);
 }
 
+TEST_F(PrivacyHubNotificationControllerTest, ClickOnNotificationBody) {
+  ExpectNoNotificationActive();
+  ShowCombinedNotification();
+  EXPECT_TRUE(GetNotification());
+
+  EXPECT_EQ(histogram_tester().GetBucketCount(
+                privacy_hub_metrics::kPrivacyHubOpenedHistogram,
+                privacy_hub_metrics::PrivacyHubNavigationOrigin::kNotification),
+            0);
+
+  ClickOnNotificationBody();
+
+  WaitUntilNotificationRemoved(kPrivacyHubCameraOffNotificationId);
+
+  ExpectNoNotificationActive();
+}
+
 TEST_F(PrivacyHubNotificationControllerTest, OpenPrivacyHubSettingsPage) {
   EXPECT_EQ(GetSystemTrayClient()->show_os_settings_privacy_hub_count(), 0);
   EXPECT_EQ(histogram_tester().GetBucketCount(