Reland "[Privacy Hub] Add Privacy Hub button to combined notification"

This is a reland of commit ba83a8001b

Original change's description:
> [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}

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

ash/ash_strings.grd

@@ -5002,6 +5002,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"
@@ -29,22 +28,24 @@ bool HasNotification(const std::string& id) {
 namespace ash {
 
 PrivacyHubNotificationClickDelegate::PrivacyHubNotificationClickDelegate(
-    base::RepeatingClosure button_click)
+    base::RepeatingClosure button_click) {
-    : button_callback_(std::move(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()) {
+  if (button_index_opt.has_value()) {
-    button_callback_.Run();
+    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()) {
+    RunCallbackIfNotNull(message_callback_);
-      message_callback_.Run();
-    }
-
-    PrivacyHubNotificationController::OpenPrivacyHubSettingsPage();
   }
 }
 
@@ -53,6 +54,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,
@@ -61,21 +74,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);
 }
@@ -126,6 +139,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;
@@ -159,4 +180,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_;
 };
 
@@ -95,6 +101,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.
@@ -109,12 +120,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,12 +319,35 @@ TEST_F(PrivacyHubNotificationControllerTest, ClickOnNotificationButton) {
             1);
 }
 
+TEST_F(PrivacyHubNotificationControllerTest, ClickOnSecondNotificationButton) {
+  ExpectNoNotificationActive();
+  ShowCombinedNotification();
+  EXPECT_TRUE(GetNotification());
+
+  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);
+
+  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(GetSystemTrayClient()->show_os_settings_privacy_hub_count(), 0);
   EXPECT_EQ(histogram_tester().GetBucketCount(
                 privacy_hub_metrics::kPrivacyHubOpenedHistogram,
                 privacy_hub_metrics::PrivacyHubNavigationOrigin::kNotification),
@@ -346,11 +358,6 @@ TEST_F(PrivacyHubNotificationControllerTest, ClickOnNotificationBody) {
   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, OpenPrivacyHubSettingsPage) {

ash/system/privacy_hub/privacy_hub_notification_unittest.cc

@@ -4,7 +4,6 @@
 
 #include "ash/system/privacy_hub/privacy_hub_notification.h"
 
-#include "ash/public/cpp/test/test_system_tray_client.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "ash/test/ash_test_base.h"
 #include "base/memory/scoped_refptr.h"
@@ -146,20 +145,16 @@ TEST_F(PrivacyHubNotificationClickDelegateTest, Click) {
           base::BindLambdaForTesting(
               [&button_clicked]() { button_clicked++; }));
 
-  ASSERT_EQ(GetSystemTrayClient()->show_os_settings_privacy_hub_count(), 0);
+  // Clicking the message while no callback for it is added shouldn't result in
-
+  // a callback being executed.
-  // Without callback only Privacy Hub should be opened when clicking on the
-  // message.
   delegate->Click(absl::nullopt, absl::nullopt);
 
-  EXPECT_EQ(GetSystemTrayClient()->show_os_settings_privacy_hub_count(), 1);
   EXPECT_EQ(button_clicked, 0u);
   EXPECT_EQ(message_clicked, 0u);
 
   // Click the button.
   delegate->Click(0, absl::nullopt);
 
-  EXPECT_EQ(GetSystemTrayClient()->show_os_settings_privacy_hub_count(), 1);
   EXPECT_EQ(button_clicked, 1u);
   EXPECT_EQ(message_clicked, 0u);
 
@@ -170,19 +165,39 @@ TEST_F(PrivacyHubNotificationClickDelegateTest, Click) {
   // When clicking the button, only the button callback should be executed.
   delegate->Click(0, absl::nullopt);
 
-  EXPECT_EQ(GetSystemTrayClient()->show_os_settings_privacy_hub_count(), 1);
   EXPECT_EQ(button_clicked, 2u);
   EXPECT_EQ(message_clicked, 0u);
 
-  // Clicking the message should open Privacy Hub and execute the message
+  // Clicking the message should execute the message callback.
-  // callback.
   delegate->Click(absl::nullopt, absl::nullopt);
 
-  EXPECT_EQ(GetSystemTrayClient()->show_os_settings_privacy_hub_count(), 2);
   EXPECT_EQ(button_clicked, 2u);
   EXPECT_EQ(message_clicked, 1u);
 }
 
+TEST(PrivacyHubNotificationClickDelegateDeathTest, AddButton) {
+  scoped_refptr<PrivacyHubNotificationClickDelegate> delegate =
+      base::MakeRefCounted<PrivacyHubNotificationClickDelegate>(
+          base::DoNothing());
+
+  if (DCHECK_IS_ON()) {
+    EXPECT_DEATH(
+        delegate->Click(1, absl::nullopt),
+        "Check failed: !button_callbacks_\\[button_index\\]\\.is_null\\(\\). "
+        "button_index=1");
+  }
+}
+
+// TODO(b/271280503): Reenable when the failing builder is fixed.
+TEST(PrivacyHubNotificationClickDelegateDeathTest, DISABLED_AddButton2) {
+  scoped_refptr<PrivacyHubNotificationClickDelegate> delegate =
+      base::MakeRefCounted<PrivacyHubNotificationClickDelegate>(
+          base::DoNothing());
+  EXPECT_DEATH(delegate->Click(2, absl::nullopt),
+               "Check failed: button_callbacks_.size\\(\\) > button_index \\(2 "
+               "vs. 2\\)");
+}
+
 TEST_F(PrivacyHubNotificationTest, ShowAndHide) {
   EXPECT_FALSE(GetNotification());
 
@@ -253,6 +268,26 @@ TEST_F(PrivacyHubNotificationTest, UpdateNotification) {
   EXPECT_FALSE(GetPopupNotification());
 }
 
+TEST_F(PrivacyHubNotificationTest, AddButton) {
+  notification().Show();
+
+  EXPECT_EQ(GetNotification()->rich_notification_data().buttons.size(), 1u);
+
+  int second_button_clicked = 0;
+  notification().SetSecondButton(
+      base::BindLambdaForTesting(
+          [&second_button_clicked]() { second_button_clicked++; }),
+      IDS_PRIVACY_HUB_OPEN_SETTINGS_PAGE_BUTTON);
+
+  notification().Update();
+  message_center::Notification* test_notification = GetNotification();
+  ASSERT_EQ(test_notification->rich_notification_data().buttons.size(), 2u);
+
+  EXPECT_EQ(second_button_clicked, 0);
+  test_notification->delegate()->Click(1, absl::nullopt);
+  EXPECT_EQ(second_button_clicked, 1);
+}
+
 TEST_F(PrivacyHubNotificationTest, WithApps) {
   // No apps -> generic notification text.
   notification().Show();