Fixes the policy to choose the notifications for popups.

The existing code: choose the newer items, the new code: choose the older items
I wrote this existing code, but I was wrong.

BUG=165768
TEST=message_center_unittests passed


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

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

3 files changed, 47 insertions, 16 deletions

diff --git a/ui/message_center/notification_list.cc b/ui/message_center/notification_list.cc
index 9425454..6cbaa70 100644
--- a/ui/message_center/notification_list.cc
+++ b/ui/message_center/notification_list.cc
@@ -234,7 +234,7 @@ void NotificationList::GetPopupNotifications(
   for (int i = ui::notifications::DEFAULT_PRIORITY;
        i <= ui::notifications::MAX_PRIORITY; ++i) {
     Notifications::iterator first, last;
-    GetPopupIterators(i, first, last);
+    GetPopupIterators(i, &first, &last);
     if (first != last)
       iters.push_back(make_pair(first, last));
   }
@@ -256,7 +256,7 @@ void NotificationList::GetPopupNotifications(
 
 void NotificationList::MarkPopupsAsShown(int priority) {
   Notifications::iterator first, last;
-  GetPopupIterators(priority, first, last);
+  GetPopupIterators(priority, &first, &last);
   for (Notifications::iterator iter = first; iter != last; ++iter)
     iter->shown_as_popup = true;
 }
@@ -393,28 +393,32 @@ void NotificationList::PushNotification(Notification& notification) {
 }
 
 void NotificationList::GetPopupIterators(int priority,
-                                         Notifications::iterator& first,
-                                         Notifications::iterator& last) {
+                                         Notifications::iterator* first,
+                                         Notifications::iterator* last) {
   Notifications& notifications = notifications_[priority];
   // No popups for LOW/MIN priority.
   if (priority < ui::notifications::DEFAULT_PRIORITY) {
-    first = notifications.end();
-    last = notifications.end();
+    *first = notifications.end();
+    *last = notifications.end();
     return;
   }
 
   size_t popup_count = 0;
-  first = notifications.begin();
-  last = first;
-  while (last != notifications.end()) {
-    if (last->shown_as_popup)
+  *first = notifications.begin();
+  *last = *first;
+  while (*last != notifications.end()) {
+    if ((*last)->shown_as_popup)
       break;
-    ++last;
-    popup_count++;
-    // No limits for HIGH/MAX priority.
+    ++(*last);
+
+    // Checking limits. No limits for HIGH/MAX priority. DEFAULT priority
+    // will return at most kMaxVisiblePopupNotifications entries. If the
+    // popup entries are more, older entries are used. see crbug.com/165768
     if (priority == ui::notifications::DEFAULT_PRIORITY &&
         popup_count >= kMaxVisiblePopupNotifications) {
-      break;
+      ++(*first);
+    } else {
+      ++popup_count;
     }
   }
 }
diff --git a/ui/message_center/notification_list.h b/ui/message_center/notification_list.h
index 7bc1e87..96368bb 100644
--- a/ui/message_center/notification_list.h
+++ b/ui/message_center/notification_list.h
@@ -184,8 +184,8 @@ class MESSAGE_CENTER_EXPORT NotificationList {
   // as a popup. kMaxVisiblePopupNotifications are used to limit the number of
   // notifications for the default priority.
   void GetPopupIterators(int priority,
-                         Notifications::iterator& first,
-                         Notifications::iterator& last);
+                         Notifications::iterator* first,
+                         Notifications::iterator* last);
 
   // Given a dictionary of optional notification fields (or NULL), unpacks all
   // recognized values into the given Notification struct. We assume prior
diff --git a/ui/message_center/notification_list_unittest.cc b/ui/message_center/notification_list_unittest.cc
index f863280..97f663a 100644
--- a/ui/message_center/notification_list_unittest.cc
+++ b/ui/message_center/notification_list_unittest.cc
@@ -212,6 +212,33 @@ TEST_F(NotificationListTest, SendRemoveNotifications) {
   EXPECT_EQ(3u, delegate()->GetSendRemoveCountAndReset());
 }
 
+TEST_F(NotificationListTest, OldPopupShouldNotBeHidden) {
+  std::vector<std::string> ids;
+  for (size_t i = 0; i <= NotificationList::kMaxVisiblePopupNotifications;
+       i++) {
+    ids.push_back(AddNotification(NULL));
+  }
+
+  NotificationList::Notifications popups;
+  notification_list()->GetPopupNotifications(&popups);
+  // The popup should contain the oldest kMaxVisiblePopupNotifications. Newer
+  // one should come earlier in the popup list. It means, the last element
+  // of |popups| should be the firstly added one, and so on.
+  EXPECT_EQ(NotificationList::kMaxVisiblePopupNotifications, popups.size());
+  NotificationList::Notifications::const_reverse_iterator iter =
+      popups.rbegin();
+  for (size_t i = 0; i < NotificationList::kMaxVisiblePopupNotifications;
+       ++i, ++iter) {
+    EXPECT_EQ(ids[i], iter->id) << i;
+  }
+
+  notification_list()->MarkPopupsAsShown(ui::notifications::DEFAULT_PRIORITY);
+  popups.clear();
+  notification_list()->GetPopupNotifications(&popups);
+  EXPECT_EQ(1u, popups.size());
+  EXPECT_EQ(ids[ids.size() - 1], popups.begin()->id);
+}
+
 TEST_F(NotificationListTest, Priority) {
   ASSERT_EQ(0u, notification_list()->NotificationCount());
   ASSERT_EQ(0u, notification_list()->unread_count());