lacros: Support high DPI notification icons and images

Convert icons and images to use the newly-stabilized ImageSkia mojo
transport. This allows high DPI images. It is also higher performance
than the existing crosapi::Bitmap serializer because it uses shared
memory for large images.

Lacros still sends icons and images in the old format to allow
backwards compatibility with older ash. We can remove this old
serialization in 6 weeks or so. Until then we won't see the
performance benefit.

Manually tested on gLinux with high-DPI screen.

Bug: 1113889
Test: added to unit_tests
Change-Id: I1cf4b86512fd59e287a32dfb4ac2d59d2962a6f4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2388152Reviewed-by: Jun Mukai <mukai@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Reviewed-by: Greg Kerr <kerrnel@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Auto-Submit: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803711}

chrome/browser/chromeos/crosapi/message_center_ash.cc

@@ -11,12 +11,11 @@
 #include "base/memory/scoped_refptr.h"
 #include "base/numerics/ranges.h"
 #include "base/optional.h"
-#include "chromeos/crosapi/cpp/bitmap.h"
-#include "chromeos/crosapi/cpp/bitmap_util.h"
 #include "chromeos/crosapi/mojom/message_center.mojom.h"
 #include "chromeos/crosapi/mojom/notification.mojom.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/remote.h"
+#include "ui/gfx/image/image.h"
 #include "ui/message_center/message_center.h"
 #include "ui/message_center/public/cpp/notification.h"
 #include "url/gurl.h"
@@ -48,22 +47,16 @@ mc::FullscreenVisibility FromMojo(mojom::FullscreenVisibility visibility) {
   }
 }
 
