Respect Notification::renotify in the MessageCenter.

This allows us to get rid of an ugly priority promotion hack used for
download notifications on Chrome OS.

On platforms with a message center bubble (Chrome OS), web
notifications that don't specify renotify won't pop
up again if the original popup was dismissed.

On platforms without a message center bubble (Linux, Windows when
using non-native notifications), all notifications will pop up again
on update.

Bug: none
Change-Id: I6a00b21348335b29a97a78a603c03a31ae8e5452
Reviewed-on: https://chromium-review.googlesource.com/976545Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552424}

chrome/browser/download/notification/download_item_notification.cc

@@ -333,15 +333,14 @@ void DownloadItemNotification::Update() {
   auto download_state = item_->GetState();
 
   // When the download is just completed, interrupted or transitions to
-  // dangerous, increase the priority over its previous value to make sure it
-  // pops up again.
-  bool popup =
+  // dangerous, make sure it pops up again.
+  bool pop_up =
       ((item_->IsDangerous() && !previous_dangerous_state_) ||
        (download_state == download::DownloadItem::COMPLETE &&
         previous_download_state_ != download::DownloadItem::COMPLETE) ||
        (download_state == download::DownloadItem::INTERRUPTED &&
         previous_download_state_ != download::DownloadItem::INTERRUPTED));
-  UpdateNotificationData(!closed_ || show_next_ || popup, popup);
+  UpdateNotificationData(!closed_ || show_next_ || pop_up, pop_up);
 
   show_next_ = false;
   previous_download_state_ = item_->GetState();
@@ -349,7 +348,7 @@ void DownloadItemNotification::Update() {
 }
 
 void DownloadItemNotification::UpdateNotificationData(bool display,
-                                                      bool bump_priority) {
+                                                      bool force_pop_up) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   if (item_->GetState() == download::DownloadItem::CANCELLED) {
@@ -427,12 +426,10 @@ void DownloadItemNotification::UpdateNotificationData(bool display,
   }
   notification_->set_buttons(notification_actions);
 
+  notification_->set_renotify(force_pop_up);
+
   if (display) {
     closed_ = false;
-    if (bump_priority &&
-        notification_->priority() < message_center::HIGH_PRIORITY) {
-      notification_->set_priority(notification_->priority() + 1);
-    }
     NotificationDisplayServiceFactory::GetForProfile(profile())->Display(
         NotificationHandler::Type::TRANSIENT, *notification_);
   }

chrome/browser/download/notification/download_item_notification.h

@@ -56,7 +56,7 @@ class DownloadItemNotification : public ImageDecoder::ImageRequest,
 
   void CloseNotification();
   void Update();
-  void UpdateNotificationData(bool display, bool bump_priority);
+  void UpdateNotificationData(bool display, bool force_pop_up);
   SkColor GetNotificationIconColor();
 
   // Set preview image of the notification. Must be called on IO thread.

chrome/browser/download/notification/download_notification_interactive_uitest.cc

@@ -572,12 +572,10 @@ IN_PROC_BROWSER_TEST_F(DownloadNotificationTest, DownloadMultipleFiles) {
   EXPECT_FALSE(notification_id2.empty());
 
   // Confirms that the old one is low priority, and the new one is default.
-  const int in_progress_priority1 =
-      GetNotification(notification_id1)->priority();
-  const int in_progress_priority2 =
-      GetNotification(notification_id2)->priority();
-  EXPECT_EQ(message_center::LOW_PRIORITY, in_progress_priority1);
-  EXPECT_EQ(message_center::DEFAULT_PRIORITY, in_progress_priority2);
+  EXPECT_EQ(message_center::LOW_PRIORITY,
+            GetNotification(notification_id1)->priority());
+  EXPECT_EQ(message_center::DEFAULT_PRIORITY,
+            GetNotification(notification_id2)->priority());
 
   // Confirms that the updates of both download are delivered to the
   // notifications.
@@ -599,11 +597,15 @@ IN_PROC_BROWSER_TEST_F(DownloadNotificationTest, DownloadMultipleFiles) {
   ASSERT_TRUE(GetNotification(notification_id1));
   ASSERT_TRUE(GetNotification(notification_id2));
 
-  // Confirms that both increase in priority when finished.
-  EXPECT_GT(GetNotification(notification_id1)->priority(),
-            in_progress_priority1);
-  EXPECT_GT(GetNotification(notification_id2)->priority(),
-            in_progress_priority2);
+  // Confirms that both ask to be re-shown when finished.
+  EXPECT_TRUE(GetNotification(notification_id1)->renotify());
+  EXPECT_TRUE(GetNotification(notification_id2)->renotify());
+
+  // Confirms that both are default priority after finishing.
+  EXPECT_EQ(message_center::DEFAULT_PRIORITY,
+            GetNotification(notification_id1)->priority());
+  EXPECT_EQ(message_center::DEFAULT_PRIORITY,
+            GetNotification(notification_id2)->priority());
 
   // Confirms the types of download notifications are correct.
   EXPECT_EQ(message_center::NOTIFICATION_TYPE_BASE_FORMAT,

chrome/browser/ui/views/message_center/popups_only_ui_delegate.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/ui/views/message_center/popups_only_ui_delegate.h"
 
 #include "ui/display/screen.h"
+#include "ui/message_center/message_center.h"
 #include "ui/message_center/ui_controller.h"
 #include "ui/message_center/views/desktop_popup_alignment_delegate.h"
 #include "ui/message_center/views/message_popup_collection.h"
@@ -18,6 +19,7 @@ PopupsOnlyUiDelegate::PopupsOnlyUiDelegate() {
   alignment_delegate_.reset(new message_center::DesktopPopupAlignmentDelegate);
   popup_collection_.reset(new message_center::MessagePopupCollection(
       message_center(), ui_controller_.get(), alignment_delegate_.get()));
+  message_center()->SetHasMessageCenterView(false);
 }
 
 PopupsOnlyUiDelegate::~PopupsOnlyUiDelegate() {

ui/message_center/fake_message_center.cc

@@ -124,6 +124,14 @@ bool FakeMessageCenter::IsMessageCenterVisible() const {
   return false;
 }
 
+void FakeMessageCenter::SetHasMessageCenterView(bool has_message_center_view) {
+  has_message_center_view_ = has_message_center_view;
+}
+
+bool FakeMessageCenter::HasMessageCenterView() const {
+  return has_message_center_view_;
+}
+
 void FakeMessageCenter::RestartPopupTimers() {}
 
 void FakeMessageCenter::PausePopupTimers() {}

ui/message_center/fake_message_center.h

@@ -63,6 +63,8 @@ class FakeMessageCenter : public MessageCenter {
   void EnterQuietModeWithExpire(const base::TimeDelta& expires_in) override;
   void SetVisibility(Visibility visible) override;
   bool IsMessageCenterVisible() const override;
+  void SetHasMessageCenterView(bool has_message_center_view) override;
+  bool HasMessageCenterView() const override;
   void RestartPopupTimers() override;
   void PausePopupTimers() override;
   const base::string16& GetSystemNotificationAppName() const override;
@@ -73,6 +75,7 @@ class FakeMessageCenter : public MessageCenter {
 
  private:
   const NotificationList::Notifications empty_notifications_;
+  bool has_message_center_view_ = true;
 
   DISALLOW_COPY_AND_ASSIGN(FakeMessageCenter);
 };

ui/message_center/message_center.h

@@ -175,6 +175,14 @@ class MESSAGE_CENTER_EXPORT MessageCenter {
   // Allows querying the visibility of the center.
   virtual bool IsMessageCenterVisible() const = 0;
 
+  // Informs the MessageCenter whether there's a bubble anchored to a system
+  // tray which holds notifications. If false, only toasts are shown (e.g. on
+  // desktop Linux and Windows). When there's no message center view, updated
+  // notifications will be re-appear as toasts even if they've already been
+  // shown.
+  virtual void SetHasMessageCenterView(bool has_message_center_view) = 0;
+  virtual bool HasMessageCenterView() const = 0;
+
   // UI classes should call this when there is cause to leave popups visible for
   // longer than the default (for example, when the mouse hovers over a popup).
   virtual void PausePopupTimers() = 0;

ui/message_center/message_center_impl.cc

@@ -109,6 +109,16 @@ bool MessageCenterImpl::IsMessageCenterVisible() const {
   return visible_;
 }
 
+void MessageCenterImpl::SetHasMessageCenterView(bool has_message_center_view) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  has_message_center_view_ = has_message_center_view;
+}
+
+bool MessageCenterImpl::HasMessageCenterView() const {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  return has_message_center_view_;
+}
+
 size_t MessageCenterImpl::NotificationCount() const {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   return visible_notifications_.size();

ui/message_center/message_center_impl.h

@@ -41,6 +41,8 @@ class MESSAGE_CENTER_EXPORT MessageCenterImpl
   void RemoveNotificationBlocker(NotificationBlocker* blocker) override;
   void SetVisibility(Visibility visible) override;
   bool IsMessageCenterVisible() const override;
+  void SetHasMessageCenterView(bool has_message_center_view) override;
+  bool HasMessageCenterView() const override;
   size_t NotificationCount() const override;
   bool HasPopupNotifications() const override;
   bool IsQuietMode() const override;
@@ -97,6 +99,7 @@ class MESSAGE_CENTER_EXPORT MessageCenterImpl
   std::vector<NotificationBlocker*> blockers_;
 
   bool visible_ = false;
+  bool has_message_center_view_ = true;
 
   base::string16 system_notification_app_name_;
 

ui/message_center/notification_list.cc

@@ -96,15 +96,10 @@ void NotificationList::UpdateNotificationMessage(
   if (iter == notifications_.end())
     return;
 
-  Notification* notification = iter->first.get();
   NotificationState state = iter->second;
 
-  // Handles priority promotion. If the notification is already dismissed but
-  // the updated notification has higher priority, it should re-appear as a
-  // toast. Notifications coming from websites through the Web Notification API
-  // will always re-appear on update.
-  if ((notification->priority() < new_notification->priority() ||
-       new_notification->notifier_id().type == NotifierId::WEB_PAGE) &&
+  if ((new_notification->renotify() ||
+       !message_center_->HasMessageCenterView()) &&
       !quiet_mode_) {
     state = NotificationState();
   }

ui/message_center/public/mojo/notification.mojom

@@ -62,7 +62,7 @@ struct RichNotificationData {
   bool should_make_spoken_feedback_for_popup_updates;
   bool pinned;
   // |vibration_pattern| intentionally omitted
-  // |renotify| intentionally omitted
+  bool renotify;
   // |silent| intentionally omitted
   mojo_base.mojom.String16 accessible_name;
   string vector_small_image_id;

ui/message_center/public/mojo/notification_struct_traits.cc

@@ -116,6 +116,11 @@ bool RichNotificationDataStructTraits::pinned(const RichNotificationData& r) {
   return r.pinned;
 }
 
+// static
+bool RichNotificationDataStructTraits::renotify(const RichNotificationData& r) {
+  return r.renotify;
+}
+
 // static
 const base::string16& RichNotificationDataStructTraits::accessible_name(
     const RichNotificationData& r) {
@@ -166,6 +171,7 @@ bool RichNotificationDataStructTraits::Read(RichNotificationDataDataView data,
   out->should_make_spoken_feedback_for_popup_updates =
       data.should_make_spoken_feedback_for_popup_updates();
   out->pinned = data.pinned();
+  out->renotify = data.renotify();
 
   // Look up the vector icon by ID. This will only work if RegisterVectorIcon
   // has been called with an appropriate icon.

ui/message_center/public/mojo/notification_struct_traits.h

@@ -176,6 +176,7 @@ struct StructTraits<message_center::mojom::RichNotificationDataDataView,
   static bool should_make_spoken_feedback_for_popup_updates(
       const message_center::RichNotificationData& r);
   static bool pinned(const message_center::RichNotificationData& r);
+  static bool renotify(const message_center::RichNotificationData& r);
   static const base::string16& accessible_name(
       const message_center::RichNotificationData& r);
   static std::string vector_small_image_id(

ui/message_center/public/mojo/struct_traits_unittest.cc

@@ -49,6 +49,8 @@ class StructTraitsTest : public testing::Test, public mojom::TraitsTestService {
                   .should_make_spoken_feedback_for_popup_updates,
               output.rich_notification_data()
                   .should_make_spoken_feedback_for_popup_updates);
+    EXPECT_EQ(input.pinned(), output.pinned());
+    EXPECT_EQ(input.renotify(), output.renotify());
     EXPECT_EQ(input.accessible_name(), output.accessible_name());
     EXPECT_EQ(input.accent_color(), output.accent_color());
     EXPECT_EQ(input.should_show_settings_button(),