Fix the bug that external display shows the old notification right after being connected.

In current code, notification may be removed before animation which is
triggered by the previous operation on MessagePopupCollection ends. As
result, when a new notification with the same ID is created, calling
MessagePopupCollection::Update will not update the popup's content. Then
the new notification popup fails to show. The CL fixes this bug.

Test: message_center_unittests
Bug: 921402
Change-Id: I56743b5e17487d3f5bf52ef934b0a53d8f6471d0
Reviewed-on: https://chromium-review.googlesource.com/c/1422738Reviewed-by: Justin DeWitt <dewittj@chromium.org>
Reviewed-by: Tetsui Ohkubo <tetsui@chromium.org>
Commit-Queue: Andrew Xu <andrewxu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#626116}

ui/message_center/views/message_popup_collection.cc

@@ -119,7 +119,13 @@ void MessagePopupCollection::NotifyPopupClosed(MessagePopupView* popup) {
 
 void MessagePopupCollection::OnNotificationAdded(
     const std::string& notification_id) {
-  Update();
+  // Should not call MessagePopupCollection::Update here. Because notification
+  // may be removed before animation which is triggered by the previous
+  // operation on MessagePopupCollection ends. As result, when a new
+  // notification with the same ID is created, calling
+  // MessagePopupCollection::Update will not update the popup's content. Then
+  // the new notification popup fails to show. (see https://crbug.com/921402)
+  OnNotificationUpdated(notification_id);
 }
 
 void MessagePopupCollection::OnNotificationRemoved(

ui/message_center/views/message_popup_collection_unittest.cc

@@ -116,7 +116,9 @@ class MockMessagePopupView : public MessagePopupView {
                        MockMessagePopupCollection* popup_collection)
       : MessagePopupView(alignment_delegate, popup_collection),
         popup_collection_(popup_collection),
-        id_(id) {
+        id_(id),
+        title_(base::UTF16ToUTF8(
+            MessageCenter::Get()->FindVisibleNotificationById(id)->title())) {
     auto* view = new views::View;
     view->SetPreferredSize(gfx::Size(kNotificationWidth, init_height));
     AddChildView(view);
@@ -134,6 +136,7 @@ class MockMessagePopupView : public MessagePopupView {
       SetPreferredHeight(height_after_update_.value());
     popup_collection_->NotifyPopupResized();
     updated_ = true;
+    title_ = base::UTF16ToUTF8(notification.title());
   }
 
   void AutoCollapse() override {
@@ -165,6 +168,8 @@ class MockMessagePopupView : public MessagePopupView {
   const std::string& id() const { return id_; }
   bool updated() const { return updated_; }
 
+  const std::string& title() const { return title_; }
+
   void set_expandable(bool expandable) { expandable_ = expandable; }
 
   void set_height_after_update(base::Optional<int> height_after_update) {
@@ -177,6 +182,7 @@ class MockMessagePopupView : public MessagePopupView {
   std::string id_;
   bool updated_ = false;
   bool expandable_ = false;
+  std::string title_;
 
   base::Optional<int> height_after_update_;
 };
@@ -235,8 +241,13 @@ class MessagePopupCollectionTest : public views::ViewsTestBase,
 
  protected:
   std::unique_ptr<Notification> CreateNotification(const std::string& id) {
+    return CreateNotification(id, "test title");
+  }
+
+  std::unique_ptr<Notification> CreateNotification(const std::string& id,
+                                                   const std::string& title) {
     return std::make_unique<Notification>(
-        NOTIFICATION_TYPE_BASE_FORMAT, id, base::UTF8ToUTF16("test title"),
+        NOTIFICATION_TYPE_BASE_FORMAT, id, base::UTF8ToUTF16(title),
         base::UTF8ToUTF16("test message"), gfx::Image(),
         base::string16() /* display_source */, GURL(), NotifierId(),
         RichNotificationData(), new NotificationDelegate());
@@ -1100,4 +1111,44 @@ TEST_F(MessagePopupCollectionTest, HighPriorityNotificationShownAgain) {
   EXPECT_EQ(1u, GetPopupCounts());
 }
 
+// Notification removing may occur while the animation triggered by the previous
+// operation is running. As result, notification is removed from the message
+// center but its popup is still kept. At this moment, a new notification with
+// the same notification id may be added to the message center. This can happen
+// on Chrome OS when an external display is connected with the Chromebook device
+// (see https://crbug.com/921402). This test case emulates the procedure of
+// the external display connection that is mentioned in the link above. Verifies
+// that under this circumstance the notification popup is updated.
+TEST_F(MessagePopupCollectionTest, RemoveNotificationWhileAnimating) {
+  const std::string notification_id("test_id");
+  const std::string old_notification_title("old_title");
+  const std::string new_notification_title("new_title");
+
+  // Create a notification and add it to message center.
+  auto old_notification =
+      CreateNotification(notification_id, old_notification_title);
+  MessageCenter::Get()->AddNotification(std::move(old_notification));
+  AnimateToMiddle();
+
+  // On real device, MessageCenter::RemoveNotification is called before the
+  // animation ends. As result, notification is removed while popup keeps still.
+  EXPECT_TRUE(IsAnimating());
+  MessageCenter::Get()->RemoveNotification(notification_id, false);
+  EXPECT_FALSE(MessageCenter::Get()->HasPopupNotifications());
+  EXPECT_EQ(1u, GetPopupCounts());
+  EXPECT_EQ(old_notification_title, GetPopup(notification_id)->title());
+
+  // On real device, the new notification with the same notification id is
+  // created and added to message center before the animation ends.
+  auto new_notification =
+      CreateNotification(notification_id, new_notification_title);
+  EXPECT_TRUE(IsAnimating());
+  MessageCenter::Get()->AddNotification(std::move(new_notification));
+  AnimateUntilIdle();
+
+  // Verifies that the new notification popup is shown.
+  EXPECT_EQ(1u, GetPopupCounts());
+  EXPECT_EQ(new_notification_title, GetPopup(notification_id)->title());
+}
+
 }  // namespace message_center