Show the update in message center for progress notifications

BUG=170924
TEST=new tests added

Review URL: https://chromiumcodereview.appspot.com/21411002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@215216 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 204 insertions, 60 deletions

diff --git a/chrome/browser/notifications/message_center_notification_manager.cc b/chrome/browser/notifications/message_center_notification_manager.cc
index 76458b4..0ccc840 100644
--- a/chrome/browser/notifications/message_center_notification_manager.cc
+++ b/chrome/browser/notifications/message_center_notification_manager.cc
@@ -164,7 +164,12 @@ bool MessageCenterNotificationManager::ShowNotification(
 
 bool MessageCenterNotificationManager::UpdateNotification(
     const Notification& notification, Profile* profile) {
-  if (message_center_->IsMessageCenterVisible())
+  // Only progress notification update can be reflected immediately in the
+  // message center.
+  bool update_progress_notification =
+      notification.type() == message_center::NOTIFICATION_TYPE_PROGRESS;
+  bool is_message_center_visible = message_center_->IsMessageCenterVisible();
+  if (!update_progress_notification && is_message_center_visible)
     return false;
 
   const string16& replace_id = notification.replace_id();
@@ -183,13 +188,20 @@ bool MessageCenterNotificationManager::UpdateNotification(
     if (old_notification->notification().replace_id() == replace_id &&
         old_notification->notification().origin_url() == origin_url &&
         old_notification->profile() == profile) {
+      // Changing the type from non-progress to progress does not count towards
+      // the immediate update allowed in the message center.
+      if (update_progress_notification && is_message_center_visible &&
+          old_notification->notification().type() !=
+              message_center::NOTIFICATION_TYPE_PROGRESS) {
+        return false;
+      }
+
       std::string old_id =
           old_notification->notification().notification_id();
       DCHECK(message_center_->HasNotification(old_id));
 
       // Add/remove notification in the local list but just update the same
       // one in MessageCenter.
-      old_notification->notification().Close(false); // Not by user.
       delete old_notification;
       profile_notifications_.erase(old_id);
       ProfileNotification* new_notification =
diff --git a/chrome/browser/notifications/message_center_notifications_browsertest.cc b/chrome/browser/notifications/message_center_notifications_browsertest.cc
index 3469414..28b4728 100644
--- a/chrome/browser/notifications/message_center_notifications_browsertest.cc
+++ b/chrome/browser/notifications/message_center_notifications_browsertest.cc
@@ -6,7 +6,6 @@
 
 #include "base/command_line.h"
 #include "base/message_loop/message_loop.h"
-#include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -24,30 +23,31 @@
 
 class TestAddObserver : public message_center::MessageCenterObserver {
  public:
-  TestAddObserver(const std::string& id,
-                  message_center::MessageCenter* message_center)
-      : id_(id), message_center_(message_center) {
-    quit_closure_ = run_loop_.QuitClosure();
+  explicit TestAddObserver(message_center::MessageCenter* message_center)
+      : message_center_(message_center) {
     message_center_->AddObserver(this);
   }
 
   virtual ~TestAddObserver() { message_center_->RemoveObserver(this); }
 
   virtual void OnNotificationAdded(const std::string& id) OVERRIDE {
-    log_ += "_" + id;
-    if (id == id_)
-      base::MessageLoop::current()->PostTask(FROM_HERE, quit_closure_);
+    if (log_ != "")
+      log_ += "_";
+    log_ += "add-" + id;
+  }
+
+  virtual void OnNotificationUpdated(const std::string& id) OVERRIDE {
+    if (log_ != "")
+      log_ += "_";
+    log_ += "update-" + id;
   }
 
-  void Run() { run_loop_.Run(); }
   const std::string log() const { return log_; }
+  void reset_log() { log_ = ""; }
 
  private:
-  std::string id_;
   std::string log_;
   message_center::MessageCenter* message_center_;
-  base::RunLoop run_loop_;
-  base::Closure quit_closure_;
 };
 
 class MessageCenterNotificationsTest : public InProcessBrowserTest {
@@ -247,7 +247,7 @@ IN_PROC_BROWSER_TEST_F(MessageCenterNotificationsTest,
   manager()->Add(CreateRichTestNotification("n", &delegate2), profile());
 
   manager()->CancelById("n");
-  EXPECT_EQ("Display_Close_programmatically_", delegate->log());
+  EXPECT_EQ("Display_", delegate->log());
   EXPECT_EQ("Close_programmatically_", delegate2->log());
 
   delegate->Release();
@@ -270,7 +270,7 @@ IN_PROC_BROWSER_TEST_F(MessageCenterNotificationsTest,
 #endif
 
   EXPECT_TRUE(NotificationUIManager::DelegatesToMessageCenter());
-  TestAddObserver observer("n2", message_center());
+  TestAddObserver observer(message_center());
 
   TestDelegate* delegate;
   TestDelegate* delegate2;
@@ -279,15 +279,130 @@ IN_PROC_BROWSER_TEST_F(MessageCenterNotificationsTest,
   message_center()->SetMessageCenterVisible(true);
   manager()->Add(CreateTestNotification("n2", &delegate2), profile());
 
-  EXPECT_EQ("_n", observer.log());
+  EXPECT_EQ("add-n", observer.log());
 
   message_center()->SetMessageCenterVisible(false);
-  observer.Run();
 
-  EXPECT_EQ("_n_n2", observer.log());
+  EXPECT_EQ("add-n_add-n2", observer.log());
 
   delegate->Release();
   delegate2->Release();
 }
 
+// MessaceCenter-specific test.
+#if defined(RUN_MESSAGE_CENTER_TESTS)
+#define MAYBE_UpdateNonProgressNotificationWhenCenterVisible \
+    UpdateNonProgressNotificationWhenCenterVisible
+#else
+#define MAYBE_UpdateNonProgressNotificationWhenCenterVisible \
+    DISABLED_UpdateNonProgressNotificationWhenCenterVisible
+#endif
+
+IN_PROC_BROWSER_TEST_F(MessageCenterNotificationsTest,
+                       MAYBE_UpdateNonProgressNotificationWhenCenterVisible) {
+#if defined(OS_WIN) && defined(USE_ASH)
+  // Disable this test in Metro+Ash for now (http://crbug.com/262796).
+  if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kAshBrowserTests))
+    return;
+#endif
+
+  EXPECT_TRUE(NotificationUIManager::DelegatesToMessageCenter());
+  TestAddObserver observer(message_center());
+
+  TestDelegate* delegate;
+
+  // Add a non-progress notification and update it while the message center
+  // is visible.
+  Notification notification = CreateTestNotification("n", &delegate);
+  manager()->Add(notification, profile());
+  message_center()->ClickOnNotification("n");
+  message_center()->SetMessageCenterVisible(true);
+  observer.reset_log();
+  notification.set_title(ASCIIToUTF16("title2"));
+  manager()->Update(notification, profile());
+
+  // Expect that the notification update is not done.
+  EXPECT_EQ("", observer.log());
+
+  delegate->Release();
+}
+
+// MessaceCenter-specific test.
+#if defined(RUN_MESSAGE_CENTER_TESTS)
+#define MAYBE_UpdateNonProgressToProgressNotificationWhenCenterVisible \
+    UpdateNonProgressToProgressNotificationWhenCenterVisible
+#else
+#define MAYBE_UpdateNonProgressToProgressNotificationWhenCenterVisible \
+    DISABLED_UpdateNonProgressToProgressNotificationWhenCenterVisible
+#endif
+
+IN_PROC_BROWSER_TEST_F(
+    MessageCenterNotificationsTest,
+    MAYBE_UpdateNonProgressToProgressNotificationWhenCenterVisible) {
+#if defined(OS_WIN) && defined(USE_ASH)
+  // Disable this test in Metro+Ash for now (http://crbug.com/262796).
+  if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kAshBrowserTests))
+    return;
+#endif
+
+  EXPECT_TRUE(NotificationUIManager::DelegatesToMessageCenter());
+  TestAddObserver observer(message_center());
+
+  TestDelegate* delegate;
+
+  // Add a non-progress notification and change the type to progress while the
+  // message center is visible.
+  Notification notification = CreateTestNotification("n", &delegate);
+  manager()->Add(notification, profile());
+  message_center()->ClickOnNotification("n");
+  message_center()->SetMessageCenterVisible(true);
+  observer.reset_log();
+  notification.set_type(message_center::NOTIFICATION_TYPE_PROGRESS);
+  manager()->Update(notification, profile());
+
+  // Expect that the notification update is not done.
+  EXPECT_EQ("", observer.log());
+
+  delegate->Release();
+}
+
+// MessaceCenter-specific test.
+#if defined(RUN_MESSAGE_CENTER_TESTS)
+#define MAYBE_UpdateProgressNotificationWhenCenterVisible \
+    UpdateProgressNotificationWhenCenterVisible
+#else
+#define MAYBE_UpdateProgressNotificationWhenCenterVisible \
+    DISABLED_UpdateProgressNotificationWhenCenterVisible
+#endif
+
+IN_PROC_BROWSER_TEST_F(MessageCenterNotificationsTest,
+                       MAYBE_UpdateProgressNotificationWhenCenterVisible) {
+#if defined(OS_WIN) && defined(USE_ASH)
+  // Disable this test in Metro+Ash for now (http://crbug.com/262796).
+  if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kAshBrowserTests))
+    return;
+#endif
+
+  EXPECT_TRUE(NotificationUIManager::DelegatesToMessageCenter());
+  TestAddObserver observer(message_center());
+
+  TestDelegate* delegate;
+
+  // Add a progress notification and update it while the message center
+  // is visible.
+  Notification notification = CreateTestNotification("n", &delegate);
+  notification.set_type(message_center::NOTIFICATION_TYPE_PROGRESS);
+  manager()->Add(notification, profile());
+  message_center()->ClickOnNotification("n");
+  message_center()->SetMessageCenterVisible(true);
+  observer.reset_log();
+  notification.set_progress(50);
+  manager()->Update(notification, profile());
+
+  // Expect that the progress notification update is performed.
+  EXPECT_EQ("update-n", observer.log());
+
+  delegate->Release();
+}
+
 #endif  // !defined(OS_MACOSX)
diff --git a/chrome/browser/notifications/notification_ui_manager.h b/chrome/browser/notifications/notification_ui_manager.h
index a86f1d2..5018e0e 100644
--- a/chrome/browser/notifications/notification_ui_manager.h
+++ b/chrome/browser/notifications/notification_ui_manager.h
@@ -26,8 +26,11 @@ class NotificationUIManager {
   static NotificationUIManager* Create(PrefService* local_state);
 
   // Adds a notification to be displayed. Virtual for unit test override.
-  virtual void Add(const Notification& notification,
-                   Profile* profile) = 0;
+  virtual void Add(const Notification& notification, Profile* profile) = 0;
+
+  // Updates an existing notification. If |update_progress_only|, assume
+  // only message and progress properties are updated.
+  virtual bool Update(const Notification& notification, Profile* profile) = 0;
 
   // Returns the pointer to a notification if it match the supplied ID, either
   // currently displayed or in the queue.
diff --git a/chrome/browser/notifications/notification_ui_manager_impl.cc b/chrome/browser/notifications/notification_ui_manager_impl.cc
index d2b08d0..258c84b 100644
--- a/chrome/browser/notifications/notification_ui_manager_impl.cc
+++ b/chrome/browser/notifications/notification_ui_manager_impl.cc
@@ -63,7 +63,7 @@ NotificationUIManagerImpl::~NotificationUIManagerImpl() {
 
 void NotificationUIManagerImpl::Add(const Notification& notification,
                                     Profile* profile) {
-  if (TryReplacement(notification, profile)) {
+  if (Update(notification, profile)) {
     return;
   }
 
@@ -74,6 +74,30 @@ void NotificationUIManagerImpl::Add(const Notification& notification,
   CheckAndShowNotifications();
 }
 
+bool NotificationUIManagerImpl::Update(const Notification& notification,
+                                       Profile* profile) {
+  const GURL& origin = notification.origin_url();
+  const string16& replace_id = notification.replace_id();
+
+  if (replace_id.empty())
+    return false;
+
+  // First check the queue of pending notifications for replacement.
+  // Then check the list of notifications already being shown.
+  for (NotificationDeque::const_iterator iter = show_queue_.begin();
+       iter != show_queue_.end(); ++iter) {
+    if (profile == (*iter)->profile() &&
+        origin == (*iter)->notification().origin_url() &&
+        replace_id == (*iter)->notification().replace_id()) {
+      (*iter)->Replace(notification);
+      return true;
+    }
+  }
+
+  // Give the subclass the opportunity to update existing notification.
+  return UpdateNotification(notification, profile);
+}
+
 const Notification* NotificationUIManagerImpl::FindById(
     const std::string& id) const {
   for (NotificationDeque::const_iterator iter = show_queue_.begin();
@@ -189,30 +213,6 @@ void NotificationUIManagerImpl::ShowNotifications() {
   }
 }
 
-bool NotificationUIManagerImpl::TryReplacement(
-    const Notification& notification, Profile* profile) {
-  const GURL& origin = notification.origin_url();
-  const string16& replace_id = notification.replace_id();
-
-  if (replace_id.empty())
-    return false;
-
-  // First check the queue of pending notifications for replacement.
-  // Then check the list of notifications already being shown.
-  for (NotificationDeque::const_iterator iter = show_queue_.begin();
-       iter != show_queue_.end(); ++iter) {
-    if (profile == (*iter)->profile() &&
-        origin == (*iter)->notification().origin_url() &&
-        replace_id == (*iter)->notification().replace_id()) {
-      (*iter)->Replace(notification);
-      return true;
-    }
-  }
-
-  // Give the subclass the opportunity to update existing notification.
-  return UpdateNotification(notification, profile);
-}
-
 void NotificationUIManagerImpl::GetQueuedNotificationsForTesting(
     std::vector<const Notification*>* notifications) {
   for (NotificationDeque::const_iterator iter = show_queue_.begin();
diff --git a/chrome/browser/notifications/notification_ui_manager_impl.h b/chrome/browser/notifications/notification_ui_manager_impl.h
index 5ced721..a56d439 100644
--- a/chrome/browser/notifications/notification_ui_manager_impl.h
+++ b/chrome/browser/notifications/notification_ui_manager_impl.h
@@ -34,6 +34,8 @@ class NotificationUIManagerImpl
   // NotificationUIManager:
   virtual void Add(const Notification& notification,
                    Profile* profile) OVERRIDE;
+  virtual bool Update(const Notification& notification,
+                      Profile* profile) OVERRIDE;
   virtual const Notification* FindById(
       const std::string& notification_id) const OVERRIDE;
   virtual bool CancelById(const std::string& notification_id) OVERRIDE;
@@ -55,17 +57,17 @@ class NotificationUIManagerImpl
   virtual bool ShowNotification(const Notification& notification,
                                 Profile* profile) = 0;
 
- // Replace an existing notification of the same id with this one if applicable;
- // subclass returns 'true' if the replacement happened.
- virtual bool UpdateNotification(const Notification& notification,
-                                 Profile* profile) = 0;
+  // Replace an existing notification of the same id with this one if
+  // applicable; subclass returns 'true' if the replacement happened.
+  virtual bool UpdateNotification(const Notification& notification,
+                                  Profile* profile) = 0;
 
- // Attempts to display notifications from the show_queue. Invoked by subclasses
- // if they previously returned 'false' from ShowNotifications, which may happen
- // when there is no room to show another notification. When room appears, the
- // subclass should call this method to cause an attempt to show more
- // notifications from the waiting queue.
- void CheckAndShowNotifications();
+  // Attempts to display notifications from the show_queue. Invoked by subclass
+  // if it previously returned 'false' from ShowNotifications, which may happen
+  // when there is no room to show another notification. When room appears, the
+  // subclass should call this method to cause an attempt to show more
+  // notifications from the waiting queue.
+  void CheckAndShowNotifications();
 
  private:
   // content::NotificationObserver override.
@@ -76,10 +78,6 @@ class NotificationUIManagerImpl
   // Attempts to display notifications from the show_queue.
   void ShowNotifications();
 
-  // Replace an existing notification with this one if applicable;
-  // returns true if the replacement happened.
-  bool TryReplacement(const Notification& notification, Profile* profile);
-
   // Checks the user state to decide if we want to show the notification.
   void CheckUserState();
 
diff --git a/chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc b/chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc
index 38fbb80..17fe01f 100644
--- a/chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc
+++ b/chrome/browser/notifications/sync_notifier/chrome_notifier_service_unittest.cc
@@ -64,6 +64,14 @@ class StubNotificationUIManager : public NotificationUIManager {
     profile_ = profile;
   }
 
+  virtual bool Update(const Notification& notification, Profile* profile)
+      OVERRIDE {
+    // Make a deep copy of the notification that we can inspect.
+    notification_ = notification;
+    profile_ = profile;
+    return true;
+  }
+
   // Returns true if any notifications match the supplied ID, either currently
   // displayed or in the queue.
   virtual const Notification* FindById(const std::string& id) const OVERRIDE {
diff --git a/chrome/browser/notifications/sync_notifier/synced_notification_unittest.cc b/chrome/browser/notifications/sync_notifier/synced_notification_unittest.cc
index 38794aa..84392da 100644
--- a/chrome/browser/notifications/sync_notifier/synced_notification_unittest.cc
+++ b/chrome/browser/notifications/sync_notifier/synced_notification_unittest.cc
@@ -56,6 +56,14 @@ class StubNotificationUIManager : public NotificationUIManager {
     profile_ = profile;
   }
 
+  virtual bool Update(const Notification& notification, Profile* profile)
+      OVERRIDE {
+    // Make a deep copy of the notification that we can inspect.
+    notification_ = notification;
+    profile_ = profile;
+    return true;
+  }
+
   // Returns true if any notifications match the supplied ID, either currently
   // displayed or in the queue.
   virtual const Notification* FindById(const std::string& id) const OVERRIDE {