Reconsider notification icon sizing in the Views implementation

Today, the Views implementation of notification toasts enforces a
minimum size of 40x40, allowing larger icons up to 80x80 unless the icon
has a transparant background.

This is different from the Cocoa implementation, which has no minimum
size and allows icons of up to 80x80 regardless of transparancy.

This CL adopts the behaviour of the Cocoa implementation in Views, which
makes more sense. It also renames the Notification.adjust_image() option
to something more appropriate.

Screenshots: http://imgur.com/qHyiTn6

BUG=587985

Review URL: https://codereview.chromium.org/1774893003

Cr-Commit-Position: refs/heads/master@{#381458}

6 files changed, 45 insertions, 84 deletions

diff --git a/chrome/browser/download/notification/download_item_notification.cc b/chrome/browser/download/notification/download_item_notification.cc
index 48cc7f5..996cf6c 100644
--- a/chrome/browser/download/notification/download_item_notification.cc
+++ b/chrome/browser/download/notification/download_item_notification.cc
@@ -194,7 +194,7 @@ DownloadItemNotification::DownloadItemNotification(
 
   notification_->set_progress(0);
   notification_->set_never_timeout(false);
-  notification_->set_adjust_icon(false);
+  notification_->set_draw_icon_background(false);
 
   Update();
 }
diff --git a/ui/message_center/notification.cc b/ui/message_center/notification.cc
index 7c76b2c..b0a379d 100644
--- a/ui/message_center/notification.cc
+++ b/ui/message_center/notification.cc
@@ -77,7 +77,7 @@ Notification::Notification(NotificationType type,
       title_(title),
       message_(message),
       icon_(icon),
-      adjust_icon_(true),
+      draw_icon_background_(true),
       display_source_(display_source),
       origin_url_(origin_url),
       notifier_id_(notifier_id),
@@ -93,7 +93,7 @@ Notification::Notification(const std::string& id, const Notification& other)
       title_(other.title_),
       message_(other.message_),
       icon_(other.icon_),
-      adjust_icon_(other.adjust_icon_),
+      draw_icon_background_(other.draw_icon_background_),
       display_source_(other.display_source_),
       origin_url_(other.origin_url_),
       notifier_id_(other.notifier_id_),
@@ -109,7 +109,7 @@ Notification::Notification(const Notification& other)
       title_(other.title_),
       message_(other.message_),
       icon_(other.icon_),
-      adjust_icon_(other.adjust_icon_),
+      draw_icon_background_(other.draw_icon_background_),
       display_source_(other.display_source_),
       origin_url_(other.origin_url_),
       notifier_id_(other.notifier_id_),
@@ -125,7 +125,7 @@ Notification& Notification::operator=(const Notification& other) {
   title_ = other.title_;
   message_ = other.message_;
   icon_ = other.icon_;
-  adjust_icon_ = other.adjust_icon_;
+  draw_icon_background_ = other.draw_icon_background_;
   display_source_ = other.display_source_;
   origin_url_ = other.origin_url_;
   notifier_id_ = other.notifier_id_;
diff --git a/ui/message_center/notification.h b/ui/message_center/notification.h
index 7bbcc8f..f06e92d 100644
--- a/ui/message_center/notification.h
+++ b/ui/message_center/notification.h
@@ -171,12 +171,10 @@ class MESSAGE_CENTER_EXPORT Notification {
   const gfx::Image& icon() const { return icon_; }
   void set_icon(const gfx::Image& icon) { icon_ = icon; }
 
-  // Gets and sets whether to adjust the icon before displaying. The adjustment
-  // is designed to accomodate legacy HTML icons but isn't necessary for
-  // Chrome's hardcoded notifications. NB: this is currently ignored outside of
-  // Views.
-  bool adjust_icon() const { return adjust_icon_; }
-  void set_adjust_icon(bool adjust) { adjust_icon_ = adjust; }
+  // Gets and sets whether to draw a solid background colour behind the
+  // notification's icon. Only applies to the Views implementation.
+  bool draw_icon_background() const { return draw_icon_background_; }
+  void set_draw_icon_background(bool draw) { draw_icon_background_ = draw; }
 
   const gfx::Image& image() const { return optional_fields_.image; }
   void set_image(const gfx::Image& image) { optional_fields_.image = image; }
@@ -270,9 +268,9 @@ class MESSAGE_CENTER_EXPORT Notification {
   // Image data for the associated icon, used by Ash when available.
   gfx::Image icon_;
 
-  // True by default; controls whether to apply adjustments such as BG color and
-  // size scaling to |icon_|.
-  bool adjust_icon_;
+  // True by default; controls whether to draw a solid background colour behind
+  // the |icon_|. Only applies to the Views implementation.
+  bool draw_icon_background_;
 
   // The display string for the source of the notification.  Could be
   // the same as origin_url_, or the name of an extension.
diff --git a/ui/message_center/views/constants.h b/ui/message_center/views/constants.h
index 2600d91..9942ed8 100644
--- a/ui/message_center/views/constants.h
+++ b/ui/message_center/views/constants.h
@@ -20,8 +20,6 @@ const SkColor kRegularTextBackgroundColor = SK_ColorWHITE;
 const SkColor kDimTextBackgroundColor = SK_ColorWHITE;
 const SkColor kContextTextBackgroundColor = SK_ColorWHITE;
 
-const int kIconSize = message_center::kNotificationIconSize;
-const int kLegacyIconSize = 40;
 const int kTextBottomPadding = 12;
 const int kItemTitleToMessagePadding = 3;
 const int kButtonVecticalPadding = 0;
diff --git a/ui/message_center/views/notification_view.cc b/ui/message_center/views/notification_view.cc
index 254fcbf..b1b346e 100644
--- a/ui/message_center/views/notification_view.cc
+++ b/ui/message_center/views/notification_view.cc
@@ -94,32 +94,6 @@ scoped_ptr<views::Border> MakeSeparatorBorder(int top,
   return views::Border::CreateSolidSidedBorder(top, left, 0, 0, color);
 }
 
-// static
-// Return true if and only if the image is null or has alpha.
-bool HasAlpha(gfx::ImageSkia& image, views::Widget* widget) {
-  // Determine which bitmap to use.
-  float factor = 1.0f;
-  if (widget)
-    factor = ui::GetScaleFactorForNativeView(widget->GetNativeView());
-
-  // Extract that bitmap's alpha and look for a non-opaque pixel there.
-  SkBitmap bitmap = image.GetRepresentation(factor).sk_bitmap();
-  if (!bitmap.isNull()) {
-    SkBitmap alpha;
-    bitmap.extractAlpha(&alpha);
-    for (int y = 0; y < bitmap.height(); ++y) {
-      for (int x = 0; x < bitmap.width(); ++x) {
-        if (alpha.getColor(x, y) != SK_ColorBLACK) {
-          return true;
-        }
-      }
-    }
-  }
-
-  // If no opaque pixel was found, return false unless the bitmap is empty.
-  return bitmap.isNull();
-}
-
 // ItemView ////////////////////////////////////////////////////////////////////
 
 // ItemViews are responsible for drawing each list notification item's title and
@@ -353,13 +327,16 @@ int NotificationView::GetHeightForWidth(int width) const {
     }
   }
 
-  int content_height = std::max(top_height, kIconSize) + bottom_height;
+  int content_height =
+      std::max(top_height, kNotificationIconSize) + bottom_height;
 
   // Adjust the height to make sure there is at least 16px of space below the
   // icon if there is any space there (<http://crbug.com/232966>).
-  if (content_height > kIconSize)
-    content_height = std::max(content_height,
-                              kIconSize + message_center::kIconBottomPadding);
+  if (content_height > kNotificationIconSize) {
+    content_height =
+        std::max(content_height,
+                 kNotificationIconSize + message_center::kIconBottomPadding);
+  }
 
   return content_height + GetInsets().height();
 }
@@ -393,10 +370,11 @@ void NotificationView::Layout() {
   }
 
   // Icon.
-  icon_view_->SetBounds(insets.left(), insets.top(), kIconSize, kIconSize);
+  icon_view_->SetBounds(insets.left(), insets.top(), kNotificationIconSize,
+                        kNotificationIconSize);
 
   // Settings & Bottom views.
-  int bottom_y = insets.top() + std::max(top_height, kIconSize);
+  int bottom_y = insets.top() + std::max(top_height, kNotificationIconSize);
   int bottom_height = bottom_view_->GetHeightForWidth(content_width);
 
   if (settings_button_view_) {
@@ -681,24 +659,20 @@ void NotificationView::CreateOrUpdateListItemViews(
 
 void NotificationView::CreateOrUpdateIconView(
     const Notification& notification) {
+  gfx::Size image_view_size(kNotificationIconSize, kNotificationIconSize);
+
   if (!icon_view_) {
-    icon_view_ = new ProportionalImageView(gfx::Size(kIconSize, kIconSize));
+    icon_view_ = new ProportionalImageView(image_view_size);
     AddChildView(icon_view_);
   }
 
   gfx::ImageSkia icon = notification.icon().AsImageSkia();
-  if (notification.adjust_icon()) {
+  icon_view_->SetImage(icon, icon.size());
+
+  if (notification.draw_icon_background()) {
     icon_view_->set_background(
         views::Background::CreateSolidBackground(kIconBackgroundColor));
-    gfx::Size max_image_size =
-        notification.type() == NOTIFICATION_TYPE_SIMPLE &&
-                (icon.width() < kIconSize || icon.height() < kIconSize ||
-                 HasAlpha(icon, GetWidget()))
-            ? gfx::Size(kLegacyIconSize, kLegacyIconSize)
-            : gfx::Size(kIconSize, kIconSize);
-    icon_view_->SetImage(icon, max_image_size);
   } else {
-    icon_view_->SetImage(icon, icon.size());
     icon_view_->set_background(nullptr);
   }
 }
diff --git a/ui/message_center/views/notification_view_unittest.cc b/ui/message_center/views/notification_view_unittest.cc
index bcaf421..9be249c 100644
--- a/ui/message_center/views/notification_view_unittest.cc
+++ b/ui/message_center/views/notification_view_unittest.cc
@@ -355,42 +355,33 @@ TEST_F(NotificationViewTest, TestIconSizing) {
   notification()->set_type(NOTIFICATION_TYPE_SIMPLE);
   ProportionalImageView* view = notification_view()->icon_view_;
 
-  // Icons smaller than the legacy size should be scaled up to it.
-  notification()->set_icon(CreateTestImage(kLegacyIconSize / 2,
-                                           kLegacyIconSize / 2));
+  // Icons smaller than the maximum size should remain unscaled.
+  notification()->set_icon(CreateTestImage(kNotificationIconSize / 2,
+                                           kNotificationIconSize / 4));
   UpdateNotificationViews();
-  EXPECT_EQ(gfx::Size(kLegacyIconSize, kLegacyIconSize).ToString(),
+  EXPECT_EQ(gfx::Size(kNotificationIconSize / 2,
+                      kNotificationIconSize / 4).ToString(),
             GetImagePaintSize(view).ToString());
 
-  // Icons at the legacy size should be unscaled.
-  notification()->set_icon(CreateTestImage(kLegacyIconSize, kLegacyIconSize));
+  // Icons of exactly the intended icon size should remain unscaled.
+  notification()->set_icon(CreateTestImage(kNotificationIconSize,
+                                           kNotificationIconSize));
   UpdateNotificationViews();
-  EXPECT_EQ(gfx::Size(kLegacyIconSize, kLegacyIconSize).ToString(),
+  EXPECT_EQ(gfx::Size(kNotificationIconSize, kNotificationIconSize).ToString(),
             GetImagePaintSize(view).ToString());
 
-  // Icons slightly smaller than the preferred size should be scaled down to the
-  // legacy size to avoid having tiny borders (http://crbug.com/232966).
-  notification()->set_icon(CreateTestImage(kIconSize - 1, kIconSize - 1));
+  // Icons over the maximum size should be scaled down, maintaining proportions.
+  notification()->set_icon(CreateTestImage(2 * kNotificationIconSize,
+                                           2 * kNotificationIconSize));
   UpdateNotificationViews();
-  EXPECT_EQ(gfx::Size(kLegacyIconSize, kLegacyIconSize).ToString(),
+  EXPECT_EQ(gfx::Size(kNotificationIconSize, kNotificationIconSize).ToString(),
             GetImagePaintSize(view).ToString());
 
-  // Icons at the preferred size or above should be scaled down to the preferred
-  // size.
-  notification()->set_icon(CreateTestImage(kIconSize, kIconSize));
+  notification()->set_icon(CreateTestImage(4 * kNotificationIconSize,
+                                           2 * kNotificationIconSize));
   UpdateNotificationViews();
-  EXPECT_EQ(gfx::Size(kIconSize, kIconSize).ToString(),
-            GetImagePaintSize(view).ToString());
-
-  notification()->set_icon(CreateTestImage(2 * kIconSize, 2 * kIconSize));
-  UpdateNotificationViews();
-  EXPECT_EQ(gfx::Size(kIconSize, kIconSize).ToString(),
-            GetImagePaintSize(view).ToString());
-
-  // Large, non-square images' aspect ratios should be preserved.
-  notification()->set_icon(CreateTestImage(4 * kIconSize, 2 * kIconSize));
-  UpdateNotificationViews();
-  EXPECT_EQ(gfx::Size(kIconSize, kIconSize / 2).ToString(),
+  EXPECT_EQ(gfx::Size(kNotificationIconSize,
+                      kNotificationIconSize / 2).ToString(),
             GetImagePaintSize(view).ToString());
 }