Linux native notifications: Support image notifications

chrome/browser/notifications/notification_platform_bridge_linux.cc

Index: chrome/browser/notifications/notification_platform_bridge_linux.cc
diff --git a/chrome/browser/notifications/notification_platform_bridge_linux.cc b/chrome/browser/notifications/notification_platform_bridge_linux.cc
index 79ae1651465dda83865f2b41dd0974b01ced8478..9e6e0739d61f2ab1708a7d1ecea093931052581a 100644
--- a/chrome/browser/notifications/notification_platform_bridge_linux.cc
+++ b/chrome/browser/notifications/notification_platform_bridge_linux.cc
@@ -34,6 +34,7 @@
 #include "dbus/bus.h"
 #include "dbus/message.h"
 #include "dbus/object_proxy.h"
+#include "skia/ext/image_operations.h"
 #include "ui/gfx/image/image_skia.h"
 
 namespace {
@@ -44,6 +45,10 @@ const char kFreedesktopNotificationsPath[] = "/org/freedesktop/Notifications";
 const char kDefaultButtonId[] = "default";
 const char kSettingsButtonId[] = "settings";
 
+// Specified in FDO notification specification.
+const int kMaxImageWidth = 200;
+const int kMaxImageHeight = 100;

[Peter Beverloo, 2017/05/10 12:54:40]

This is tiny! Developers have to provide images with a width of 1440 pixels for
modern phones. Screen density makes a huge difference here, so it'd be
interesting to learn how this'll work out in practice.

[Tom (Use chromium acct), 2017/05/11 00:07:04]

Yeah, but the only notification server that actually supports inline images
(LXQt notification daemon) sort of goes crazy when you give it a larger image.

+
 // The values in this enumeration correspond to those of the
 // Linux.NotificationPlatformBridge.InitializationStatus histogram, so
 // the ordering should not be changed.  New error codes should be
@@ -57,6 +62,14 @@ enum class ConnectionInitializationStatusCode {
   NUM_ITEMS
 };
 
+int ClampInt(int v, int lo, int hi) {
+  if (v < lo)
+    return lo;
+  if (v > hi)
+    return hi;
+  return v;
+}
+
 gfx::Image DeepCopyImage(const gfx::Image& image) {
   if (image.IsEmpty())
     return gfx::Image();
@@ -198,7 +211,24 @@ class NotificationPlatformBridgeLinuxImpl
     // non-thread-safe reference counts) to the task runner thread.
     auto notification_copy = base::MakeUnique<Notification>(notification);
     notification_copy->set_icon(DeepCopyImage(notification_copy->icon()));
-    notification_copy->set_image(gfx::Image());
+    if (body_images_supported_ != BODY_IMAGES_NOT_SUPPORTED &&
+        !notification_copy->image().IsEmpty()) {
+      int width = notification_copy->image().Width();
+      int height = notification_copy->image().Height();
+      if (width <= kMaxImageWidth && height <= kMaxImageHeight) {
+        notification_copy->set_image(DeepCopyImage(notification_copy->image()));
+      } else {
+        const SkBitmap* image_bitmap = notification_copy->image().ToSkBitmap();
+        double scale = std::min(static_cast<double>(kMaxImageWidth) / width,
+                                static_cast<double>(kMaxImageHeight) / height);
+        width = ClampInt(scale * width, 1, kMaxImageWidth);
+        height = ClampInt(scale * height, 1, kMaxImageHeight);
+        notification_copy->set_image(gfx::Image(
+            gfx::ImageSkia::CreateFrom1xBitmap(skia::ImageOperations::Resize(
+                *image_bitmap, skia::ImageOperations::RESIZE_LANCZOS3, width,
+                height))));

[Peter Beverloo, 2017/05/10 12:54:40]

Since these images potentially can be huge, we shouldn't do this on the UI
thread. Can the task runner thread be used for the resize? That'd avoid the
multi-thread warning at the member declaration site below.

[Tom (Use chromium acct), 2017/05/11 00:07:04]

Done.

+      }
+    }
     notification_copy->set_small_image(gfx::Image());
     for (size_t i = 0; i < notification_copy->buttons().size(); i++)
       notification_copy->SetButtonIcon(i, gfx::Image());
@@ -349,6 +379,9 @@ class NotificationPlatformBridgeLinuxImpl
         capabilities_.insert(capability);
     }
     RecordMetricsForCapabilities();
+    body_images_supported_ = base::ContainsKey(capabilities_, "body-images")
+                                 ? BODY_IMAGES_SUPPORTED
+                                 : BODY_IMAGES_NOT_SUPPORTED;
 
     dbus::MethodCall get_server_information_call(kFreedesktopNotificationsName,
                                                  "GetServerInformation");
@@ -457,6 +490,17 @@ class NotificationPlatformBridgeLinuxImpl
           else
             body += title + " - " + message;
         }
+      } else if (notification->type() ==
+                 message_center::NOTIFICATION_TYPE_IMAGE) {
+        std::unique_ptr<ResourceFile> image_file =
+            WriteDataToTmpFile(notification->image().As1xPNGBytes());
+        if (image_file) {
+          if (!body.empty())
+            body += "\n";
+          body +=
+              "<img src=\"" + image_file->file_path().value() + "\" alt=\"\"/>";

[Peter Beverloo, 2017/05/10 12:54:40]

Is a data: URL an option here? If the toast requires full HTML rendering anyway
I wouldn't expect that to be significantly slower.

[Tom (Use chromium acct), 2017/05/11 00:07:04]

No, the spec says "Images referenced must always be local files."

Also: "A full-blown HTML implementation is not required of this spec, and
notifications should never take advantage of tags that are not listed above. As
notifications are not a substitute for web browsers or complex dialogs, advanced
layout is not necessary, and may in fact limit the number of systems that
notification services can run on, due to memory usage and screen space. Such
examples are PDAs, certain cell phones, and slow PCs or laptops with little
memory."

+          data->resource_files.push_back(std::move(image_file));
+        }
       }
     }
     writer.AppendString(body);
@@ -768,6 +812,19 @@ class NotificationPlatformBridgeLinuxImpl
 
   UnorderedUniqueSet<NotificationData> notifications_;
 
+  //////////////////////////////////////////////////////////////////////////////
+  // Members used on both the UI thread and the task runner thread.
+
+  // Notification servers very rarely have the 'body-images'
+  // capability, so try to avoid an image copy/resize if possible.
+  // Reads and writes to |body_images_supported_| should be atomic, so
+  // no locking is necessary.
+  enum {
+    BODY_IMAGES_UNKNOWN,
+    BODY_IMAGES_SUPPORTED,
+    BODY_IMAGES_NOT_SUPPORTED,
+  } body_images_supported_ = BODY_IMAGES_UNKNOWN;

[Peter Beverloo, 2017/05/10 12:54:40]

It looks like you're considering base::Optional<bool> semantics here? Could even
simplify that and just default to false until line 382 executes.

[Tom (Use chromium acct), 2017/05/11 00:07:04]

Done.

+
   DISALLOW_COPY_AND_ASSIGN(NotificationPlatformBridgeLinuxImpl);
 };