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..93823af25e6c90447484175ca31443b3ed97be7d 100644
--- a/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
+++ b/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
@@ -14,11 +14,31 @@
 #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 DEFINE_SINGLE_TYPE_COUNT_HISTOGRAM(metric, type_name, value, max)      \
+  case NotificationImageLoader::Type::type_name: {                             \
+    DEFINE_THREAD_SAFE_STATIC_LOCAL(                                           \
+        CustomCountHistogram, metric##type_name##Histogram,                    \
+        new CustomCountHistogram(                                              \
+            "Notifications." #metric "." #type_name,                           \
+                                 1 /* min */, max, 50 /* buckets */));         \

[Peter Beverloo, 2016/11/29 19:01:40]

nit: fix alignment

[johnme, 2016/11/29 20:04:29]

Done.

+    metric##type_name##Histogram.count(value);                                 \
+    break;                                                                     \
+  }
+
+#define DEFINE_COUNT_HISTOGRAM(metric, type, value, max)                       \
+  switch (type) {                                                              \
+    DEFINE_SINGLE_TYPE_COUNT_HISTOGRAM(metric, Image, value, max);             \
+    DEFINE_SINGLE_TYPE_COUNT_HISTOGRAM(metric, Icon, value, max);              \
+    DEFINE_SINGLE_TYPE_COUNT_HISTOGRAM(metric, Badge, value, max);             \
+    DEFINE_SINGLE_TYPE_COUNT_HISTOGRAM(metric, ActionIcon, value, max);        \

[Peter Beverloo, 2016/11/29 19:01:40]

No semi-colons after these lines. (You wouldn't add them after a closing bracket
either.)

[johnme, 2016/11/29 20:04:28]

Done.

+  }
+
 namespace {
 
 // 99.9% of all images were fetched successfully in 90 seconds.
@@ -28,11 +48,47 @@ 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;

[Peter Beverloo, 2016/11/29 19:01:40]

You're going to want some |break| statements in this switch.

[johnme, 2016/11/29 20:04:28]

Done :)

+    case Type::Icon:
+      maxWidthPx = kWebNotificationMaxIconSizePx;
+      maxHeightPx = kWebNotificationMaxIconSizePx;
+    case Type::Badge:
+      maxWidthPx = kWebNotificationMaxBadgeSizePx;
+      maxHeightPx = kWebNotificationMaxBadgeSizePx;
+    case Type::ActionIcon:
+      maxWidthPx = kWebNotificationMaxActionIconSizePx;
+      maxHeightPx = kWebNotificationMaxActionIconSizePx;
+  }

[Peter Beverloo, 2016/11/29 19:01:40]

DCHECK_NE on maxWidthPx/maxHeightPx not being -1

[johnme, 2016/11/29 20:04:28]

Done (DCHECK_GT 0).

+  // TODO(mvanouwerkerk): Explore doing the scaling on a background thread.

[Peter Beverloo, 2016/11/29 19:01:40]

s/mvanouwerkerk/another name/

[johnme, 2016/11/29 20:04:28]

Done.

+  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()));
+    DEFINE_COUNT_HISTOGRAM(LoadScaleDownTime, type,
+                           monotonicallyIncreasingTimeMS() - startTime,
+                           1000 * 10 /* 10 seconds max */);
+    return scaledImage;

[Peter Beverloo, 2016/11/29 19:01:40]

nit: std::move(scaledImage) to prevent silly compilers from making a copy? (Even
though it's just ref churn...)

[johnme, 2016/11/29 20:04:29]

error: moving a local object in a return statement prevents copy elision
[-Werror,-Wpessimizing-move]

+  }
+  return image;
+}
+
 void NotificationImageLoader::start(
     ExecutionContext* executionContext,
     const KURL& url,
@@ -89,19 +145,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);
+  DEFINE_COUNT_HISTOGRAM(LoadFinishTime, m_type,
+                         monotonicallyIncreasingTimeMS() - m_startTime,
+                         1000 * 60 * 60 /* 1 hour max */);

[Peter Beverloo, 2016/11/29 19:01:40]

Please find a better name for DEFINE_COUNT_HISTOGRAM. It doesn't just define the
histogram, but it also counts the value.

[johnme, 2016/11/29 20:04:28]

Done (NOTIFICATION_HISTOGRAM_COUNTS, by analogy to UMA_HISTOGRAM_CUSTOM_COUNTS
in chromium, which also both defines a static local and does the counting).

 
   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());
+    DEFINE_COUNT_HISTOGRAM(LoadFileSize, m_type,
+                           m_data->size(), 10000000 /* ~10mb max */);
 
     std::unique_ptr<ImageDecoder> decoder = ImageDecoder::create(
         m_data, true /* dataComplete */, ImageDecoder::AlphaPremultiplied,
@@ -119,12 +169,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);
+  DEFINE_COUNT_HISTOGRAM(LoadFailTime, m_type,
+                         monotonicallyIncreasingTimeMS() - m_startTime,
+                         1000 * 60 * 60 /* 1 hour max */);
 
   runCallbackWithEmptyBitmap();
 }