fix: keep layout of notifications after updates

This ensures that the layout of notifications is kept as intended.

Bug: 901045
Change-Id: I0dcc92464a5751cd61ff7552824c57798aa73661
Reviewed-on: https://chromium-review.googlesource.com/c/1320169
Commit-Queue: Richard Knoll <knollr@chromium.org>
Reviewed-by: Justin DeWitt <dewittj@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609356}

ui/message_center/views/notification_view.cc

@@ -146,6 +146,8 @@ void NotificationItemView::SetVisible(bool visible) {
 // NotificationView ////////////////////////////////////////////////////////////
 
 void NotificationView::CreateOrUpdateViews(const Notification& notification) {
+  top_view_count_ = 0;
+
   CreateOrUpdateTitleView(notification);
   CreateOrUpdateMessageView(notification);
   CreateOrUpdateProgressBarView(notification);
@@ -370,10 +372,12 @@ void NotificationView::CreateOrUpdateTitleView(
     title_view_->SetLineLimit(kMaxTitleLines);
     title_view_->SetColor(kRegularTextColor);
     title_view_->SetBorder(MakeTextBorder(padding, 3, 0));
-    top_view_->AddChildView(title_view_);
+    top_view_->AddChildViewAt(title_view_, top_view_count_);
   } else {
     title_view_->SetText(title);
   }
+
+  top_view_count_++;
 }
 
 void NotificationView::CreateOrUpdateMessageView(
@@ -396,12 +400,13 @@ void NotificationView::CreateOrUpdateMessageView(
     message_view_->SetLineHeight(kMessageLineHeight);
     message_view_->SetColor(kRegularTextColor);
     message_view_->SetBorder(MakeTextBorder(padding, 4, 0));
-    top_view_->AddChildView(message_view_);
+    top_view_->AddChildViewAt(message_view_, top_view_count_);
   } else {
     message_view_->SetText(text);
   }
 
   message_view_->SetVisible(notification.items().empty());
+  top_view_count_++;
 }
 
 base::string16 NotificationView::FormatContextMessage(
@@ -440,10 +445,12 @@ void NotificationView::CreateOrUpdateContextMessageView(
     context_message_view_->SetLineHeight(kMessageLineHeight);
     context_message_view_->SetColor(kDimTextColor);
     context_message_view_->SetBorder(MakeTextBorder(padding, 4, 0));
-    top_view_->AddChildView(context_message_view_);
+    top_view_->AddChildViewAt(context_message_view_, top_view_count_);
   } else {
     context_message_view_->SetText(message);
   }
+
+  top_view_count_++;
 }
 
 void NotificationView::CreateOrUpdateProgressBarView(
@@ -461,11 +468,12 @@ void NotificationView::CreateOrUpdateProgressBarView(
     progress_bar_view_ = new views::ProgressBar();
     progress_bar_view_->SetBorder(MakeProgressBarBorder(
         kProgressBarTopPadding, kProgressBarBottomPadding));
-    top_view_->AddChildView(progress_bar_view_);
+    top_view_->AddChildViewAt(progress_bar_view_, top_view_count_);
   }
 
   progress_bar_view_->SetValue(notification.progress() / 100.0);
   progress_bar_view_->SetVisible(notification.items().empty());
+  top_view_count_++;
 }
 
 void NotificationView::CreateOrUpdateListItemViews(
@@ -485,7 +493,7 @@ void NotificationView::CreateOrUpdateListItemViews(
     NotificationItemView* item_view = new NotificationItemView(items[i]);
     item_view->SetBorder(MakeTextBorder(padding, i ? 0 : 4, 0));
     item_views_.push_back(item_view);
-    top_view_->AddChildView(item_view);
+    top_view_->AddChildViewAt(item_view, top_view_count_++);
   }
 }
 

ui/message_center/views/notification_view.h

@@ -62,6 +62,7 @@ class MESSAGE_CENTER_EXPORT NotificationView : public MessageView,
   FRIEND_TEST_ALL_PREFIXES(NotificationViewTest, TestImageSizing);
   FRIEND_TEST_ALL_PREFIXES(NotificationViewTest, UpdateButtonsStateTest);
   FRIEND_TEST_ALL_PREFIXES(NotificationViewTest, UpdateButtonCountTest);
+  FRIEND_TEST_ALL_PREFIXES(NotificationViewTest, UpdateViewsOrderingTest);
 
   friend class NotificationViewTest;
 
@@ -108,6 +109,10 @@ class MESSAGE_CENTER_EXPORT NotificationView : public MessageView,
   std::unique_ptr<views::ImageView> small_image_view_;
   NotificationControlButtonsView* control_buttons_view_;
 
+  // Counter for view layouting, which is used during the CreateOrUpdate*
+  // phases to keep track of the view ordering. See crbug.com/901045
+  int top_view_count_;
+
   DISALLOW_COPY_AND_ASSIGN(NotificationView);
 };
 