-gfx::Image ImageFromBitmap(const crosapi::Bitmap& bitmap) {
-  SkBitmap sk_bitmap = crosapi::SkBitmapFromBitmap(bitmap);
-  // TODO(https://crbug.com/1113889): High DPI support.
-  return gfx::Image::CreateFrom1xBitmap(sk_bitmap);
-}
-
 std::unique_ptr<mc::Notification> FromMojo(
     mojom::NotificationPtr notification) {
   mc::RichNotificationData rich_data;
   rich_data.priority = base::ClampToRange(notification->priority, -2, 2);
   rich_data.never_timeout = notification->require_interaction;
   rich_data.timestamp = notification->timestamp;
-  if (notification->image)
-    rich_data.image = ImageFromBitmap(notification->image.value());
-  if (notification->badge)
-    rich_data.small_image = ImageFromBitmap(notification->badge.value());
+  if (!notification->image.isNull())
+    rich_data.image = gfx::Image(notification->image);
+  if (!notification->badge.isNull())
+    rich_data.small_image = gfx::Image(notification->badge);
   for (const auto& mojo_item : notification->items) {
     mc::NotificationItem item;
     item.title = mojo_item->title;
@@ -85,8 +78,8 @@ std::unique_ptr<mc::Notification> FromMojo(
       FromMojo(notification->fullscreen_visibility);
 
   gfx::Image icon;
-  if (notification->icon)
-    icon = ImageFromBitmap(notification->icon.value());
+  if (!notification->icon.isNull())
+    icon = gfx::Image(notification->icon);
   GURL origin_url = notification->origin_url.value_or(GURL());
   // TODO(crbug.com/1113889): NotifierId support.
   return std::make_unique<mc::Notification>(

chrome/browser/chromeos/crosapi/message_center_ash_unittest.cc

@@ -8,39 +8,26 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/task_environment.h"
 #include "base/time/time.h"
-#include "cc/test/pixel_comparator.h"
-#include "chromeos/crosapi/cpp/bitmap_util.h"
 #include "chromeos/crosapi/mojom/message_center.mojom.h"
 #include "chromeos/crosapi/mojom/notification.mojom.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkColor.h"
+#include "ui/gfx/image/image_skia.h"
+#include "ui/gfx/image/image_unittest_util.h"
 #include "ui/message_center/message_center.h"
 #include "ui/message_center/public/cpp/notification.h"
 #include "ui/message_center/public/cpp/notification_delegate.h"
 #include "url/gurl.h"
 
 using base::ASCIIToUTF16;
+using gfx::test::AreBitmapsEqual;
+using gfx::test::AreImagesEqual;
 
 namespace crosapi {
 namespace {
 
-// Returns a bitmap filled with red.
-SkBitmap CreateRedBitmap(int width, int height) {
-  SkBitmap bitmap;
-  bitmap.allocN32Pixels(width, height);
-  bitmap.eraseColor(SK_ColorRED);
-  return bitmap;
-}
-
-bool BitmapEquals(const SkBitmap& a, const SkBitmap& b) {
-  if (a.width() != b.width() || a.height() != b.height())
-    return false;
-  cc::ExactPixelComparator comparator(/*discard_alpha=*/false);
-  return comparator.Compare(a, b);
-}
-
 class MojoDelegate : public mojom::NotificationDelegate {
  public:
   MojoDelegate() = default;
@@ -113,10 +100,10 @@ TEST_F(MessageCenterAshTest, SerializationSimple) {
   mojo_notification->fullscreen_visibility =
       mojom::FullscreenVisibility::kOverUser;
 
-  SkBitmap test_badge = CreateRedBitmap(1, 2);
-  mojo_notification->badge = BitmapFromSkBitmap(test_badge);
-  SkBitmap test_icon = CreateRedBitmap(3, 4);
-  mojo_notification->icon = BitmapFromSkBitmap(test_icon);
+  SkBitmap test_badge = gfx::test::CreateBitmap(1, 2);
+  mojo_notification->badge = gfx::ImageSkia::CreateFrom1xBitmap(test_badge);
+  SkBitmap test_icon = gfx::test::CreateBitmap(3, 4);
+  mojo_notification->icon = gfx::ImageSkia::CreateFrom1xBitmap(test_icon);
 
   auto button1 = mojom::ButtonInfo::New();
   button1->title = ASCIIToUTF16("button1");
@@ -152,8 +139,8 @@ TEST_F(MessageCenterAshTest, SerializationSimple) {
             ui_notification->fullscreen_visibility());
 
   EXPECT_TRUE(
-      BitmapEquals(test_badge, ui_notification->small_image().AsBitmap()));
-  EXPECT_TRUE(BitmapEquals(test_icon, ui_notification->icon().AsBitmap()));
+      AreBitmapsEqual(test_badge, ui_notification->small_image().AsBitmap()));
+  EXPECT_TRUE(AreBitmapsEqual(test_icon, ui_notification->icon().AsBitmap()));
 
   ASSERT_EQ(2u, ui_notification->buttons().size());
   EXPECT_EQ(ASCIIToUTF16("button1"), ui_notification->buttons()[0].title);
@@ -166,8 +153,8 @@ TEST_F(MessageCenterAshTest, SerializationImage) {
   mojo_notification->type = mojom::NotificationType::kImage;
   mojo_notification->id = "test_id";
 
-  SkBitmap test_image = CreateRedBitmap(5, 6);
-  mojo_notification->image = BitmapFromSkBitmap(test_image);
+  SkBitmap test_image = gfx::test::CreateBitmap(5, 6);
+  mojo_notification->image = gfx::ImageSkia::CreateFrom1xBitmap(test_image);
 
   // Display the notification.
   MojoDelegate mojo_delegate;
@@ -181,7 +168,34 @@ TEST_F(MessageCenterAshTest, SerializationImage) {
   message_center::Notification* ui_notification =
       message_center->FindVisibleNotificationById("test_id");
   ASSERT_TRUE(ui_notification);
-  EXPECT_TRUE(BitmapEquals(test_image, ui_notification->image().AsBitmap()));
+  EXPECT_TRUE(AreBitmapsEqual(test_image, ui_notification->image().AsBitmap()));
+}
+
+TEST_F(MessageCenterAshTest, HighDpiImage) {
+  // Create a notification with an image.
+  auto mojo_notification = mojom::Notification::New();
+  mojo_notification->type = mojom::NotificationType::kImage;
+  mojo_notification->id = "test_id";
+
+  // Create a high DPI image.
+  SkBitmap bitmap = gfx::test::CreateBitmap(2, 4);
+  gfx::ImageSkia high_dpi_image_skia(gfx::ImageSkiaRep(bitmap, 2.0f));
+  mojo_notification->image = high_dpi_image_skia;
+
+  // Display the notification.
+  MojoDelegate mojo_delegate;
+  message_center_remote_->DisplayNotification(
+      std::move(mojo_notification),
+      mojo_delegate.receiver_.BindNewPipeAndPassRemote());
+  message_center_remote_.FlushForTesting();
+
+  // Notification exists and has the high DPI image.
+  auto* message_center = message_center::MessageCenter::Get();
+  message_center::Notification* ui_notification =
+      message_center->FindVisibleNotificationById("test_id");
+  ASSERT_TRUE(ui_notification);
+  EXPECT_TRUE(AreImagesEqual(gfx::Image(high_dpi_image_skia),
+                             ui_notification->image()));
 }
 
 TEST_F(MessageCenterAshTest, SerializationList) {

chrome/browser/notifications/notification_platform_bridge_lacros.cc

@@ -60,19 +60,28 @@ crosapi::mojom::NotificationPtr ToMojo(
   mojo_note->display_source = notification.display_source();
   mojo_note->origin_url = notification.origin_url();
   if (!notification.icon().IsEmpty()) {
+    // TODO(https://crbug.com/1123969): Don't send the deprecated field after
+    // ash M87 beta.
     SkBitmap icon = notification.icon().AsBitmap();
-    mojo_note->icon = crosapi::BitmapFromSkBitmap(icon);
+    mojo_note->deprecated_icon = crosapi::BitmapFromSkBitmap(icon);
+    mojo_note->icon = notification.icon().AsImageSkia();
   }
   mojo_note->priority = base::ClampToRange(notification.priority(), -2, 2);
   mojo_note->require_interaction = notification.never_timeout();
   mojo_note->timestamp = notification.timestamp();
   if (!notification.image().IsEmpty()) {
+    // TODO(https://crbug.com/1123969): Don't send the deprecated field after
+    // ash M87 beta.
     SkBitmap image = notification.image().AsBitmap();
-    mojo_note->image = crosapi::BitmapFromSkBitmap(image);
+    mojo_note->deprecated_image = crosapi::BitmapFromSkBitmap(image);
+    mojo_note->image = notification.image().AsImageSkia();
   }
   if (!notification.small_image().IsEmpty()) {
+    // TODO(https://crbug.com/1123969): Don't send the deprecated field after
+    // ash M87 beta.
     SkBitmap badge = notification.small_image().AsBitmap();
-    mojo_note->badge = crosapi::BitmapFromSkBitmap(badge);
+    mojo_note->deprecated_badge = crosapi::BitmapFromSkBitmap(badge);
+    mojo_note->badge = notification.small_image().AsImageSkia();
   }
   for (const auto& item : notification.items()) {
     auto mojo_item = crosapi::mojom::NotificationItem::New();

chrome/browser/notifications/notification_platform_bridge_lacros_unittest.cc

@@ -6,7 +6,6 @@
 
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
-#include "cc/test/pixel_comparator.h"
 #include "chrome/browser/notifications/notification_platform_bridge_delegate.h"
 #include "chromeos/crosapi/cpp/bitmap.h"
 #include "chromeos/crosapi/cpp/bitmap_util.h"
@@ -16,29 +15,19 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkColor.h"
+#include "ui/gfx/image/image.h"
+#include "ui/gfx/image/image_skia_rep.h"
+#include "ui/gfx/image/image_unittest_util.h"
 #include "ui/message_center/public/cpp/notification.h"
 #include "ui/message_center/public/cpp/notification_types.h"
 #include "url/gurl.h"
 
 using base::ASCIIToUTF16;
+using gfx::test::AreBitmapsEqual;
+using gfx::test::AreImagesEqual;
 
 namespace {
 
-// Returns an image filled with red.
-gfx::Image CreateRedImage(int width, int height) {
-  SkBitmap bitmap;
-  bitmap.allocN32Pixels(width, height);
-  bitmap.eraseColor(SK_ColorRED);
-  return gfx::Image::CreateFrom1xBitmap(bitmap);
-}
-
-bool BitmapEquals(const SkBitmap& a, const SkBitmap& b) {
-  if (a.width() != b.width() || a.height() != b.height())
-    return false;
-  cc::ExactPixelComparator comparator(/*discard_alpha=*/false);
-  return comparator.Compare(a, b);
-}
-
 // Tracks user actions that would be passed into chrome's cross-platform
 // notification subsystem.
 class TestPlatformBridgeDelegate : public NotificationPlatformBridgeDelegate {
@@ -143,9 +132,14 @@ TEST_F(NotificationPlatformBridgeLacrosTest, SerializationSimple) {
   rich_data.fullscreen_visibility =
       message_center::FullscreenVisibility::OVER_USER;
 
-  gfx::Image badge = CreateRedImage(1, 2);
+  // Create badge and icon with both low DPI and high DPI versions.
+  gfx::Image badge = gfx::test::CreateImage(1, 2);
+  badge.AsImageSkia().AddRepresentation(
+      gfx::ImageSkiaRep(gfx::test::CreateBitmap(2, 4), /*scale=*/2.0f));
   rich_data.small_image = badge;
-  gfx::Image icon = CreateRedImage(3, 4);
+  gfx::Image icon = gfx::test::CreateImage(3, 4);
+  icon.AsImageSkia().AddRepresentation(
+      gfx::ImageSkiaRep(gfx::test::CreateBitmap(6, 8), /*scale=*/2.0f));
 
   message_center::ButtonInfo button1;
   button1.title = ASCIIToUTF16("button1");
@@ -181,12 +175,26 @@ TEST_F(NotificationPlatformBridgeLacrosTest, SerializationSimple) {
   EXPECT_EQ(crosapi::mojom::FullscreenVisibility::kOverUser,
             last_notification->fullscreen_visibility);
 
-  ASSERT_TRUE(last_notification->badge.has_value());
-  EXPECT_TRUE(BitmapEquals(badge.AsBitmap(), crosapi::SkBitmapFromBitmap(
-                                                 *last_notification->badge)));
-  ASSERT_TRUE(last_notification->icon.has_value());
-  EXPECT_TRUE(BitmapEquals(
-      icon.AsBitmap(), crosapi::SkBitmapFromBitmap(*last_notification->icon)));
+  // TODO(https://crbug.com/1123969): Don't test the deprecated field after
+  // ash M87 beta.
+  ASSERT_TRUE(last_notification->deprecated_badge.has_value());
+  EXPECT_TRUE(AreBitmapsEqual(
+      badge.AsBitmap(),
+      crosapi::SkBitmapFromBitmap(*last_notification->deprecated_badge)));
+  ASSERT_TRUE(last_notification->deprecated_icon.has_value());
+  EXPECT_TRUE(AreBitmapsEqual(
+      icon.AsBitmap(),
+      crosapi::SkBitmapFromBitmap(*last_notification->deprecated_icon)));
+
+  ASSERT_FALSE(last_notification->badge.isNull());
+  EXPECT_TRUE(last_notification->badge.HasRepresentation(1.0f));
+  EXPECT_TRUE(last_notification->badge.HasRepresentation(2.0f));
+  EXPECT_TRUE(AreImagesEqual(badge, gfx::Image(last_notification->badge)));
+
+  ASSERT_FALSE(last_notification->icon.isNull());
+  EXPECT_TRUE(last_notification->icon.HasRepresentation(1.0f));
+  EXPECT_TRUE(last_notification->icon.HasRepresentation(2.0f));
+  EXPECT_TRUE(AreImagesEqual(icon, gfx::Image(last_notification->icon)));
 
   ASSERT_EQ(2u, last_notification->buttons.size());
   EXPECT_EQ(ASCIIToUTF16("button1"), last_notification->buttons[0]->title);
@@ -195,7 +203,7 @@ TEST_F(NotificationPlatformBridgeLacrosTest, SerializationSimple) {
 
 TEST_F(NotificationPlatformBridgeLacrosTest, SerializationImage) {
   // Create a message_center notification.
-  gfx::Image image = CreateRedImage(5, 6);
+  gfx::Image image = gfx::test::CreateImage(5, 6);
   message_center::RichNotificationData rich_data;
   rich_data.image = image;
   message_center::Notification ui_notification(
@@ -212,9 +220,14 @@ TEST_F(NotificationPlatformBridgeLacrosTest, SerializationImage) {
   crosapi::mojom::Notification* last_notification =
       test_message_center_.last_notification_.get();
   ASSERT_TRUE(last_notification);
-  ASSERT_TRUE(last_notification->image.has_value());
-  EXPECT_TRUE(BitmapEquals(image.AsBitmap(), crosapi::SkBitmapFromBitmap(
-                                                 *last_notification->image)));
+  // TODO(https://crbug.com/1123969): Don't test the deprecated field after
+  // ash M87 beta.
+  ASSERT_TRUE(last_notification->deprecated_image.has_value());
+  EXPECT_TRUE(AreBitmapsEqual(
+      image.AsBitmap(),
+      crosapi::SkBitmapFromBitmap(*last_notification->deprecated_image)));
+  ASSERT_FALSE(last_notification->image.isNull());
+  EXPECT_TRUE(AreImagesEqual(image, gfx::Image(last_notification->image)));
 }
 
 TEST_F(NotificationPlatformBridgeLacrosTest, SerializationList) {

chromeos/crosapi/mojom/BUILD.gn

@@ -36,6 +36,7 @@ mojom("mojom") {
   public_deps = [
     "//mojo/public/mojom/base",
     "//services/device/public/mojom:mojom",
+    "//ui/gfx/image/mojom",
     "//url/mojom:url_mojom_gurl",
   ]
 }

chromeos/crosapi/mojom/notification.mojom

@@ -7,6 +7,7 @@ module crosapi.mojom;
 import "chromeos/crosapi/mojom/bitmap.mojom";
 import "mojo/public/mojom/base/string16.mojom";
 import "mojo/public/mojom/base/time.mojom";
+import "ui/gfx/image/mojom/image.mojom";
 import "url/mojom/url.mojom";
 
 // Type of notification to show. See the chrome.notifications extension API:
@@ -76,8 +77,8 @@ struct Notification {
   // notification. Otherwise empty.
   url.mojom.Url? origin_url;
 
-  // Icon for the notification.
-  Bitmap? icon;
+  // DEPRECATED, replaced with |icon| below.
+  Bitmap? deprecated_icon;
 
   // Priority from -2 to 2. Zero is the default. Other values are clamped.
   int32 priority;
@@ -89,11 +90,11 @@ struct Notification {
   // See web platform API "timestamp" and chrome.notifications "eventTime".
   mojo_base.mojom.Time timestamp;
 
-  // For type kImage, the large image.
-  Bitmap? image;
+  // DEPRECATED, replaced with |image| below.
+  Bitmap? deprecated_image;
 
-  // Badge to show the source of the notification (e.g. a 16x16 browser icon).
-  Bitmap? badge;
+  // DEPRECATED, replaced with |badge| below.
+  Bitmap? deprecated_badge;
 
   // Item list for type kList. Not displayed for other types.
   array<NotificationItem> items;
@@ -128,4 +129,16 @@ struct Notification {
 
   // Whether to show on top of fullscreen windows. See enum definition.
   FullscreenVisibility fullscreen_visibility;
+
+  // Icon for the notification.
+  [MinVersion=1]
+  gfx.mojom.ImageSkia? icon;
+
+  // For type kImage, the large image.
+  [MinVersion=1]
+  gfx.mojom.ImageSkia? image;
+
+  // Badge to show the source of the notification (e.g. a 16x16 browser icon).
+  [MinVersion=1]
+  gfx.mojom.ImageSkia? badge;
 };