[IC] Use a seperate class for metrics reporting

Follow-up from previous histogram CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1250061/5
comments in cached_image_fetcher.h.

Bug: 888712
Change-Id: I87b769eb0754b4e49917ea0f62eeaf12513ec72d
Reviewed-on: https://chromium-review.googlesource.com/c/1286897
Commit-Queue: Brandon Wylie <wylieb@chromium.org>
Reviewed-by: Filip Gorski <fgorski@chromium.org>
Reviewed-by: Sky Malice <skym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600914}

components/image_fetcher/core/cache/BUILD.gn

@@ -4,6 +4,8 @@
 
 static_library("cache") {
   sources = [
+    "cached_image_fetcher_metrics_reporter.cc",
+    "cached_image_fetcher_metrics_reporter.h",
     "image_cache.cc",
     "image_cache.h",
     "image_data_store.h",

components/image_fetcher/core/cache/cached_image_fetcher_metrics_reporter.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/image_fetcher/core/cache/cached_image_fetcher_metrics_reporter.h"
+
+#include "base/metrics/histogram_macros.h"
+
+namespace image_fetcher {
+
+// static
+void CachedImageFetcherMetricsReporter::ReportEvent(
+    CachedImageFetcherEvent event) {
+  UMA_HISTOGRAM_ENUMERATION("CachedImageFetcher.Events", event);
+}
+
+// static
+void CachedImageFetcherMetricsReporter::ReportLoadTime(LoadTimeType type,
+                                                       base::Time start_time) {
+  base::TimeDelta time_delta = base::Time::Now() - start_time;
+  switch (type) {
+    case LoadTimeType::kLoadFromCache:
+      UMA_HISTOGRAM_TIMES("CachedImageFetcher.ImageLoadFromCacheTime",
+                          time_delta);
+      break;
+    case LoadTimeType::kLoadFromNetwork:
+      UMA_HISTOGRAM_TIMES("CachedImageFetcher.ImageLoadFromNetworkTime",
+                          time_delta);
+      break;
+    case LoadTimeType::kLoadFromNetworkAfterCacheHit:
+      UMA_HISTOGRAM_TIMES(
+          "CachedImageFetcher.ImageLoadFromNetworkAfterCacheHit", time_delta);
+      break;
+  }
+}
+
+}  // namespace image_fetcher
\ No newline at end of file

components/image_fetcher/core/cache/cached_image_fetcher_metrics_reporter.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_IMAGE_FETCHER_CORE_CACHE_CACHED_IMAGE_FETCHER_METRICS_REPORTER_H_
+#define COMPONENTS_IMAGE_FETCHER_CORE_CACHE_CACHED_IMAGE_FETCHER_METRICS_REPORTER_H_
+
+#include "base/time/time.h"
+
+namespace image_fetcher {
+
+// Enum for the result of the fetch, reported through UMA. Present in enums.xml
+// as CachedImageFetcherEvent. New values should be added at the end and things
+// should not be renumbered.
+enum class CachedImageFetcherEvent {
+  kImageRequest = 0,
+  kCacheHit = 1,
+  kCacheMiss = 2,
+  kCacheDecodingError = 3,
+  kTranscodingError = 4,
+  kFailure = 5,
+  kMaxValue = kFailure,
+};
+
+// Tracks the various forms of timing events.
+enum class LoadTimeType {
+  kLoadFromCache = 0,
+  kLoadFromNetwork = 1,
+  kLoadFromNetworkAfterCacheHit = 2
+};
+
+class CachedImageFetcherMetricsReporter {
+ public:
+  // Report cache events, used by CachedImageFetcher and composing classes.
+  static void ReportEvent(CachedImageFetcherEvent event);
+
+  // Report timing for various Cache events related to CachedImageFetcher.
+  static void ReportLoadTime(LoadTimeType type, base::Time start_time);
+};
+
+}  // namespace image_fetcher
+
+#endif  // COMPONENTS_IMAGE_FETCHER_CORE_CACHE_CACHED_IMAGE_FETCHER_METRICS_REPORTER_H_
\ No newline at end of file

components/image_fetcher/core/cached_image_fetcher.cc

@@ -9,6 +9,7 @@
 #include "base/bind.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/timer/elapsed_timer.h"
+#include "components/image_fetcher/core/cache/cached_image_fetcher_metrics_reporter.h"
 #include "components/image_fetcher/core/cache/image_cache.h"
 #include "components/image_fetcher/core/image_decoder.h"
 #include "components/image_fetcher/core/request_metadata.h"
@@ -21,13 +22,6 @@ namespace image_fetcher {
 
 namespace {
 
-// Tracks the various forms of timing events.
-enum class LoadTimeType {
-  kLoadFromCache = 0,
-  kLoadFromNetwork = 1,
-  kLoadFromNetworkAfterCacheHit = 2
-};
-
 void DataCallbackIfPresent(ImageDataFetcherCallback data_callback,
                            const std::string& image_data,
                            const image_fetcher::RequestMetadata& metadata) {
@@ -60,31 +54,8 @@ bool EncodeSkBitmapToPNG(const SkBitmap& bitmap,
       std::vector<gfx::PNGCodec::Comment>(), dest);
 }
 
-void ReportLoadTime(LoadTimeType type, base::Time start_time) {
-  base::TimeDelta time_delta = base::Time::Now() - start_time;
-  switch (type) {
-    case LoadTimeType::kLoadFromCache:
-      UMA_HISTOGRAM_TIMES("CachedImageFetcher.ImageLoadFromCacheTime",
-                          time_delta);
-      break;
-    case LoadTimeType::kLoadFromNetwork:
-      UMA_HISTOGRAM_TIMES("CachedImageFetcher.ImageLoadFromNetworkTime",
-                          time_delta);
-      break;
-    case LoadTimeType::kLoadFromNetworkAfterCacheHit:
-      UMA_HISTOGRAM_TIMES(
-          "CachedImageFetcher.ImageLoadFromNetworkAfterCacheHit", time_delta);
-      break;
-  }
-}
-
 }  // namespace
 
-// static
-void CachedImageFetcher::ReportEvent(CachedImageFetcherEvent event) {
-  UMA_HISTOGRAM_ENUMERATION("CachedImageFetcher.Events", event);
-}
-
 CachedImageFetcher::CachedImageFetcher(
     std::unique_ptr<ImageFetcher> image_fetcher,
     scoped_refptr<ImageCache> image_cache,
@@ -132,7 +103,8 @@ void CachedImageFetcher::FetchImageAndData(
                      image_url, std::move(data_callback),
                      std::move(image_callback), traffic_annotation));
 
-  ReportEvent(CachedImageFetcherEvent::kImageRequest);
+  CachedImageFetcherMetricsReporter::ReportEvent(
+      CachedImageFetcherEvent::kImageRequest);
 }
 
 void CachedImageFetcher::OnImageFetchedFromCache(
@@ -149,7 +121,8 @@ void CachedImageFetcher::OnImageFetchedFromCache(
                           std::move(data_callback), std::move(image_callback),
                           traffic_annotation);
 
-    ReportEvent(CachedImageFetcherEvent::kCacheMiss);
+    CachedImageFetcherMetricsReporter::ReportEvent(
+        CachedImageFetcherEvent::kCacheMiss);
   } else {
     DataCallbackIfPresent(std::move(data_callback), image_data,
                           RequestMetadata());
@@ -161,7 +134,8 @@ void CachedImageFetcher::OnImageFetchedFromCache(
                             image_url, base::Passed(std::move(data_callback)),
                             base::Passed(std::move(image_callback)),
                             traffic_annotation, image_data));
-    ReportEvent(CachedImageFetcherEvent::kCacheHit);
+    CachedImageFetcherMetricsReporter::ReportEvent(
+        CachedImageFetcherEvent::kCacheHit);
   }
 }
 
