GMC: Hide Chromium icons shown in notification header

This CL adds the ability to hide the app icon view in Message Center's NotificationHeaderView. This allows the Global Media Controls dialog to hide the unnecessary Chromium icon that appears before the URL. Bug: 1011047 Change-Id: Ic3e5a8e57ecfc14698697370e4fc0d5840c28ff0 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1875276 Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Tommy Steimel <steimel@chromium.org> Cr-Commit-Position: refs/heads/master@{#708760}
marcosholgado · Oct 23, 2019 · a4808cd · a4808cd
1 parent 4d9ad32
commit a4808cd
Show file tree

Hide file tree

Showing 8 changed files with 67 additions and 15 deletions.
diff --git a/ash/media/media_notification_container_impl.cc b/ash/media/media_notification_container_impl.cc
@@ -25,7 +25,8 @@ MediaNotificationContainerImpl::MediaNotificationContainerImpl(
           std::move(item),
           control_buttons_view_.get(),
           message_center::MessageCenter::Get()->GetSystemNotificationAppName(),
-          message_center::kNotificationWidth) {
+          message_center::kNotificationWidth,
+          /*should_show_icon=*/true) {
   SetLayoutManager(std::make_unique<views::FillLayout>());
 
   // Since we own these, we don't want Views to destroy them when their parent

diff --git a/chrome/browser/ui/views/global_media_controls/media_notification_container_impl_view.cc b/chrome/browser/ui/views/global_media_controls/media_notification_container_impl_view.cc
@@ -93,7 +93,7 @@ MediaNotificationContainerImplView::MediaNotificationContainerImplView(
 
   view_ = std::make_unique<media_message_center::MediaNotificationView>(
       this, std::move(item), dismiss_button_container_.get(), base::string16(),
-      kWidth);
+      kWidth, /*should_show_icon=*/false);
   view_->set_owned_by_client();
   ForceExpandedState();
 

diff --git a/components/media_message_center/media_notification_view.cc b/components/media_message_center/media_notification_view.cc
@@ -44,9 +44,10 @@ constexpr double kMediaImageMaxWidthExpandedPct = 0.4;
 constexpr gfx::Size kMediaButtonSize = gfx::Size(36, 36);
 constexpr int kMediaButtonRowSeparator = 8;
 constexpr gfx::Insets kMediaTitleArtistInsets = gfx::Insets(8, 8, 0, 8);
-constexpr int kMediaNotificationHeaderTopInset = 6;
-constexpr int kMediaNotificationHeaderRightInset = 6;
-constexpr int kMediaNotificationHeaderInset = 0;
+constexpr gfx::Insets kIconlessMediaNotificationHeaderInsets =
+    gfx::Insets(6, 14, 0, 6);
+constexpr gfx::Insets kIconMediaNotificationHeaderInsets =
+    gfx::Insets(6, 0, 0, 6);
 constexpr gfx::Size kMediaNotificationButtonRowSize =
     gfx::Size(124, kMediaButtonSize.height());
 
@@ -100,7 +101,8 @@ MediaNotificationView::MediaNotificationView(
     base::WeakPtr<MediaNotificationItem> item,
     views::View* header_row_controls_view,
     const base::string16& default_app_name,
-    int notification_width)
+    int notification_width,
+    bool should_show_icon)
     : container_(container),
       item_(std::move(item)),
       header_row_controls_view_(header_row_controls_view),
@@ -118,12 +120,17 @@ MediaNotificationView::MediaNotificationView(
     header_row->AddChildView(header_row_controls_view_);
 
   header_row->SetAppName(default_app_name_);
-  header_row->ClearAppIcon();
-  header_row->SetProperty(
-      views::kMarginsKey,
-      gfx::Insets(kMediaNotificationHeaderTopInset,
-                  kMediaNotificationHeaderInset, kMediaNotificationHeaderInset,
-                  kMediaNotificationHeaderRightInset));
+
+  if (should_show_icon) {
+    header_row->ClearAppIcon();
+    header_row->SetProperty(views::kMarginsKey,
+                            kIconMediaNotificationHeaderInsets);
+  } else {
+    header_row->HideAppIcon();
+    header_row->SetProperty(views::kMarginsKey,
+                            kIconlessMediaNotificationHeaderInsets);
+  }
+
   header_row_ = AddChildView(std::move(header_row));
 
   // |main_row_| holds the main content of the notification.

diff --git a/components/media_message_center/media_notification_view.h b/components/media_message_center/media_notification_view.h
@@ -66,7 +66,8 @@ class COMPONENT_EXPORT(MEDIA_MESSAGE_CENTER) MediaNotificationView
                         base::WeakPtr<MediaNotificationItem> item,
                         views::View* header_row_controls_view,
                         const base::string16& default_app_name,
-                        int notification_width);
+                        int notification_width,
+                        bool should_show_icon);
   ~MediaNotificationView() override;
 
   void SetExpanded(bool expanded);

