GMC: Add separator lines between notifications

This CL uses borders to separate notifications in the media dialog.

Bug: 1004707
Change-Id: I3c129e46ac1c855a906ac0577fcd092f9087c576
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1864096
Commit-Queue: Tommy Steimel <steimel@chromium.org>
Reviewed-by: Jazz Xu <jazzhsu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706770}

chrome/browser/ui/views/global_media_controls/media_notification_list_view.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/ui/views/global_media_controls/media_notification_list_view.h"
 
 #include "chrome/browser/ui/views/global_media_controls/media_notification_container_impl_view.h"
+#include "ui/views/border.h"
 #include "ui/views/controls/scrollbar/overlay_scroll_bar.h"
 #include "ui/views/layout/box_layout.h"
 
@@ -12,6 +13,19 @@ namespace {
 
 constexpr int kMediaListMaxHeight = 478;
 
+// Color for border between notifications.
+constexpr SkColor kMediaListSeparatorColor = SkColorSetA(SK_ColorBLACK, 31);
+
+// Thickness of separator border.
+constexpr int kMediaListSeparatorThickness = 2;
+
+std::unique_ptr<views::Border> CreateMediaListSeparatorBorder() {
+  return views::CreateSolidSidedBorder(/*top=*/kMediaListSeparatorThickness,
+                                       /*left=*/0,
+                                       /*bottom=*/0,
+                                       /*right=*/0, kMediaListSeparatorColor);
+}
+
 }  // anonymous namespace
 
 MediaNotificationListView::MediaNotificationListView() {
@@ -35,6 +49,11 @@ void MediaNotificationListView::ShowNotification(
   DCHECK(!base::Contains(notifications_, id));
   DCHECK_NE(nullptr, notification.get());
 
+  // If this isn't the first notification, then create a top-sided separator
+  // border.
+  if (!notifications_.empty())
+    notification->SetBorder(CreateMediaListSeparatorBorder());
+
   notifications_[id] = contents()->AddChildView(std::move(notification));
 
   contents()->InvalidateLayout();
@@ -45,7 +64,18 @@ void MediaNotificationListView::HideNotification(const std::string& id) {
   if (!base::Contains(notifications_, id))
     return;
 
+  // If we're removing the topmost notification and there are others, then we
+  // need to remove the top-sided separator border from the new topmost
+  // notification.
+  if (contents()->children().size() > 1 &&
+      contents()->children().at(0) == notifications_[id]) {
+    contents()->children().at(1)->SetBorder(nullptr);
+  }
+
+  // Remove the notification. Note that since |RemoveChildView()| does not
+  // delete the notification, we must do so manually here.
   contents()->RemoveChildView(notifications_[id]);
+  delete notifications_[id];
   notifications_.erase(id);
 
   contents()->InvalidateLayout();

chrome/browser/ui/views/global_media_controls/media_notification_list_view_unittest.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/views/global_media_controls/media_notification_list_view.h"
+
+#include <string>
+
+#include "chrome/browser/ui/views/global_media_controls/media_notification_container_impl_view.h"
+#include "ui/views/test/views_test_base.h"
+
+namespace {
+
+// Test IDs for notifications.
+const char kTestNotificationId1[] = "testid1";
+const char kTestNotificationId2[] = "testid2";
+const char kTestNotificationId3[] = "testid3";
+
+}  // anonymous namespace
+
+class MediaNotificationListViewTest : public views::ViewsTestBase {
+ public:
+  MediaNotificationListViewTest() = default;
+  ~MediaNotificationListViewTest() override = default;
+
+  // ViewsTestBase:
+  void SetUp() override {
+    ViewsTestBase::SetUp();
+
+    views::Widget::InitParams params =
+        CreateParams(views::Widget::InitParams::TYPE_WINDOW_FRAMELESS);
+    params.bounds = gfx::Rect(400, 400);
+    params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+    widget_.Init(std::move(params));
+
+    list_view_ = new MediaNotificationListView();
+    widget_.SetContentsView(list_view_);
+
+    widget_.Show();
+  }
+
+  void TearDown() override {
+    widget_.Close();
+    ViewsTestBase::TearDown();
+  }
+
+  void ShowNotification(const std::string& id) {
+    list_view_->ShowNotification(
+        id, std::make_unique<MediaNotificationContainerImplView>(id, nullptr));
+  }
+
+  void HideNotification(const std::string& id) {
+    list_view_->HideNotification(id);
+  }
+
+  MediaNotificationListView* list_view() { return list_view_; }
+
+ private:
+  views::Widget widget_;
+  MediaNotificationListView* list_view_ = nullptr;
+
+  DISALLOW_COPY_AND_ASSIGN(MediaNotificationListViewTest);
+};
+
+TEST_F(MediaNotificationListViewTest, NoSeparatorForOneNotification) {
+  // Show a single notification.
+  ShowNotification(kTestNotificationId1);
+
+  // There should be just one notification.
+  EXPECT_EQ(1u, list_view()->notifications_for_testing().size());
+
+  // Since there's only one, there should be no separator line.
+  EXPECT_EQ(nullptr, list_view()
+                         ->notifications_for_testing()
+                         .at(kTestNotificationId1)
+                         ->border());
+}
+
+TEST_F(MediaNotificationListViewTest, SeparatorBetweenNotifications) {
+  // Show two notifications.
+  ShowNotification(kTestNotificationId1);
+  ShowNotification(kTestNotificationId2);
+
+  // There should be two notifications.
+  EXPECT_EQ(2u, list_view()->notifications_for_testing().size());
+
+  // There should be a separator between them. Since the separators are
+  // top-sided, the bottom notification should have one.
+  EXPECT_EQ(nullptr, list_view()
+                         ->notifications_for_testing()
+                         .at(kTestNotificationId1)
+                         ->border());
+  EXPECT_NE(nullptr, list_view()
+                         ->notifications_for_testing()
+                         .at(kTestNotificationId2)
+                         ->border());
+}
+
+TEST_F(MediaNotificationListViewTest, SeparatorRemovedWhenNotificationRemoved) {
+  // Show three notifications.
+  ShowNotification(kTestNotificationId1);
+  ShowNotification(kTestNotificationId2);
+  ShowNotification(kTestNotificationId3);
+
+  // There should be three notifications.
+  EXPECT_EQ(3u, list_view()->notifications_for_testing().size());
+
+  // There should be separators.
+  EXPECT_EQ(nullptr, list_view()
+                         ->notifications_for_testing()
+                         .at(kTestNotificationId1)
+                         ->border());
+  EXPECT_NE(nullptr, list_view()
+                         ->notifications_for_testing()
+                         .at(kTestNotificationId2)
+                         ->border());
+  EXPECT_NE(nullptr, list_view()
+                         ->notifications_for_testing()
+                         .at(kTestNotificationId3)
+                         ->border());
+
+  // Remove the topmost notification.
+  HideNotification(kTestNotificationId1);
+
+  // There should be two notifications.
+  EXPECT_EQ(2u, list_view()->notifications_for_testing().size());
+
+  // The new top notification should have lost its top separator.
+  EXPECT_EQ(nullptr, list_view()
+                         ->notifications_for_testing()
+                         .at(kTestNotificationId2)
+                         ->border());
+  EXPECT_NE(nullptr, list_view()
+                         ->notifications_for_testing()
+                         .at(kTestNotificationId3)
+                         ->border());
+}

chrome/test/BUILD.gn

@@ -5013,6 +5013,7 @@ test("unit_tests") {
       "../browser/ui/views/fullscreen_control/fullscreen_control_popup_unittest.cc",
       "../browser/ui/views/global_error_bubble_view_unittest.cc",
       "../browser/ui/views/global_media_controls/media_notification_container_impl_view_unittest.cc",
+      "../browser/ui/views/global_media_controls/media_notification_list_view_unittest.cc",
       "../browser/ui/views/hover_button_unittest.cc",
       "../browser/ui/views/infobars/infobar_view_unittest.cc",
       "../browser/ui/views/intent_picker_bubble_view_unittest.cc",