Queue notification center deletes while visible to the user.

This extends the notification center queueing system to support deletes.
Formerly they would just pass through to the views, causing re-rendering
artifacts.

R=jianli@chromium.org
TBR=oshima@chromium.org,stevenjb@chromium.org,benwells@chromium.org
BUG=267066

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

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

11 files changed, 331 insertions, 147 deletions

diff --git a/ui/message_center/cocoa/tray_view_controller.mm b/ui/message_center/cocoa/tray_view_controller.mm
index 17c24f7..6865562 100644
--- a/ui/message_center/cocoa/tray_view_controller.mm
+++ b/ui/message_center/cocoa/tray_view_controller.mm
@@ -188,7 +188,7 @@ const CGFloat kTrayBottomMargin = 75;
   // origin is in the lower-left. Remove from |notificationsMap_| all the
   // ones still in the updated model, so that those that should be removed
   // will remain in the map.
-  const auto& modelNotifications = messageCenter_->GetNotifications();
+  const auto& modelNotifications = messageCenter_->GetVisibleNotifications();
   for (auto it = modelNotifications.rbegin();
        it != modelNotifications.rend();
        ++it) {
diff --git a/ui/message_center/fake_message_center.cc b/ui/message_center/fake_message_center.cc
index eaa4c42..98d9035 100644
--- a/ui/message_center/fake_message_center.cc
+++ b/ui/message_center/fake_message_center.cc
@@ -50,7 +50,8 @@ bool FakeMessageCenter::HasClickedListener(const std::string& id) {
   return false;
 }
 
-const NotificationList::Notifications& FakeMessageCenter::GetNotifications() {
+const NotificationList::Notifications&
+FakeMessageCenter::GetVisibleNotifications() {
   return empty_notifications_;
 }
 
diff --git a/ui/message_center/fake_message_center.h b/ui/message_center/fake_message_center.h
index e7fbddc..08f94ea 100644
--- a/ui/message_center/fake_message_center.h
+++ b/ui/message_center/fake_message_center.h
@@ -29,7 +29,8 @@ class FakeMessageCenter : public MessageCenter {
   virtual bool HasNotification(const std::string& id) OVERRIDE;
   virtual bool IsQuietMode() const OVERRIDE;
   virtual bool HasClickedListener(const std::string& id) OVERRIDE;
-  virtual const NotificationList::Notifications& GetNotifications() OVERRIDE;
+  virtual const NotificationList::Notifications& GetVisibleNotifications()
+      OVERRIDE;
   virtual NotificationList::PopupNotifications GetPopupNotifications() OVERRIDE;
   virtual void AddNotification(scoped_ptr<Notification> notification) OVERRIDE;
   virtual void UpdateNotification(const std::string& old_id,
diff --git a/ui/message_center/message_center.h b/ui/message_center/message_center.h
index ebe90d2..061dd44 100644
--- a/ui/message_center/message_center.h
+++ b/ui/message_center/message_center.h
@@ -58,9 +58,14 @@ class MESSAGE_CENTER_EXPORT MessageCenter {
   virtual bool IsQuietMode() const = 0;
   virtual bool HasClickedListener(const std::string& id) = 0;
 
-  // Getters of the current notifications.
-  virtual const NotificationList::Notifications& GetNotifications() = 0;
-  // Gets all notifications being shown as popups.
+  // Gets all notifications to be shown to the user in the message center.  Note
+  // that queued changes due to the message center being open are not reflected
+  // in this list.
+  virtual const NotificationList::Notifications& GetVisibleNotifications() = 0;
+
+  // Gets all notifications being shown as popups.  This should not be affected
+  // by the change queue since notifications are not held up while the state is
+  // VISIBILITY_TRANSIENT or VISIBILITY_SETTINGS.
   virtual NotificationList::PopupNotifications GetPopupNotifications() = 0;
 
   // Management of NotificaitonBlockers.
diff --git a/ui/message_center/message_center_impl.cc b/ui/message_center/message_center_impl.cc
index 99c00e8..d07695f 100644
--- a/ui/message_center/message_center_impl.cc
+++ b/ui/message_center/message_center_impl.cc
@@ -6,6 +6,7 @@
 
 #include <algorithm>
 
+#include "base/memory/scoped_vector.h"
 #include "base/observer_list.h"
 #include "ui/message_center/message_center_style.h"
 #include "ui/message_center/message_center_types.h"
@@ -30,34 +31,187 @@ base::TimeDelta GetTimeoutForPriority(int priority) {
 namespace message_center {
 namespace internal {
 
+// ChangeQueue keeps track of all the changes that we need to make to the
+// notification list once the visibility is set to VISIBILITY_TRANSIENT.
+class ChangeQueue {
+ public:
+  enum ChangeType {
+    CHANGE_TYPE_ADD = 0,
+    CHANGE_TYPE_UPDATE,
+    CHANGE_TYPE_DELETE
+  };
+
+  // Change represents an operation made on a notification.  Since it contains
+  // the final state of the notification, we only keep the last change for a
+  // particular notification that is in the notification list around.  There are
+  // two ids; |id_| is the newest notification id that has been assigned by an
+  // update, and |notification_list_id_| is the id of the notification it should
+  // be updating as it exists in the notification list.
+  class Change {
+   public:
+    Change(ChangeType type,
+           const std::string& id,
+           scoped_ptr<Notification> notification);
+    ~Change();
+
+    // Used to transfer ownership of the contained notification.
+    scoped_ptr<Notification> PassNotification();
+
+    const std::string& id() const { return id_; }
+    ChangeType type() const { return type_; }
+    bool by_user() const { return by_user_; }
+    void set_by_user(bool by_user) { by_user_ = by_user; }
+    const std::string& notification_list_id() const {
+      return notification_list_id_;
+    }
+    void set_notification_list_id(const std::string& id) {
+      notification_list_id_ = id;
+    }
+
+   private:
+    const ChangeType type_;
+    const std::string id_;
+    std::string notification_list_id_;
+    bool by_user_;
+    scoped_ptr<Notification> notification_;
+
+    DISALLOW_COPY_AND_ASSIGN(Change);
+  };
+
+  ChangeQueue();
+  ~ChangeQueue();
+
+  // Called when the message center has appropriate visibility.  Modifies
+  // |message_center| but does not retain it.  This also causes the queue to
+  // empty itself.
+  void ApplyChanges(MessageCenter* message_center);
+
+  // Causes a TYPE_ADD change to be added to the queue.
+  void AddNotification(scoped_ptr<Notification> notification);
+
+  // Causes a TYPE_UPDATE change to be added to the queue.
+  void UpdateNotification(const std::string& old_id,
+                          scoped_ptr<Notification> notification);
+
+  // Causes a TYPE_DELETE change to be added to the queue.
+  void EraseNotification(const std::string& id, bool by_user);
+
+  // Returns whether the queue matches an id.  The id given will be matched
+  // against the ID of all changes post-update, not the id of the notification
+  // as it stands in the notification list.
+  bool Has(const std::string& id) const;
+
+ private:
+  void Replace(const std::string& id, scoped_ptr<Change> change);
+
+  ScopedVector<Change> changes_;
+};
+
 ////////////////////////////////////////////////////////////////////////////////
-// NotificationQueueItem
+// ChangeFinder
 
-struct NotificationQueueItem {
- public:
-  Notification* notification;
-  bool is_update;
-  std::string pre_update_id;
+struct ChangeFinder {
+  explicit ChangeFinder(const std::string& id) : id(id) {}
+  bool operator()(ChangeQueue::Change* change) { return change->id() == id; }
+
+  std::string id;
 };
 
-void DeleteQueueNotification(NotificationQueueItem item) {
-  if (item.notification)
-    delete item.notification;
+////////////////////////////////////////////////////////////////////////////////
+// ChangeQueue::Change
+
+ChangeQueue::Change::Change(ChangeType type,
+                            const std::string& id,
+                            scoped_ptr<Notification> notification)
+    : type_(type),
+      id_(id),
+      notification_list_id_(id),
+      by_user_(false),
+      notification_(notification.Pass()) {
+  DCHECK(!id.empty() &&
+         (type != CHANGE_TYPE_DELETE || notification_.get() == NULL));
 }
 
+ChangeQueue::Change::~Change() {}
+
+scoped_ptr<Notification> ChangeQueue::Change::PassNotification() {
+  return notification_.Pass();
+}
 
 ////////////////////////////////////////////////////////////////////////////////
-// NotificationFinder
+// ChangeQueue
+
+ChangeQueue::ChangeQueue() {}
+
+ChangeQueue::~ChangeQueue() {}
+
+void ChangeQueue::ApplyChanges(MessageCenter* message_center) {
+  ScopedVector<Change>::const_iterator iter = changes_.begin();
+  for (; iter != changes_.end(); ++iter) {
+    Change* change = *iter;
+    // |message_center| is taking ownership of each element here.
+    switch (change->type()) {
+      case CHANGE_TYPE_ADD:
+        message_center->AddNotification(change->PassNotification());
+        break;
+      case CHANGE_TYPE_UPDATE:
+        message_center->UpdateNotification(change->notification_list_id(),
+                                           change->PassNotification());
+        break;
+      case CHANGE_TYPE_DELETE:
+        message_center->RemoveNotification(change->notification_list_id(),
+                                           change->by_user());
+        break;
+      default:
+        NOTREACHED();
+    }
+  }
 
-struct NotificationFinder {
-  explicit NotificationFinder(const std::string& id) : id(id) {}
-  bool operator()(const NotificationQueueItem& item) {
-    DCHECK(item.notification);
-    return item.notification->id() == id;
+  changes_.clear();
+}
+
+void ChangeQueue::AddNotification(scoped_ptr<Notification> notification) {
+  std::string id = notification->id();
+  scoped_ptr<Change> change(
+      new Change(CHANGE_TYPE_ADD, id, notification.Pass()));
+  Replace(id, change.Pass());
+}
+
+void ChangeQueue::UpdateNotification(const std::string& old_id,
+                                     scoped_ptr<Notification> notification) {
+  std::string new_id = notification->id();
+  scoped_ptr<Change> change(
+      new Change(CHANGE_TYPE_UPDATE, new_id, notification.Pass()));
+  Replace(old_id, change.Pass());
+}
+
+void ChangeQueue::EraseNotification(const std::string& id, bool by_user) {
+  scoped_ptr<Change> change(
+      new Change(CHANGE_TYPE_DELETE, id, scoped_ptr<Notification>()));
+  change->set_by_user(by_user);
+  Replace(id, change.Pass());
+}
+
+bool ChangeQueue::Has(const std::string& id) const {
+  ScopedVector<Change>::const_iterator iter =
+      std::find_if(changes_.begin(), changes_.end(), ChangeFinder(id));
+  return iter != changes_.end();
+}
+
+void ChangeQueue::Replace(const std::string& changed_id,
+                          scoped_ptr<Change> new_change) {
+  ScopedVector<Change>::iterator iter =
+      std::find_if(changes_.begin(), changes_.end(), ChangeFinder(changed_id));
+  if (iter != changes_.end()) {
+    Change* old_change = *iter;
+    new_change->set_notification_list_id(old_change->notification_list_id());
+    changes_.erase(iter);
+  } else {
+    new_change->set_notification_list_id(changed_id);
   }
 
-  std::string id;
-};
+  changes_.push_back(new_change.release());
+}
 
 ////////////////////////////////////////////////////////////////////////////////
 // PopupTimer
@@ -229,15 +383,10 @@ MessageCenterImpl::MessageCenterImpl()
       popup_timers_controller_(new internal::PopupTimersController(this)),
       settings_provider_(NULL) {
   notification_list_.reset(new NotificationList());
+  notification_queue_.reset(new internal::ChangeQueue());
 }
 
-MessageCenterImpl::~MessageCenterImpl() {
-  notification_list_.reset();
-  for_each(notification_queue_.begin(),
-           notification_queue_.end(),
-           internal::DeleteQueueNotification);
-  notification_queue_.clear();
-}
+MessageCenterImpl::~MessageCenterImpl() {}
 
 void MessageCenterImpl::AddObserver(MessageCenterObserver* observer) {
   observer_list_.AddObserver(observer);
@@ -289,23 +438,9 @@ void MessageCenterImpl::SetVisibility(Visibility visibility) {
         MessageCenterObserver, observer_list_, OnNotificationUpdated(*iter));
   }
 
-  if (visibility == VISIBILITY_TRANSIENT) {
-    for (std::list<internal::NotificationQueueItem>::const_iterator iter =
-             notification_queue_.begin();
-         iter != notification_queue_.end();
-         ++iter) {
-      // Warning: After this line, the queue will no longer own the
-      // notifications contained.
-      if ((*iter).is_update)
-        UpdateNotification((*iter).pre_update_id,
-                           scoped_ptr<Notification>((*iter).notification));
-      else
-        AddNotification(scoped_ptr<Notification>((*iter).notification));
-    }
-    // This does not delete the internal notification pointers since they are
-    // now owned by the notification list.
-    notification_queue_.clear();
-  }
+  if (visibility == VISIBILITY_TRANSIENT)
+    notification_queue_->ApplyChanges(this);
+
   FOR_EACH_OBSERVER(MessageCenterObserver,
                     observer_list_,
                     OnCenterVisibilityChanged(visibility));
@@ -341,7 +476,8 @@ bool MessageCenterImpl::HasClickedListener(const std::string& id) {
   return delegate && delegate->HasClickedListener();
 }
 
-const NotificationList::Notifications& MessageCenterImpl::GetNotifications() {
+const NotificationList::Notifications&
+MessageCenterImpl::GetVisibleNotifications() {
   return notification_list_->GetNotifications();
 }
 
@@ -359,20 +495,7 @@ void MessageCenterImpl::AddNotification(scoped_ptr<Notification> notification) {
     blockers_[i]->CheckState();
 
   if (notification_list_->is_message_center_visible()) {
-    std::list<internal::NotificationQueueItem>::iterator iter = std::find_if(
-        notification_queue_.begin(),
-        notification_queue_.end(),
-        internal::NotificationFinder(notification->id()));
-    if (iter != notification_queue_.end()) {
-      internal::DeleteQueueNotification(*iter);
-      notification_queue_.erase(iter);
-    }
-    internal::NotificationQueueItem item = {
-      notification.release(),
-      false,
-      std::string()
-    };
-    notification_queue_.push_back(item);
+    notification_queue_->AddNotification(notification.Pass());
     return;
   }
 
@@ -398,33 +521,27 @@ void MessageCenterImpl::UpdateNotification(
   for (size_t i = 0; i < blockers_.size(); ++i)
     blockers_[i]->CheckState();
 
-  // We will allow notifications that are progress types (and stay progress
-  // types) to be updated even if the message center is open.
-  bool update_keeps_progress_type =
-      new_notification->type() == NOTIFICATION_TYPE_PROGRESS;
-  if (!notification_list_->HasNotificationOfType(old_id,
-                                                 NOTIFICATION_TYPE_PROGRESS)) {
-    update_keeps_progress_type = false;
-  }
-
-  // Updates are allowed only for progress notifications.
-  if (notification_list_->is_message_center_visible() &&
-      !update_keeps_progress_type) {
-    std::list<internal::NotificationQueueItem>::iterator iter = std::find_if(
-        notification_queue_.begin(),
-        notification_queue_.end(),
-        internal::NotificationFinder(old_id));
-    if (iter != notification_queue_.end()) {
-      internal::DeleteQueueNotification(*iter);
-      notification_queue_.erase(iter);
+  if (notification_list_->is_message_center_visible()) {
+    // We will allow notifications that are progress types (and stay progress
+    // types) to be updated even if the message center is open.  There are 3
+    // requirements here:
+    //  * Notification of type PROGRESS exists with same ID in the center
+    //  * There are no queued updates for this notification (they imply a change
+    //    that violates the PROGRESS invariant
+    //  * The new notification is type PROGRESS.
+    // TODO(dewittj): Ensure this works when the ID is changed by the caller.
+    // This shouldn't be an issue in practice since only W3C notifications
+    // change the ID on update, and they don't have progress type notifications.
+    bool update_keeps_progress_type =
+        new_notification->type() == NOTIFICATION_TYPE_PROGRESS &&
+        !notification_queue_->Has(old_id) &&
+        notification_list_->HasNotificationOfType(old_id,
+                                                  NOTIFICATION_TYPE_PROGRESS);
+    if (!update_keeps_progress_type) {
+      // Updates are allowed only for progress notifications.
+      notification_queue_->UpdateNotification(old_id, new_notification.Pass());
+      return;
     }
-    internal::NotificationQueueItem item = {
-      new_notification.release(),
-      true,
-      old_id
-    };
-    notification_queue_.push_back(item);
-    return;
   }
 
   std::string new_id = new_notification->id();
@@ -443,13 +560,9 @@ void MessageCenterImpl::UpdateNotification(
 
 void MessageCenterImpl::RemoveNotification(const std::string& id,
                                            bool by_user) {
-  std::list<internal::NotificationQueueItem>::iterator iter = std::find_if(
-      notification_queue_.begin(),
-      notification_queue_.end(),
-      internal::NotificationFinder(id));
-  if (iter != notification_queue_.end()) {
-    DeleteQueueNotification(*iter);
-    notification_queue_.erase(iter);
+  if (!by_user && notification_list_->is_message_center_visible()) {
+    notification_queue_->EraseNotification(id, by_user);
+    return;
   }
 
   if (!HasNotification(id))
diff --git a/ui/message_center/message_center_impl.h b/ui/message_center/message_center_impl.h
index b57f532..6d19b90 100644
--- a/ui/message_center/message_center_impl.h
+++ b/ui/message_center/message_center_impl.h
@@ -8,6 +8,7 @@
 #include <string>
 #include <vector>
 
+#include "base/memory/scoped_vector.h"
 #include "base/memory/weak_ptr.h"
 #include "base/stl_util.h"
 #include "base/time/time.h"
@@ -22,7 +23,7 @@ class NotificationDelegate;
 class MessageCenterImpl;
 
 namespace internal {
-struct NotificationQueueItem;
+class ChangeQueue;
 class PopupTimersController;
 
 // A class that manages timeout behavior for notification popups.  One instance
@@ -152,7 +153,8 @@ class MessageCenterImpl : public MessageCenter,
   virtual bool HasNotification(const std::string& id) OVERRIDE;
   virtual bool IsQuietMode() const OVERRIDE;
   virtual bool HasClickedListener(const std::string& id) OVERRIDE;
-  virtual const NotificationList::Notifications& GetNotifications() OVERRIDE;
+  virtual const NotificationList::Notifications& GetVisibleNotifications()
+      OVERRIDE;
   virtual NotificationList::PopupNotifications GetPopupNotifications() OVERRIDE;
   virtual void AddNotification(scoped_ptr<Notification> notification) OVERRIDE;
   virtual void UpdateNotification(const std::string& old_id,
@@ -199,9 +201,9 @@ class MessageCenterImpl : public MessageCenter,
   NotifierSettingsProvider* settings_provider_;
   std::vector<NotificationBlocker*> blockers_;
 
-  // queue for the notifications to delay the addition/updates when the message
+  // Queue for the notifications to delay the addition/updates when the message
   // center is visible.
-  std::list<internal::NotificationQueueItem> notification_queue_;
+  scoped_ptr<internal::ChangeQueue> notification_queue_;
 
   DISALLOW_COPY_AND_ASSIGN(MessageCenterImpl);
 };
diff --git a/ui/message_center/message_center_impl_unittest.cc b/ui/message_center/message_center_impl_unittest.cc
index e6de02b..d77e4ef 100644
--- a/ui/message_center/message_center_impl_unittest.cc
+++ b/ui/message_center/message_center_impl_unittest.cc
@@ -41,6 +41,24 @@ class MessageCenterImplTest : public testing::Test,
   base::RunLoop* run_loop() const { return run_loop_.get(); }
   base::Closure closure() const { return closure_; }
 
+ protected:
+  Notification* CreateSimpleNotification(const std::string& id) {
+    return CreateNotification(id, NOTIFICATION_TYPE_SIMPLE);
+  }
+
+  Notification* CreateNotification(const std::string& id,
+                                   message_center::NotificationType type) {
+    return new Notification(type,
+                            id,
+                            UTF8ToUTF16("title"),
+                            UTF8ToUTF16(id),
+                            gfx::Image() /* icon */,
+                            base::string16() /* display_source */,
+                            NotifierId(NotifierId::APPLICATION, "app1"),
+                            RichNotificationData(),
+                            NULL);
+  }
+
  private:
   MessageCenter* message_center_;
   scoped_ptr<base::MessageLoop> loop_;
@@ -123,7 +141,6 @@ class MockPopupTimersController : public PopupTimersController {
   virtual ~MockPopupTimersController() {}
 
   virtual void TimerFinished(const std::string& id) OVERRIDE {
-    LOG(INFO) << "In timer finished for id " << id;
     base::MessageLoop::current()->PostTask(FROM_HERE, quit_closure_);
     timer_finished_ = true;
     last_id_ = id;
@@ -294,35 +311,35 @@ TEST_F(MessageCenterImplTest, NotificationBlocker) {
       RichNotificationData(),
       NULL)));
   EXPECT_EQ(2u, message_center()->GetPopupNotifications().size());
-  EXPECT_EQ(2u, message_center()->GetNotifications().size());
+  EXPECT_EQ(2u, message_center()->GetVisibleNotifications().size());
 
   // Block all notifications. All popups are gone and message center should be
   // hidden.
   blocker1.SetNotificationsEnabled(false);
   EXPECT_TRUE(message_center()->GetPopupNotifications().empty());
-  EXPECT_EQ(2u, message_center()->GetNotifications().size());
+  EXPECT_EQ(2u, message_center()->GetVisibleNotifications().size());
 
   // Updates |blocker2| state, which doesn't affect the global state.
   blocker2.SetNotificationsEnabled(false);
   EXPECT_TRUE(message_center()->GetPopupNotifications().empty());
-  EXPECT_EQ(2u, message_center()->GetNotifications().size());
+  EXPECT_EQ(2u, message_center()->GetVisibleNotifications().size());
 
   blocker2.SetNotificationsEnabled(true);
   EXPECT_TRUE(message_center()->GetPopupNotifications().empty());
-  EXPECT_EQ(2u, message_center()->GetNotifications().size());
+  EXPECT_EQ(2u, message_center()->GetVisibleNotifications().size());
 
   // If |blocker2| blocks, then unblocking blocker1 doesn't change the global
   // state.
   blocker2.SetNotificationsEnabled(false);
   blocker1.SetNotificationsEnabled(true);
   EXPECT_TRUE(message_center()->GetPopupNotifications().empty());
-  EXPECT_EQ(2u, message_center()->GetNotifications().size());
+  EXPECT_EQ(2u, message_center()->GetVisibleNotifications().size());
 
   // Unblock both blockers, which recovers the global state, but the popups
   // aren't shown.
   blocker2.SetNotificationsEnabled(true);
   EXPECT_TRUE(message_center()->GetPopupNotifications().empty());
-  EXPECT_EQ(2u, message_center()->GetNotifications().size());
+  EXPECT_EQ(2u, message_center()->GetVisibleNotifications().size());
 }
 
 TEST_F(MessageCenterImplTest, NotificationsDuringBlocked) {
@@ -340,7 +357,7 @@ TEST_F(MessageCenterImplTest, NotificationsDuringBlocked) {
       RichNotificationData(),
       NULL)));
   EXPECT_EQ(1u, message_center()->GetPopupNotifications().size());
-  EXPECT_EQ(1u, message_center()->GetNotifications().size());
+  EXPECT_EQ(1u, message_center()->GetVisibleNotifications().size());
 
   // Create a notification during blocked. Still no popups.
   blocker.SetNotificationsEnabled(false);
@@ -355,7 +372,7 @@ TEST_F(MessageCenterImplTest, NotificationsDuringBlocked) {
       RichNotificationData(),
       NULL)));
   EXPECT_TRUE(message_center()->GetPopupNotifications().empty());
-  EXPECT_EQ(2u, message_center()->GetNotifications().size());
+  EXPECT_EQ(2u, message_center()->GetVisibleNotifications().size());
 
   // Unblock notifications, the id1 should appear as a popup.
   blocker.SetNotificationsEnabled(true);
@@ -363,7 +380,7 @@ TEST_F(MessageCenterImplTest, NotificationsDuringBlocked) {
       message_center()->GetPopupNotifications();
   EXPECT_EQ(1u, popups.size());
   EXPECT_TRUE(PopupNotificationsContain(popups, "id2"));
-  EXPECT_EQ(2u, message_center()->GetNotifications().size());
+  EXPECT_EQ(2u, message_center()->GetVisibleNotifications().size());
 }
 
 // Similar to other blocker cases but this test case allows |notifier_id2| even
@@ -400,7 +417,7 @@ TEST_F(MessageCenterImplTest, NotificationBlockerAllowsPopups) {
       message_center()->GetPopupNotifications();
   EXPECT_EQ(1u, popups.size());
   EXPECT_TRUE(PopupNotificationsContain(popups, "id2"));
-  EXPECT_EQ(2u, message_center()->GetNotifications().size());
+  EXPECT_EQ(2u, message_center()->GetVisibleNotifications().size());
 
   message_center()->AddNotification(scoped_ptr<Notification>(new Notification(
       NOTIFICATION_TYPE_SIMPLE,
@@ -426,7 +443,7 @@ TEST_F(MessageCenterImplTest, NotificationBlockerAllowsPopups) {
   EXPECT_EQ(2u, popups.size());
   EXPECT_TRUE(PopupNotificationsContain(popups, "id2"));
   EXPECT_TRUE(PopupNotificationsContain(popups, "id4"));
-  EXPECT_EQ(4u, message_center()->GetNotifications().size());
+  EXPECT_EQ(4u, message_center()->GetVisibleNotifications().size());
 
   blocker.SetNotificationsEnabled(true);
   popups = message_center()->GetPopupNotifications();
@@ -434,7 +451,7 @@ TEST_F(MessageCenterImplTest, NotificationBlockerAllowsPopups) {
   EXPECT_TRUE(PopupNotificationsContain(popups, "id2"));
   EXPECT_TRUE(PopupNotificationsContain(popups, "id3"));
   EXPECT_TRUE(PopupNotificationsContain(popups, "id4"));
-  EXPECT_EQ(4u, message_center()->GetNotifications().size());
+  EXPECT_EQ(4u, message_center()->GetVisibleNotifications().size());
 }
 
 TEST_F(MessageCenterImplTest, QueueUpdatesWithCenterVisible) {
@@ -442,49 +459,91 @@ TEST_F(MessageCenterImplTest, QueueUpdatesWithCenterVisible) {
   std::string id2("id2");
   NotifierId notifier_id1(NotifierId::APPLICATION, "app1");
 
-  scoped_ptr<Notification> notification(new Notification(
-      NOTIFICATION_TYPE_SIMPLE,
-      id,
-      UTF8ToUTF16("title"),
-      UTF8ToUTF16("message"),
-      gfx::Image() /* icon */,
-      base::string16() /* display_source */,
-      notifier_id1,
-      RichNotificationData(),
-      NULL));
-
+  // First, add and update a notification to ensure updates happen
+  // normally.
+  scoped_ptr<Notification> notification(CreateSimpleNotification(id));
   message_center()->AddNotification(notification.Pass());
-  notification.reset(new Notification(
-      NOTIFICATION_TYPE_MULTIPLE,
-      id2,
-      UTF8ToUTF16("title"),
-      UTF8ToUTF16("message"),
-      gfx::Image() /* icon */,
-      base::string16() /* display_source */,
-      notifier_id1,
-      RichNotificationData(),
-      NULL));
+  notification.reset(CreateSimpleNotification(id2));
   message_center()->UpdateNotification(id, notification.Pass());
   EXPECT_TRUE(message_center()->HasNotification(id2));
   EXPECT_FALSE(message_center()->HasNotification(id));
+
+  // Then open the message center.
   message_center()->SetVisibility(VISIBILITY_MESSAGE_CENTER);
-  notification.reset(new Notification(
-      NOTIFICATION_TYPE_MULTIPLE,
-      id,
-      UTF8ToUTF16("title"),
-      UTF8ToUTF16("message"),
-      gfx::Image() /* icon */,
-      base::string16() /* display_source */,
-      notifier_id1,
-      RichNotificationData(),
-      NULL));
+
+  // Then update a notification; nothing should have happened.
+  notification.reset(CreateSimpleNotification(id));
   message_center()->UpdateNotification(id2, notification.Pass());
   EXPECT_TRUE(message_center()->HasNotification(id2));
   EXPECT_FALSE(message_center()->HasNotification(id));
+
+  // Close the message center; then the update should have propagated.
   message_center()->SetVisibility(VISIBILITY_TRANSIENT);
   EXPECT_FALSE(message_center()->HasNotification(id2));
   EXPECT_TRUE(message_center()->HasNotification(id));
 }
 
+TEST_F(MessageCenterImplTest, ComplexQueueing) {
+  std::string ids[5] = {"0", "1", "2", "3", "4p"};
+  NotifierId notifier_id1(NotifierId::APPLICATION, "app1");
+
+  scoped_ptr<Notification> notification;
+  // Add some notifications
+  int i = 0;
+  for (; i < 3; i++) {
+    notification.reset(CreateSimpleNotification(ids[i]));
+    message_center()->AddNotification(notification.Pass());
+  }
+  for (i = 0; i < 3; i++) {
+    EXPECT_TRUE(message_center()->HasNotification(ids[i]));
+  }
+  for (; i < 5; i++) {
+    EXPECT_FALSE(message_center()->HasNotification(ids[i]));
+  }
+
+  notification.reset(CreateNotification(ids[4], NOTIFICATION_TYPE_PROGRESS));
+  message_center()->AddNotification(notification.Pass());
+
+  // Now start queueing.
+  // NL: ["0", "1", "2", "4p"]
+  message_center()->SetVisibility(VISIBILITY_MESSAGE_CENTER);
+
+  // This should update notification "1" to have id "3".
+  notification.reset(CreateSimpleNotification(ids[3]));
+  message_center()->UpdateNotification(ids[1], notification.Pass());
+
+  notification.reset(CreateSimpleNotification(ids[4]));
+  message_center()->UpdateNotification(ids[4], notification.Pass());
+
+  notification.reset(CreateNotification(ids[4], NOTIFICATION_TYPE_PROGRESS));
+  message_center()->UpdateNotification(ids[4], notification.Pass());
+
+  // This should update notification "3" to a new ID after we go TRANSIENT.
+  notification.reset(CreateSimpleNotification("New id"));
+  message_center()->UpdateNotification(ids[3], notification.Pass());
+
+  // This should create a new "3", that doesn't overwrite the update to 3
+  // before.
+  notification.reset(CreateSimpleNotification(ids[3]));
+  message_center()->AddNotification(notification.Pass());
+
+  // The NL should still be the same: ["0", "1", "2", "4p"]
+  EXPECT_TRUE(message_center()->HasNotification(ids[0]));
+  EXPECT_TRUE(message_center()->HasNotification(ids[1]));
+  EXPECT_TRUE(message_center()->HasNotification(ids[2]));
+  EXPECT_FALSE(message_center()->HasNotification(ids[3]));
+  EXPECT_TRUE(message_center()->HasNotification(ids[4]));
+  EXPECT_EQ(message_center()->GetVisibleNotifications().size(), 4u);
+  message_center()->SetVisibility(VISIBILITY_TRANSIENT);
+
+  EXPECT_TRUE(message_center()->HasNotification(ids[0]));
+  EXPECT_FALSE(message_center()->HasNotification(ids[1]));
+  EXPECT_TRUE(message_center()->HasNotification(ids[2]));
+  EXPECT_TRUE(message_center()->HasNotification(ids[3]));
+  EXPECT_TRUE(message_center()->HasNotification(ids[4]));
+  EXPECT_TRUE(message_center()->HasNotification("New id"));
+  EXPECT_EQ(message_center()->GetVisibleNotifications().size(), 5u);
+}
+
 }  // namespace internal
 }  // namespace message_center
diff --git a/ui/message_center/notification_list.cc b/ui/message_center/notification_list.cc
index bed592c..73ea08b 100644
--- a/ui/message_center/notification_list.cc
+++ b/ui/message_center/notification_list.cc
@@ -113,6 +113,9 @@ void NotificationList::UpdateNotificationMessage(
   Notification* old = *iter;
   notifications_.erase(iter);
   delete old;
+
+  // We really don't want duplicate IDs.
+  DCHECK(GetNotification(new_notification->id()) == notifications_.end());
   notifications_.insert(new_notification.release());
 }
 
diff --git a/ui/message_center/views/message_center_bubble.cc b/ui/message_center/views/message_center_bubble.cc
index aa8b646..5886531b 100644
--- a/ui/message_center/views/message_center_bubble.cc
+++ b/ui/message_center/views/message_center_bubble.cc
@@ -115,7 +115,7 @@ void MessageCenterBubble::UpdateBubbleView() {
   if (!bubble_view())
     return;  // Could get called after view is closed
   const NotificationList::Notifications& notifications =
-      message_center()->GetNotifications();
+      message_center()->GetVisibleNotifications();
   message_center_view_->SetNotifications(notifications);
   bubble_view()->GetWidget()->Show();
   bubble_view()->UpdateBubble();
diff --git a/ui/message_center/views/message_center_view.cc b/ui/message_center/views/message_center_view.cc
index 2acece7..19cb85f 100644
--- a/ui/message_center/views/message_center_view.cc
+++ b/ui/message_center/views/message_center_view.cc
@@ -861,7 +861,7 @@ void MessageCenterView::OnMouseExited(const ui::MouseEvent& event) {
 void MessageCenterView::OnNotificationAdded(const std::string& id) {
   int index = 0;
   const NotificationList::Notifications& notifications =
-      message_center_->GetNotifications();
+      message_center_->GetVisibleNotifications();
   for (NotificationList::Notifications::const_iterator iter =
            notifications.begin(); iter != notifications.end();
        ++iter, ++index) {
@@ -908,7 +908,7 @@ void MessageCenterView::OnNotificationRemoved(const std::string& id,
 
 void MessageCenterView::OnNotificationUpdated(const std::string& id) {
   const NotificationList::Notifications& notifications =
-      message_center_->GetNotifications();
+      message_center_->GetVisibleNotifications();
   size_t index = 0;
   for (NotificationList::Notifications::const_iterator iter =
            notifications.begin();
diff --git a/ui/message_center/views/toast_contents_view.cc b/ui/message_center/views/toast_contents_view.cc
index 645c654..1ca3ed0 100644
--- a/ui/message_center/views/toast_contents_view.cc
+++ b/ui/message_center/views/toast_contents_view.cc
@@ -95,7 +95,7 @@ void ToastContentsView::SetContents(MessageView* view) {
   // won't be read for this view which returns ROLE_WINDOW.
   if (already_has_contents) {
     const NotificationList::Notifications& notifications =
-        message_center_->GetNotifications();
+        message_center_->GetVisibleNotifications();
     for (NotificationList::Notifications::const_iterator iter =
              notifications.begin(); iter != notifications.end(); ++iter) {
       if ((*iter)->id() != id_)