@@ -180,11 +154,13 @@ void CachedImageFetcher::OnImageDecodedFromCache(
                           std::move(data_callback), std::move(image_callback),
                           traffic_annotation);
 
-    ReportEvent(CachedImageFetcherEvent::kCacheDecodingError);
+    CachedImageFetcherMetricsReporter::ReportEvent(
+        CachedImageFetcherEvent::kCacheDecodingError);
   } else {
     ImageCallbackIfPresent(std::move(image_callback), id, image,
                            RequestMetadata());
-    ReportLoadTime(LoadTimeType::kLoadFromCache, start_time);
+    CachedImageFetcherMetricsReporter::ReportLoadTime(
+        LoadTimeType::kLoadFromCache, start_time);
   }
 }
 
@@ -224,13 +200,15 @@ void CachedImageFetcher::OnImageFetchedFromNetwork(
 
   // Report failure if the image is empty.
   if (image.IsEmpty()) {
-    ReportEvent(CachedImageFetcherEvent::kFailure);
+    CachedImageFetcherMetricsReporter::ReportEvent(
+        CachedImageFetcherEvent::kFailure);
   }
 
   // Report to different histograms depending upon if there was a cache hit.
-  ReportLoadTime(cache_hit ? LoadTimeType::kLoadFromNetworkAfterCacheHit
-                           : LoadTimeType::kLoadFromNetwork,
-                 start_time);
+  CachedImageFetcherMetricsReporter::ReportLoadTime(
+      cache_hit ? LoadTimeType::kLoadFromNetworkAfterCacheHit
+                : LoadTimeType::kLoadFromNetwork,
+      start_time);
 }
 
 void CachedImageFetcher::DecodeDataForCaching(
@@ -252,7 +230,8 @@ void CachedImageFetcher::EncodeDataAndCache(const GURL& image_url,
   // it.
   if (image.IsEmpty() ||
       !EncodeSkBitmapToPNG(*image.ToSkBitmap(), &encoded_data)) {
-    ReportEvent(CachedImageFetcherEvent::kTranscodingError);
+    CachedImageFetcherMetricsReporter::ReportEvent(
+        CachedImageFetcherEvent::kTranscodingError);
     image_cache_->DeleteImage(image_url.spec());
     return;
   }

components/image_fetcher/core/cached_image_fetcher.h

@@ -12,6 +12,7 @@
 #include "base/containers/flat_map.h"
 #include "base/memory/weak_ptr.h"
 #include "base/timer/timer.h"
+#include "components/image_fetcher/core/cache/cached_image_fetcher_metrics_reporter.h"
 #include "components/image_fetcher/core/image_decoder.h"
 #include "components/image_fetcher/core/image_fetcher.h"
 #include "components/image_fetcher/core/image_fetcher_types.h"
@@ -30,30 +31,12 @@ class ImageCache;
 class ImageFetcher;
 struct RequestMetadata;
 
-// Enum for the result of the fetch, reported through UMA Present in enums.xml
-// as CachedImageFetcherEvent. New values should be added at the end and things
-// should not be renumbered.
-enum class CachedImageFetcherEvent {
-  kImageRequest = 0,
-  kCacheHit = 1,
-  kCacheMiss = 2,
-  kCacheDecodingError = 3,
-  kTranscodingError = 4,
-  kFailure = 5,
-  kMaxValue = kFailure,
-};
-
-// TODO(wylieb): Transcode the image once it's downloaded.
 // TODO(wylieb): Consider creating a struct to encapsulate the request.
 // CachedImageFetcher takes care of fetching images from the network and caching
 // them. Has a read-only mode which doesn't perform write operations on the
 // cache.
 class CachedImageFetcher : public ImageFetcher {
  public:
-  // Report CachedImageFetcher events, used by sub-systems to report events (as
-  // well as CachedImageFetcher).
-  static void ReportEvent(CachedImageFetcherEvent event);
-
   CachedImageFetcher(std::unique_ptr<ImageFetcher> image_fetcher,
                      scoped_refptr<ImageCache> image_cache,
                      bool read_only);