Notifications: Split up image loading histograms by image type

third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp

Index: third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
diff --git a/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp b/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
index d85371c71891ce433295ba04a6b91bd7e5e3391d..59fbd351741bb8f08cf4ca945149d6128b2196b3 100644
--- a/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
+++ b/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
@@ -14,11 +14,30 @@
 #include "platform/network/ResourceRequest.h"
 #include "platform/weborigin/KURL.h"
 #include "public/platform/WebURLRequest.h"
-#include "third_party/skia/include/core/SkBitmap.h"
+#include "public/platform/modules/notifications/WebNotificationConstants.h"
+#include "skia/ext/image_operations.h"
 #include "wtf/CurrentTime.h"
 #include "wtf/Threading.h"
 #include <memory>
 
+#define NOTIFICATION_PER_TYPE_HISTOGRAM_COUNTS(metric, type_name, value, max) \
+  case NotificationImageLoader::Type::type_name: {                            \

[Peter Beverloo, 2016/11/30 17:44:02]

It's technically not needed to quality NotificationImageLoader, but OK to keep
it since the macro's definition is outside the class' scope..

[johnme, 2016/11/30 19:08:35]

Acknowledged.

+    DEFINE_THREAD_SAFE_STATIC_LOCAL(                                          \
+        CustomCountHistogram, metric##type_name##Histogram,                   \
+        new CustomCountHistogram("Notifications." #metric "." #type_name,     \
+                                 1 /* min */, max, 50 /* buckets */));        \
+    metric##type_name##Histogram.count(value);                                \
+    break;                                                                    \
+  }
+
+#define NOTIFICATION_HISTOGRAM_COUNTS(metric, type, value, max)            \
+  switch (type) {                                                          \
+    NOTIFICATION_PER_TYPE_HISTOGRAM_COUNTS(metric, Image, value, max)      \
+    NOTIFICATION_PER_TYPE_HISTOGRAM_COUNTS(metric, Icon, value, max)       \
+    NOTIFICATION_PER_TYPE_HISTOGRAM_COUNTS(metric, Badge, value, max)      \
+    NOTIFICATION_PER_TYPE_HISTOGRAM_COUNTS(metric, ActionIcon, value, max) \
+  }
+
 namespace {
 
 // 99.9% of all images were fetched successfully in 90 seconds.
@@ -28,11 +47,53 @@ const unsigned long kImageFetchTimeoutInMs = 90000;
 
 namespace blink {
 
-NotificationImageLoader::NotificationImageLoader()
-    : m_stopped(false), m_startTime(0.0) {}
+NotificationImageLoader::NotificationImageLoader(Type type)
+    : m_type(type), m_stopped(false), m_startTime(0.0) {}
 
 NotificationImageLoader::~NotificationImageLoader() {}
 
+// static
+SkBitmap NotificationImageLoader::scaleDownIfNeeded(const SkBitmap& image,
+                                                    Type type) {
+  int maxWidthPx = -1, maxHeightPx = -1;
+  switch (type) {
+    case Type::Image:
+      maxWidthPx = kWebNotificationMaxImageWidthPx;
+      maxHeightPx = kWebNotificationMaxImageHeightPx;
+      break;
+    case Type::Icon:
+      maxWidthPx = kWebNotificationMaxIconSizePx;
+      maxHeightPx = kWebNotificationMaxIconSizePx;
+      break;
+    case Type::Badge:
+      maxWidthPx = kWebNotificationMaxBadgeSizePx;
+      maxHeightPx = kWebNotificationMaxBadgeSizePx;
+      break;
+    case Type::ActionIcon:
+      maxWidthPx = kWebNotificationMaxActionIconSizePx;
+      maxHeightPx = kWebNotificationMaxActionIconSizePx;
+      break;
+  }
+  DCHECK_GT(maxWidthPx, 0);
+  DCHECK_GT(maxHeightPx, 0);

[Peter Beverloo, 2016/11/30 17:44:02]

micro nit: I have a very mild preference for initializing the variables on line
58 to "0". There's a bit of an inconsistency now.. Won't block on that.

[johnme, 2016/11/30 19:08:35]

Done.

+  // TODO(peter): Explore doing the scaling on a background thread.
+  if (image.width() > maxWidthPx || image.height() > maxHeightPx) {
+    double scale = std::min(static_cast<double>(maxWidthPx) / image.width(),
+                            static_cast<double>(maxHeightPx) / image.height());
+    double startTime = monotonicallyIncreasingTimeMS();
+    // TODO(peter): Try using RESIZE_BETTER for large images.
+    SkBitmap scaledImage =
+        skia::ImageOperations::Resize(image, skia::ImageOperations::RESIZE_BEST,
+                                      std::lround(scale * image.width()),
+                                      std::lround(scale * image.height()));
+    NOTIFICATION_HISTOGRAM_COUNTS(LoadScaleDownTime, type,
+                                  monotonicallyIncreasingTimeMS() - startTime,
+                                  1000 * 10 /* 10 seconds max */);
+    return scaledImage;
+  }
+  return image;
+}
+
 void NotificationImageLoader::start(
     ExecutionContext* executionContext,
     const KURL& url,
@@ -89,19 +150,13 @@ void NotificationImageLoader::didFinishLoading(unsigned long resourceIdentifier,
   if (m_stopped)
     return;
 
-  DEFINE_THREAD_SAFE_STATIC_LOCAL(
-      CustomCountHistogram, finishedTimeHistogram,
-      new CustomCountHistogram("Notifications.Icon.LoadFinishTime", 1,
-                               1000 * 60 * 60 /* 1 hour max */,
-                               50 /* buckets */));
-  finishedTimeHistogram.count(monotonicallyIncreasingTimeMS() - m_startTime);
+  NOTIFICATION_HISTOGRAM_COUNTS(LoadFinishTime, m_type,
+                                monotonicallyIncreasingTimeMS() - m_startTime,
+                                1000 * 60 * 60 /* 1 hour max */);
 
   if (m_data) {
-    DEFINE_THREAD_SAFE_STATIC_LOCAL(
-        CustomCountHistogram, fileSizeHistogram,
-        new CustomCountHistogram("Notifications.Icon.FileSize", 1,
-                                 10000000 /* ~10mb max */, 50 /* buckets */));
-    fileSizeHistogram.count(m_data->size());
+    NOTIFICATION_HISTOGRAM_COUNTS(LoadFileSize, m_type, m_data->size(),
+                                  10000000 /* ~10mb max */);
 
     std::unique_ptr<ImageDecoder> decoder = ImageDecoder::create(
         m_data, true /* dataComplete */, ImageDecoder::AlphaPremultiplied,
@@ -119,12 +174,9 @@ void NotificationImageLoader::didFinishLoading(unsigned long resourceIdentifier,
 }
 
 void NotificationImageLoader::didFail(const ResourceError& error) {
-  DEFINE_THREAD_SAFE_STATIC_LOCAL(
-      CustomCountHistogram, failedTimeHistogram,
-      new CustomCountHistogram("Notifications.Icon.LoadFailTime", 1,
-                               1000 * 60 * 60 /* 1 hour max */,
-                               50 /* buckets */));
-  failedTimeHistogram.count(monotonicallyIncreasingTimeMS() - m_startTime);
+  NOTIFICATION_HISTOGRAM_COUNTS(LoadFailTime, m_type,
+                                monotonicallyIncreasingTimeMS() - m_startTime,
+                                1000 * 60 * 60 /* 1 hour max */);
 
   runCallbackWithEmptyBitmap();
 }