ui/message_center/views/notification_view_md.cc

@@ -477,6 +477,8 @@ class InlineSettingsRadioButton : public views::RadioButton {
 // ////////////////////////////////////////////////////////////
 
 void NotificationViewMD::CreateOrUpdateViews(const Notification& notification) {
+  left_content_count_ = 0;
+
   CreateOrUpdateContextTitleView(notification);
   CreateOrUpdateTitleView(notification);
   CreateOrUpdateMessageView(notification);
@@ -801,10 +803,12 @@ void NotificationViewMD::CreateOrUpdateTitleView(
     title_view_->SetFontList(font_list);
     title_view_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
     title_view_->SetEnabledColor(kRegularTextColorMD);
-    left_content_->AddChildView(title_view_);
+    left_content_->AddChildViewAt(title_view_, left_content_count_);
   } else {
     title_view_->SetText(title);
   }
+
+  left_content_count_++;
 }
 
 void NotificationViewMD::CreateOrUpdateMessageView(
@@ -827,12 +831,13 @@ void NotificationViewMD::CreateOrUpdateMessageView(
     message_view_->SetLineLimit(kMaxLinesForMessageView);
     message_view_->SetColor(kDimTextColorMD);
 
-    left_content_->AddChildView(message_view_);
+    left_content_->AddChildViewAt(message_view_, left_content_count_);
   } else {
     message_view_->SetText(text);
   }
 
   message_view_->SetVisible(notification.items().empty());
+  left_content_count_++;
 }
 
 void NotificationViewMD::CreateOrUpdateCompactTitleMessageView(
@@ -846,12 +851,14 @@ void NotificationViewMD::CreateOrUpdateCompactTitleMessageView(
   }
   if (!compact_title_message_view_) {
     compact_title_message_view_ = new CompactTitleMessageView();
-    left_content_->AddChildView(compact_title_message_view_);
+    left_content_->AddChildViewAt(compact_title_message_view_,
+                                  left_content_count_);
   }
 
   compact_title_message_view_->set_title(notification.title());
   compact_title_message_view_->set_message(notification.message());
   left_content_->InvalidateLayout();
+  left_content_count_++;
 }
 
 void NotificationViewMD::CreateOrUpdateProgressBarView(
@@ -871,7 +878,7 @@ void NotificationViewMD::CreateOrUpdateProgressBarView(
                                                 /* allow_round_corner */ false);
     progress_bar_view_->SetBorder(
         views::CreateEmptyBorder(kProgressBarTopPadding, 0, 0, 0));
-    left_content_->AddChildView(progress_bar_view_);
+    left_content_->AddChildViewAt(progress_bar_view_, left_content_count_);
   }
 
   progress_bar_view_->SetValue(notification.progress() / 100.0);
@@ -881,6 +888,8 @@ void NotificationViewMD::CreateOrUpdateProgressBarView(
     header_row_->SetProgress(notification.progress());
   else
     header_row_->ClearProgress();
+
+  left_content_count_++;
 }
 
 void NotificationViewMD::CreateOrUpdateProgressStatusView(
@@ -902,10 +911,11 @@ void NotificationViewMD::CreateOrUpdateProgressStatusView(
     status_view_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
     status_view_->SetEnabledColor(kDimTextColorMD);
     status_view_->SetBorder(views::CreateEmptyBorder(kStatusTextPadding));
-    left_content_->AddChildView(status_view_);
+    left_content_->AddChildViewAt(status_view_, left_content_count_);
   }
 
   status_view_->SetText(notification.progress_status());
+  left_content_count_++;
 }
 
 void NotificationViewMD::CreateOrUpdateListItemViews(
@@ -920,7 +930,7 @@ void NotificationViewMD::CreateOrUpdateListItemViews(
        ++i) {
     std::unique_ptr<views::View> item_view = CreateItemView(items[i]);
     item_views_.push_back(item_view.get());
-    left_content_->AddChildView(item_view.release());
+    left_content_->AddChildViewAt(item_view.release(), left_content_count_++);
   }
 
   list_items_count_ = items.size();

ui/message_center/views/notification_view_md.h

@@ -222,6 +222,7 @@ class MESSAGE_CENTER_EXPORT NotificationViewMD
   FRIEND_TEST_ALL_PREFIXES(NotificationViewMDTest, UseImageAsIcon);
   FRIEND_TEST_ALL_PREFIXES(NotificationViewMDTest, NotificationWithoutIcon);
   FRIEND_TEST_ALL_PREFIXES(NotificationViewMDTest, InlineSettings);
+  FRIEND_TEST_ALL_PREFIXES(NotificationViewMDTest, UpdateViewsOrderingTest);
 
   friend class NotificationViewMDTest;
 
@@ -292,6 +293,10 @@ class MESSAGE_CENTER_EXPORT NotificationViewMD
   views::View* action_buttons_row_ = nullptr;
   NotificationInputContainerMD* inline_reply_ = nullptr;
 
+  // Counter for view layouting, which is used during the CreateOrUpdate*
+  // phases to keep track of the view ordering. See crbug.com/901045
+  int left_content_count_;
+
   // Views for inline settings.
   views::RadioButton* block_all_button_ = nullptr;
   views::RadioButton* dont_block_button_ = nullptr;

ui/message_center/views/notification_view_md_unittest.cc

@@ -344,6 +344,36 @@ TEST_F(NotificationViewMDTest, CreateOrUpdateTest) {
   EXPECT_EQ(nullptr, notification_view()->icon_view_);
 }
 
+TEST_F(NotificationViewMDTest, UpdateViewsOrderingTest) {
+  EXPECT_NE(nullptr, notification_view()->title_view_);
+  EXPECT_NE(nullptr, notification_view()->message_view_);
+  EXPECT_EQ(0, notification_view()->left_content_->GetIndexOf(
+                   notification_view()->title_view_));
+  EXPECT_EQ(1, notification_view()->left_content_->GetIndexOf(
+                   notification_view()->message_view_));
+
+  std::unique_ptr<Notification> notification = CreateSimpleNotification();
+  notification->set_title(base::string16());
+
+  notification_view()->CreateOrUpdateViews(*notification);
+
+  EXPECT_EQ(nullptr, notification_view()->title_view_);
+  EXPECT_NE(nullptr, notification_view()->message_view_);
+  EXPECT_EQ(0, notification_view()->left_content_->GetIndexOf(
+                   notification_view()->message_view_));
+
+  notification->set_title(base::UTF8ToUTF16("title"));
+
+  notification_view()->CreateOrUpdateViews(*notification);
+
+  EXPECT_NE(nullptr, notification_view()->title_view_);
+  EXPECT_NE(nullptr, notification_view()->message_view_);
+  EXPECT_EQ(0, notification_view()->left_content_->GetIndexOf(
+                   notification_view()->title_view_));
+  EXPECT_EQ(1, notification_view()->left_content_->GetIndexOf(
+                   notification_view()->message_view_));
+}
+
 TEST_F(NotificationViewMDTest, TestIconSizing) {
   // TODO(tetsui): Remove duplicated integer literal in CreateOrUpdateIconView.
   const int kNotificationIconSize = 36;

ui/message_center/views/notification_view_unittest.cc

@@ -28,6 +28,7 @@
 #include "ui/message_center/public/cpp/message_center_constants.h"
 #include "ui/message_center/public/cpp/notification.h"
 #include "ui/message_center/public/cpp/notification_types.h"
+#include "ui/message_center/views/bounded_label.h"
 #include "ui/message_center/views/message_view_factory.h"
 #include "ui/message_center/views/notification_button.h"
 #include "ui/message_center/views/notification_control_buttons_view.h"
@@ -284,6 +285,35 @@ TEST_F(NotificationViewTest, CreateOrUpdateTest) {
   EXPECT_TRUE(NULL != notification_view()->icon_view_);
 }
 
+TEST_F(NotificationViewTest, UpdateViewsOrderingTest) {
+  EXPECT_NE(nullptr, notification_view()->title_view_);
+  EXPECT_NE(nullptr, notification_view()->message_view_);
+  EXPECT_EQ(0, notification_view()->top_view_->GetIndexOf(
+                   notification_view()->title_view_));
+  EXPECT_EQ(1, notification_view()->top_view_->GetIndexOf(
+                   notification_view()->message_view_));
+
+  notification()->set_title(base::string16());
+
+  notification_view()->CreateOrUpdateViews(*notification());
+
+  EXPECT_EQ(nullptr, notification_view()->title_view_);
+  EXPECT_NE(nullptr, notification_view()->message_view_);
+  EXPECT_EQ(0, notification_view()->top_view_->GetIndexOf(
+                   notification_view()->message_view_));
+
+  notification()->set_title(base::UTF8ToUTF16("title"));
+
+  notification_view()->CreateOrUpdateViews(*notification());
+
+  EXPECT_NE(nullptr, notification_view()->title_view_);
+  EXPECT_NE(nullptr, notification_view()->message_view_);
+  EXPECT_EQ(0, notification_view()->top_view_->GetIndexOf(
+                   notification_view()->title_view_));
+  EXPECT_EQ(1, notification_view()->top_view_->GetIndexOf(
+                   notification_view()->message_view_));
+}
+
 TEST_F(NotificationViewTest, CreateOrUpdateTestSettingsButton) {
   data()->settings_button_handler = SettingsButtonHandler::INLINE;
   Notification notification(