diff --git a/components/media_message_center/media_notification_view_unittest.cc b/components/media_message_center/media_notification_view_unittest.cc
@@ -204,8 +204,13 @@ class MediaNotificationViewTest : public views::ViewsTestBase {
     return media_controller_.get();
   }
 
+  message_center::NotificationHeaderView* GetHeaderRow(
+      MediaNotificationView* view) const {
+    return view->header_row_;
+  }
+
   message_center::NotificationHeaderView* header_row() const {
-    return view()->header_row_;
+    return GetHeaderRow(view());
   }
 
   const base::string16& accessible_name() const {
@@ -306,7 +311,8 @@ class MediaNotificationViewTest : public views::ViewsTestBase {
     auto view = std::make_unique<MediaNotificationView>(
         &container_, item_->GetWeakPtr(),
         nullptr /* header_row_controls_view */,
-        base::ASCIIToUTF16(kTestDefaultAppName), kViewWidth);
+        base::ASCIIToUTF16(kTestDefaultAppName), kViewWidth,
+        /*should_show_icon=*/true);
     view->SetSize(kViewSize);
     view->set_owned_by_client();
 
@@ -1282,4 +1288,18 @@ TEST_F(MAYBE_MediaNotificationViewTest, ForcedExpandedState) {
   EXPECT_TRUE(IsActuallyExpanded());
 }
 
+TEST_F(MAYBE_MediaNotificationViewTest, AllowsHidingOfAppIcon) {
+  MediaNotificationView shows_icon(&container(), nullptr, nullptr,
+                                   base::string16(), kViewWidth,
+                                   /*should_show_icon=*/true);
+  MediaNotificationView hides_icon(&container(), nullptr, nullptr,
+                                   base::string16(), kViewWidth,
+                                   /*should_show_icon=*/false);
+
+  EXPECT_TRUE(
+      GetHeaderRow(&shows_icon)->app_icon_view_for_testing()->GetVisible());
+  EXPECT_FALSE(
+      GetHeaderRow(&hides_icon)->app_icon_view_for_testing()->GetVisible());
+}
+
 }  // namespace media_message_center
diff --git a/ui/message_center/views/notification_header_view.cc b/ui/message_center/views/notification_header_view.cc
@@ -385,6 +385,10 @@ void NotificationHeaderView::SetSubpixelRenderingEnabled(bool enabled) {
   timestamp_view_->SetSubpixelRenderingEnabled(enabled);
 }
 
+void NotificationHeaderView::HideAppIcon() {
+  app_icon_view_->SetVisible(false);
+}
+
 const base::string16& NotificationHeaderView::app_name_for_testing() const {
   return app_name_view_->GetText();
 }

diff --git a/ui/message_center/views/notification_header_view.h b/ui/message_center/views/notification_header_view.h
@@ -53,6 +53,9 @@ class MESSAGE_CENTER_EXPORT NotificationHeaderView : public views::Button {
   void ClearProgress();
   void SetSubpixelRenderingEnabled(bool enabled);
 
+  // Completely hides the app icon.
+  void HideAppIcon();
+
   // views::View:
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override;
 
@@ -64,6 +67,10 @@ class MESSAGE_CENTER_EXPORT NotificationHeaderView : public views::Button {
     return summary_text_view_;
   }
 
+  const views::ImageView* app_icon_view_for_testing() const {
+    return app_icon_view_;
+  }
+
   const base::string16& app_name_for_testing() const;
 
   const gfx::ImageSkia& app_icon_for_testing() const;

diff --git a/ui/message_center/views/notification_header_view_unittest.cc b/ui/message_center/views/notification_header_view_unittest.cc
@@ -9,6 +9,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/strings/grit/ui_strings.h"
+#include "ui/views/controls/image_view.h"
 #include "ui/views/test/views_test_base.h"
 
 namespace message_center {
@@ -82,4 +83,15 @@ TEST_F(NotificationHeaderViewTest, UpdatesTimestampOverTime) {
             notification_header_view_->timestamp_for_testing());
 }
 
+TEST_F(NotificationHeaderViewTest, AllowsHidingOfAppIcon) {
+  // The icon should be shown by default.
+  EXPECT_TRUE(
+      notification_header_view_->app_icon_view_for_testing()->IsDrawn());
+
+  // Though it can be explicitly hidden.
+  notification_header_view_->HideAppIcon();
+  EXPECT_FALSE(
+      notification_header_view_->app_icon_view_for_testing()->IsDrawn());
+}
+
 }  // namespace message_center