Fix hidden settings button in native notifications.

This will hide the settings button for native notifications if they
should not have one. This is specific to the feature that displays the
notification and is already working for non-native notifications.

Additionally, the settings button will now show for extension native
notifications, a previous CL (923877) only added it for non-native
ones.

Bug: 999979,810622
Change-Id: I26d94e275dab76d1748afdfefae78aba77977473
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1801759Reviewed-by: Peter Beverloo <peter@chromium.org>
Commit-Queue: Richard Knoll <knollr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#697494}

chrome/browser/notifications/notification_platform_bridge_linux.cc

@@ -657,8 +657,7 @@ class NotificationPlatformBridgeLinuxImpl
       actions.push_back(kDefaultButtonId);
       actions.push_back("Activate");
       // Always add a settings button for web notifications.
-      if (notification_type != NotificationHandler::Type::EXTENSION &&
-          notification_type != NotificationHandler::Type::SEND_TAB_TO_SELF) {
+      if (notification->should_show_settings_button()) {
         actions.push_back(kSettingsButtonId);
         actions.push_back(
             l10n_util::GetStringUTF8(IDS_NOTIFICATION_BUTTON_SETTINGS));

chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc

@@ -33,6 +33,7 @@
 
 using message_center::ButtonInfo;
 using message_center::Notification;
+using message_center::SettingsButtonHandler;
 using testing::_;
 using testing::ByMove;
 using testing::Return;
@@ -55,7 +56,9 @@ class NotificationBuilder {
                       GURL(),
                       message_center::NotifierId(GURL()),
                       message_center::RichNotificationData(),
-                      new message_center::NotificationDelegate()) {}
+                      new message_center::NotificationDelegate()) {
+    notification_.set_settings_button_handler(SettingsButtonHandler::DELEGATE);
+  }
 
   Notification GetResult() { return notification_; }
 
@@ -90,6 +93,11 @@ class NotificationBuilder {
     return *this;
   }
 
+  NotificationBuilder& SetSettingsButtonHandler(SettingsButtonHandler handler) {
+    notification_.set_settings_button_handler(handler);
+    return *this;
+  }
+
   NotificationBuilder& SetSilent(bool silent) {
     notification_.set_silent(silent);
     return *this;
@@ -771,6 +779,27 @@ TEST_F(NotificationPlatformBridgeLinuxTest,
       NotificationBuilder("").GetResult(), nullptr);
 }
 
+TEST_F(NotificationPlatformBridgeLinuxTest, NoSettingsButton) {
+  EXPECT_CALL(*mock_notification_proxy_.get(),
+              CallMethodAndBlock(Calls("Notify"), _))
+      .WillOnce(OnNotify(
+          [](const NotificationRequest& request) {
+            ASSERT_EQ(1UL, request.actions.size());
+            EXPECT_EQ("default", request.actions[0].id);
+            EXPECT_EQ("Activate", request.actions[0].label);
+          },
+          1));
+
+  CreateNotificationBridgeLinux(
+      TestParams().SetServerName("cinnamon").SetServerVersion("3.8.0"));
+  notification_bridge_linux_->Display(
+      NotificationHandler::Type::WEB_PERSISTENT, profile(),
+      NotificationBuilder("")
+          .SetSettingsButtonHandler(SettingsButtonHandler::NONE)
+          .GetResult(),
+      nullptr);
+}
+
 TEST_F(NotificationPlatformBridgeLinuxTest, DefaultButtonForwards) {
   EXPECT_CALL(*mock_notification_proxy_.get(),
               CallMethodAndBlock(Calls("Notify"), _))

chrome/browser/notifications/notification_platform_bridge_mac.mm

@@ -249,11 +249,7 @@ void NotificationPlatformBridgeMac::Display(
     [builder setIcon:notification.icon().ToNSImage()];
   }
 
-  [builder
-      setShowSettingsButton:(notification_type !=
-                                 NotificationHandler::Type::EXTENSION &&
-                             notification_type !=
-                                 NotificationHandler::Type::SEND_TAB_TO_SELF)];
+  [builder setShowSettingsButton:(notification.should_show_settings_button())];
   std::vector<message_center::ButtonInfo> buttons = notification.buttons();
   if (!buttons.empty()) {
     DCHECK_LE(buttons.size(), blink::kWebNotificationMaxActions);

chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm

@@ -83,7 +83,8 @@ class NotificationPlatformBridgeMacTest : public BrowserWithTestWindowTest {
                                              const char* button1,
                                              const char* button2) {
     return CreateNotification(title, subtitle, origin, button1, button2,
-                              false /* require_interaction */);
+                              /*require_interaction=*/false,
+                              /*show_settings_button=*/true);
   }
 
   std::unique_ptr<Notification> CreateAlert(const char* title,
@@ -92,7 +93,8 @@ class NotificationPlatformBridgeMacTest : public BrowserWithTestWindowTest {
                                             const char* button1,
                                             const char* button2) {
     return CreateNotification(title, subtitle, origin, button1, button2,
-                              true /* require_interaction */);
+                              /*require_interaction=*/true,
+                              /*show_settings_button=*/true);
   }
 
   std::unique_ptr<Notification> CreateNotification(const char* title,
@@ -100,7 +102,8 @@ class NotificationPlatformBridgeMacTest : public BrowserWithTestWindowTest {
                                                    const char* origin,
                                                    const char* button1,
                                                    const char* button2,
-                                                   bool require_interaction) {
+                                                   bool require_interaction,
+                                                   bool show_settings_button) {
     message_center::RichNotificationData optional_fields;
     if (button1) {
       optional_fields.buttons.push_back(
@@ -110,6 +113,10 @@ class NotificationPlatformBridgeMacTest : public BrowserWithTestWindowTest {
             message_center::ButtonInfo(base::UTF8ToUTF16(button2)));
       }
     }
+    if (show_settings_button) {
+      optional_fields.settings_button_handler =
+          message_center::SettingsButtonHandler::DELEGATE;
+    }
 
     GURL url = GURL(origin);
 
@@ -230,6 +237,28 @@ TEST_F(NotificationPlatformBridgeMacTest, TestDisplayNoButtons) {
   EXPECT_NSEQ(@"Settings", [delivered_notification actionButtonTitle]);
 }
 
+TEST_F(NotificationPlatformBridgeMacTest, TestDisplayNoSettings) {
+  std::unique_ptr<Notification> notification = CreateNotification(
+      "Title", "Context", "https://gmail.com", nullptr, nullptr,
+      /*require_interaction=*/false, /*show_settings_button=*/false);
+
+  std::unique_ptr<NotificationPlatformBridgeMac> bridge(
+      new NotificationPlatformBridgeMac(notification_center(),
+                                        alert_dispatcher()));
+  bridge->Display(NotificationHandler::Type::WEB_PERSISTENT, profile(),
+                  *notification, nullptr);
+  NSArray* notifications = [notification_center() deliveredNotifications];
+
+  EXPECT_EQ(1u, [notifications count]);
+
+  NSUserNotification* delivered_notification = [notifications objectAtIndex:0];
+  EXPECT_NSEQ(@"Title", [delivered_notification title]);
+  EXPECT_NSEQ(@"Context", [delivered_notification informativeText]);
+  EXPECT_NSEQ(@"gmail.com", [delivered_notification subtitle]);
+  EXPECT_NSEQ(@"Close", [delivered_notification otherButtonTitle]);
+  EXPECT_FALSE([delivered_notification hasActionButton]);
+}
+
 TEST_F(NotificationPlatformBridgeMacTest, TestDisplayOneButton) {
   std::unique_ptr<Notification> notification = CreateBanner(
       "Title", "Context", "https://gmail.com", "Button 1", nullptr);

chrome/browser/notifications/win/notification_template_builder.cc

@@ -390,12 +390,17 @@ base::string16 BuildNotificationTemplate(
   if (!notification.buttons().empty())
     AddActions(&xml_writer, image_retainer, notification, launch_id);
   EnsureReminderHasButton(&xml_writer, notification, launch_id);
-  if (context_menu_label_override) {
-    AddContextMenu(&xml_writer, launch_id, context_menu_label_override);
+  if (notification.should_show_settings_button()) {
+    if (context_menu_label_override) {
+      AddContextMenu(&xml_writer, launch_id, context_menu_label_override);
+    } else {
+      AddContextMenu(&xml_writer, launch_id,
+                     l10n_util::GetStringUTF8(
+                         IDS_WIN_NOTIFICATION_SETTINGS_CONTEXT_MENU_ITEM_NAME));
+    }
   } else {
-    AddContextMenu(&xml_writer, launch_id,
-                   l10n_util::GetStringUTF8(
-                       IDS_WIN_NOTIFICATION_SETTINGS_CONTEXT_MENU_ITEM_NAME));
+    DCHECK(!context_menu_label_override)
+        << "Must show custom settings button label";
   }
   EndActionsElement(&xml_writer);
 

chrome/browser/notifications/win/notification_template_builder_unittest.cc

@@ -70,6 +70,8 @@ class NotificationTemplateBuilderTest : public ::testing::Test {
         NotifierId(origin_url), RichNotificationData(), nullptr /* delegate */);
     // Set a fixed timestamp, to avoid having to test against current timestamp.
     notification.set_timestamp(FixedTime());
+    notification.set_settings_button_handler(
+        message_center::SettingsButtonHandler::DELEGATE);
     return notification;
   }
 
@@ -481,3 +483,28 @@ title4 - message4
 
   ASSERT_NO_FATAL_FAILURE(VerifyXml(notification, kExpectedXml));
 }
+
+TEST_F(NotificationTemplateBuilderTest, NoSettings) {
+  message_center::Notification notification = BuildNotification();
+
+  // Disable overriding context menu label.
+  SetContextMenuLabelForTesting(nullptr);
+
+  notification.set_settings_button_handler(
+      message_center::SettingsButtonHandler::NONE);
+
+  const wchar_t kExpectedXml[] =
+      LR"(<toast launch="0|0|Default|0|https://example.com/|notification_id" displayTimestamp="1998-09-04T01:02:03Z">
+ <visual>
+  <binding template="ToastGeneric">
+   <text>My Title</text>
+   <text>My Message</text>
+   <text placement="attribution">example.com</text>
+  </binding>
+ </visual>
+ <actions/>
+</toast>
+)";
+
+  ASSERT_NO_FATAL_FAILURE(VerifyXml(notification, kExpectedXml));
+}

ui/message_center/public/cpp/notification.h

@@ -403,6 +403,10 @@ class MESSAGE_CENTER_PUBLIC_EXPORT Notification {
            SettingsButtonHandler::NONE;
   }
 
+  void set_settings_button_handler(SettingsButtonHandler handler) {
+    optional_fields_.settings_button_handler = handler;
+  }
+
   bool should_show_snooze_button() const {
     return optional_fields_.should_show_snooze_button;
   }