Fix bug that can break stacked notification icons order

The order of the stacked notifications icons can be broken if a
notification is added / removed from the middle of the stacked
notifications. This fix prevents it by removing a notification's icon
through id lookup when a notification is removed and resetting the
notification_icons_container_ if a new notification is added while the
tray is open.

Change-Id: I108c983dcb1b42be67354598064e5e1daa299697
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1913877
Commit-Queue: Ahmed Mehfooz <amehfooz@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Tim Song <tengs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#715117}

ash/system/message_center/stacked_notification_bar.cc

@@ -15,6 +15,7 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/compositor/scoped_layer_animation_settings.h"
 #include "ui/gfx/canvas.h"
+#include "ui/message_center/message_center.h"
 #include "ui/message_center/public/cpp/message_center_constants.h"
 #include "ui/message_center/vector_icons.h"
 #include "ui/views/animation/flood_fill_ink_drop_ripple.h"
@@ -113,6 +114,17 @@ class StackingBarLabelButton : public views::LabelButton {
 
 }  // namespace
 
+class StackedNotificationBar::StackedNotificationBarIcon
+    : public views::ImageView {
+ public:
+  StackedNotificationBarIcon(const std::string& id)
+      : views::ImageView(), id_(id) {}
+  const std::string& id() const { return id_; }
+
+ private:
+  std::string id_;
+};
+
 StackedNotificationBar::StackedNotificationBar(
     UnifiedMessageCenterView* message_center_view)
     : message_center_view_(message_center_view),
@@ -126,7 +138,7 @@ StackedNotificationBar::StackedNotificationBar(
           l10n_util::GetStringUTF16(
               IDS_ASH_MESSAGE_CENTER_EXPAND_ALL_NOTIFICATIONS_BUTTON_LABEL))) {
   SetVisible(false);
-
+  message_center::MessageCenter::Get()->AddObserver(this);
   int left_padding = features::IsUnifiedMessageCenterRefactorEnabled()
                          ? 0
                          : kStackingNotificationClearAllButtonPadding.left();
@@ -165,7 +177,12 @@ StackedNotificationBar::StackedNotificationBar(
   AddChildView(expand_all_button_);
 }
 
-StackedNotificationBar::~StackedNotificationBar() = default;
+StackedNotificationBar::~StackedNotificationBar() {
+  // The MessageCenter may be destroyed already during shutdown. See
+  // crbug.com/946153.
+  if (message_center::MessageCenter::Get())
+    message_center::MessageCenter::Get()->RemoveObserver(this);
+}
 
 bool StackedNotificationBar::Update(
     int total_notification_count,
@@ -213,8 +230,8 @@ void StackedNotificationBar::SetExpanded() {
 void StackedNotificationBar::AddNotificationIcon(
     message_center::Notification* notification,
     bool at_front) {
-  views::ImageView* icon_view_ = new views::ImageView();
-
+  views::ImageView* icon_view_ =
+      new StackedNotificationBarIcon(notification->id());
   if (at_front)
     notification_icons_container_->AddChildViewAt(icon_view_, 0);
   else
@@ -233,6 +250,17 @@ void StackedNotificationBar::AddNotificationIcon(
   }
 }
 
+const StackedNotificationBar::StackedNotificationBarIcon*
+StackedNotificationBar::GetIconFromId(const std::string& id) {
+  for (auto* v : notification_icons_container_->children()) {
+    const StackedNotificationBarIcon* icon =
+        static_cast<const StackedNotificationBarIcon*>(v);
+    if (icon->id() == id)
+      return icon;
+  }
+  return nullptr;
+}
+
 void StackedNotificationBar::ShiftIconsLeft(
     std::vector<message_center::Notification*> stacked_notifications) {
   int stacked_notification_count = stacked_notifications.size();
@@ -362,4 +390,23 @@ void StackedNotificationBar::ButtonPressed(views::Button* sender,
   }
 }
 
+void StackedNotificationBar::OnNotificationAdded(const std::string& id) {
+  // Reset the stacked icons bar if a notification is added since we don't
+  // know the position where it may have been added.
+  notification_icons_container_->RemoveAllChildViews(true);
+  stacked_notification_count_ = 0;
+  UpdateStackedNotifications(message_center_view_->GetStackedNotifications());
+}
+
+void StackedNotificationBar::OnNotificationRemoved(const std::string& id,
+                                                   bool by_user) {
+  const StackedNotificationBarIcon* icon = GetIconFromId(id);
+  if (icon) {
+    delete icon;
+    stacked_notification_count_--;
+  }
+}
+
+void StackedNotificationBar::OnNotificationUpdated(const std::string& id) {}
+
 }  // namespace ash

ash/system/message_center/stacked_notification_bar.h

@@ -10,6 +10,7 @@
 #include "ash/system/message_center/unified_message_center_view.h"
 #include "ash/system/message_center/unified_message_list_view.h"
 #include "ui/gfx/animation/animation_delegate.h"
+#include "ui/message_center/message_center_observer.h"
 #include "ui/views/background.h"
 #include "ui/views/controls/button/button.h"
 #include "ui/views/controls/image_view.h"
@@ -27,7 +28,8 @@ namespace ash {
 // notifications. There are currently two UI implementations toggled by the
 // NotificationStackedBarRedesign feature flag.
 class StackedNotificationBar : public views::View,
-                               public views::ButtonListener {
+                               public views::ButtonListener,
+                               public message_center::MessageCenterObserver {
  public:
   explicit StackedNotificationBar(
       UnifiedMessageCenterView* message_center_view);
@@ -55,9 +57,19 @@ class StackedNotificationBar : public views::View,
   // views::ButtonListener:
   void ButtonPressed(views::Button* sender, const ui::Event& event) override;
 
+  // message_center::MessageCenterObserver:
+  void OnNotificationAdded(const std::string& id) override;
+  void OnNotificationRemoved(const std::string& id, bool by_user) override;
+  void OnNotificationUpdated(const std::string& id) override;
+
  private:
+  class StackedNotificationBarIcon;
   friend class UnifiedMessageCenterViewTest;
 
+  // Search for a icon view in the stacked notification bar based on a provided
+  // notification id.
+  const StackedNotificationBarIcon* GetIconFromId(const std::string& id);
+
   // Set visibility based on number of stacked notifications or animation state.
   void UpdateVisibility();