Windows Native Notifications: Add a Close button for reminders if no buttons are present.

Bug: 781792
Change-Id: Ieb5c92cf3c926e508576cb812b16ca6dda12dae9
Reviewed-on: https://chromium-review.googlesource.com/1202663
Commit-Queue: Finnur Thorarinsson <finnur@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588530}

chrome/browser/notifications/notification_platform_bridge_win.cc

@@ -837,6 +837,11 @@ bool NotificationPlatformBridgeWin::HandleActivation(
     return false;
   }
 
+  if (launch_id.is_for_dismiss_button()) {
+    LogActivationStatus(ActivationStatus::SUCCESS);
+    return true;  // We're done! The toast has already dismissed.
+  }
+
   base::Optional<base::string16> reply;
   base::string16 inline_reply =
       command_line.GetSwitchValueNative(switches::kNotificationInlineReply);

chrome/browser/notifications/win/notification_launch_id.cc

@@ -15,6 +15,7 @@ enum LaunchIdComponents {
   NORMAL = 0,
   BUTTON_INDEX = 1,
   CONTEXT_MENU = 2,
+  DISMISS_BUTTON = 3,
 };
 
 // These values are persisted to logs. Entries should not be renumbered and
@@ -90,6 +91,11 @@ NotificationLaunchId::NotificationLaunchId(const std::string& input) {
       min_num_tokens = 6;
       is_for_context_menu_ = true;
       break;
+    case DISMISS_BUTTON:
+      // type|notification_type|profile_id|incognito|origin|notification_id
+      min_num_tokens = 6;
+      is_for_dismiss_button_ = true;
+      break;
     default:
       // |components| has an invalid value.
       LogLaunchIdDecodeStatus(LaunchIdDecodeStatus::kComponentIdOutOfRange);
@@ -142,9 +148,16 @@ std::string NotificationLaunchId::Serialize() const {
   // and unsafe for origins -- and should therefore be encoded (as per
   // http://www.ietf.org/rfc/rfc1738.txt).
   std::string prefix;
-  LaunchIdComponents type = is_for_context_menu_
-                                ? CONTEXT_MENU
-                                : (button_index_ > -1 ? BUTTON_INDEX : NORMAL);
+  LaunchIdComponents type;
+  if (is_for_context_menu_)
+    type = CONTEXT_MENU;
+  else if (is_for_dismiss_button_)
+    type = DISMISS_BUTTON;
+  else if (button_index_ > -1)
+    type = BUTTON_INDEX;
+  else
+    type = NORMAL;
+
   if (button_index_ > -1)
     prefix = base::StringPrintf("|%d", button_index_);
   return base::StringPrintf(

chrome/browser/notifications/win/notification_launch_id.h

@@ -45,6 +45,11 @@ class NotificationLaunchId {
     is_for_context_menu_ = true;
   }
 
+  void set_is_for_dismiss_button() {
+    DCHECK_EQ(-1, button_index_);
+    is_for_dismiss_button_ = true;
+  }
+
   NotificationHandler::Type notification_type() const {
     DCHECK(is_valid());
     return notification_type_;
@@ -80,6 +85,11 @@ class NotificationLaunchId {
     return is_for_context_menu_;
   }
 
+  bool is_for_dismiss_button() const {
+    DCHECK(is_valid());
+    return is_for_dismiss_button_;
+  }
+
  private:
   // The notification type this launch ID is associated with.
   NotificationHandler::Type notification_type_;
@@ -100,9 +110,12 @@ class NotificationLaunchId {
   // The button index this launch ID is associated with.
   int button_index_ = -1;
 
-  // A flag indicating if this launch ID is targeting for context menu or not.
+  // A flag indicating if this launch ID is targeting the context menu or not.
   bool is_for_context_menu_ = false;
 
+  // A flag indicating if this launch ID is targeting the dismiss button or not.
+  bool is_for_dismiss_button_ = false;
+
   // A flag indicating if this launch ID is valid.
   bool is_valid_ = false;
 };

chrome/browser/notifications/win/notification_launch_id_unittest.cc

@@ -36,6 +36,16 @@ TEST(NotificationLaunchIdTest, SerializationTests) {
     EXPECT_EQ("2|0|Default|1|https://example.com/|notification_id",
               id.Serialize());
   }
+
+  {
+    NotificationLaunchId id(NotificationHandler::Type::WEB_PERSISTENT,
+                            "notification_id", "Default", true,
+                            GURL("https://example.com"));
+    id.set_is_for_dismiss_button();
+    ASSERT_TRUE(id.is_valid());
+    EXPECT_EQ("3|0|Default|1|https://example.com/|notification_id",
+              id.Serialize());
+  }
 }
 
 TEST(NotificationLaunchIdTest, ParsingTests) {
@@ -47,6 +57,7 @@ TEST(NotificationLaunchIdTest, ParsingTests) {
     ASSERT_TRUE(id.is_valid());
     EXPECT_EQ(-1, id.button_index());
     EXPECT_FALSE(id.is_for_context_menu());
+    EXPECT_FALSE(id.is_for_dismiss_button());
     EXPECT_EQ(NotificationHandler::Type::WEB_PERSISTENT,
               id.notification_type());
     EXPECT_TRUE(id.incognito());
@@ -63,6 +74,7 @@ TEST(NotificationLaunchIdTest, ParsingTests) {
     ASSERT_TRUE(id.is_valid());
     EXPECT_EQ(-1, id.button_index());
     EXPECT_FALSE(id.is_for_context_menu());
+    EXPECT_FALSE(id.is_for_dismiss_button());
     EXPECT_EQ(NotificationHandler::Type::WEB_PERSISTENT,
               id.notification_type());
     EXPECT_TRUE(id.incognito());
@@ -79,6 +91,7 @@ TEST(NotificationLaunchIdTest, ParsingTests) {
     ASSERT_TRUE(id.is_valid());
     EXPECT_EQ(0, id.button_index());
     EXPECT_FALSE(id.is_for_context_menu());
+    EXPECT_FALSE(id.is_for_dismiss_button());
     EXPECT_EQ(NotificationHandler::Type::WEB_PERSISTENT,
               id.notification_type());
     EXPECT_TRUE(id.incognito());
@@ -96,6 +109,7 @@ TEST(NotificationLaunchIdTest, ParsingTests) {
     ASSERT_TRUE(id.is_valid());
     EXPECT_EQ(0, id.button_index());
     EXPECT_FALSE(id.is_for_context_menu());
+    EXPECT_FALSE(id.is_for_dismiss_button());
     EXPECT_EQ(NotificationHandler::Type::WEB_PERSISTENT,
               id.notification_type());
     EXPECT_TRUE(id.incognito());
@@ -111,6 +125,23 @@ TEST(NotificationLaunchIdTest, ParsingTests) {
     ASSERT_TRUE(id.is_valid());
     EXPECT_EQ(-1, id.button_index());
     EXPECT_TRUE(id.is_for_context_menu());
+    EXPECT_FALSE(id.is_for_dismiss_button());
+    EXPECT_EQ(NotificationHandler::Type::WEB_PERSISTENT,
+              id.notification_type());
+    EXPECT_TRUE(id.incognito());
+    EXPECT_EQ("Default", id.profile_id());
+    EXPECT_EQ("notification_id", id.notification_id());
+  }
+
+  // Input string for when the context menu item is selected.
+  {
+    std::string encoded = "3|0|Default|1|https://example.com/|notification_id";
+    NotificationLaunchId id(encoded);
+
+    ASSERT_TRUE(id.is_valid());
+    EXPECT_EQ(-1, id.button_index());
+    EXPECT_FALSE(id.is_for_context_menu());
+    EXPECT_TRUE(id.is_for_dismiss_button());
     EXPECT_EQ(NotificationHandler::Type::WEB_PERSISTENT,
               id.notification_type());
     EXPECT_TRUE(id.incognito());
@@ -142,6 +173,8 @@ TEST(NotificationLaunchIdTest, ParsingErrorCases) {
       {"1"},
       // Missing all but the component type (type CONTEXT_MENU).
       {"2"},
+      // Missing all but the component type (type DISMISS_BUTTON).
+      {"3"},
   };
 
   for (const auto& test_case : cases) {

chrome/browser/notifications/win/notification_template_builder.cc

@@ -18,6 +18,7 @@
 #include "chrome/browser/notifications/win/notification_image_retainer.h"
 #include "chrome/browser/notifications/win/notification_launch_id.h"
 #include "chrome/grit/chromium_strings.h"
+#include "chrome/grit/generated_resources.h"
 #include "components/url_formatter/elide_url.h"
 #include "third_party/libxml/chromium/libxml_utils.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -34,6 +35,7 @@ const char kActivationType[] = "activationType";
 const char kArguments[] = "arguments";
 const char kAttribution[] = "attribution";
 const char kAudioElement[] = "audio";
+const char kBackground[] = "background";
 const char kBindingElement[] = "binding";
 const char kBindingElementTemplateAttribute[] = "template";
 const char kContent[] = "content";
@@ -129,6 +131,7 @@ std::unique_ptr<NotificationTemplateBuilder> NotificationTemplateBuilder::Build(
   builder->StartActionsElement();
   if (!notification.buttons().empty())
     builder->AddActions(notification, launch_id);
+  builder->EnsureReminderHasButton(notification, launch_id);
   builder->AddContextMenu(launch_id);
   builder->EndActionsElement();
 
@@ -374,6 +377,23 @@ void NotificationTemplateBuilder::WriteContextMenuElement(
   xml_writer_->EndElement();
 }
 
+void NotificationTemplateBuilder::EnsureReminderHasButton(
+    const message_center::Notification& notification,
+    NotificationLaunchId copied_launch_id) {
+  if (!notification.never_timeout() || !notification.buttons().empty())
+    return;
+
+  xml_writer_->StartElement(kActionElement);
+  xml_writer_->AddAttribute(kActivationType, kBackground);
+  // TODO(finnur): Add our own string here (we're past string-freeze so we're
+  // re-using the already translated "Close" from elsewhere).
+  xml_writer_->AddAttribute(
+      kContent, l10n_util::GetStringUTF8(IDS_MD_HISTORY_CLOSE_MENU_PROMO));
+  copied_launch_id.set_is_for_dismiss_button();
+  xml_writer_->AddAttribute(kArguments, copied_launch_id.Serialize());
+  xml_writer_->EndElement();
+}
+
 void NotificationTemplateBuilder::OverrideContextMenuLabelForTesting(
     const char* label) {
   context_menu_label_override_ = label;

chrome/browser/notifications/win/notification_template_builder.h

@@ -130,6 +130,13 @@ class NotificationTemplateBuilder {
   void WriteContextMenuElement(const std::string& content,
                                const std::string& arguments);
 
+  // Ensures that every reminder has at least one button, as the Action Center
+  // does not respect the Reminder setting on notifications with no buttons, so
+  // we must add a Dismiss button to the notification for those cases. For more
+  // details, see issue https://crbug.com/781792.
+  void EnsureReminderHasButton(const message_center::Notification& notification,
+                               NotificationLaunchId copied_launch_id);
+
   // Label to override context menu items in tests.
   static const char* context_menu_label_override_;