[Remote suggestions] Log favicon fetch result for both code paths

components/ntp_snippets/content_suggestions_service.cc

Index: components/ntp_snippets/content_suggestions_service.cc
diff --git a/components/ntp_snippets/content_suggestions_service.cc b/components/ntp_snippets/content_suggestions_service.cc
index 9d4955e1e2872eda23d76479904ebb3e65b90805..a4a80e05b880d66f7342271208ab2496604cdb55 100644
--- a/components/ntp_snippets/content_suggestions_service.cc
+++ b/components/ntp_snippets/content_suggestions_service.cc
@@ -12,6 +12,7 @@
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/memory/ptr_util.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/default_clock.h"
@@ -27,6 +28,27 @@
 
 namespace ntp_snippets {
 
+namespace {
+
+// Enumeration listing all possible outcomes for fetch attempts of favicons for
+// content suggestions. Used for UMA histograms, so do not change existing
+// values. Insert new values at the end, and update the histogram definition.
+// GENERATED_JAVA_ENUM_PACKAGE: org.chromium.chrome.browser.ntp.snippets
+enum class FaviconFetchResult {
+  SUCCESS_CACHED = 0,
+  SUCCESS_FETCHED = 1,
+  FAILURE = 2,
+  RESULT_MAX = 3

[Ilya Sherman, 2017/04/12 23:18:32]

Optional nit: I'd name this "COUNT" rather than "MAX" -- "max" is typically used
as an alias for the largest actual value in an enum.

[jkrcal, 2017/04/13 08:10:46]

Good point, done.

+};
+
+void RecordFaviconFetchResult(FaviconFetchResult result) {
+  UMA_HISTOGRAM_ENUMERATION(
+      "NewTabPage.ContentSuggestions.ArticleFaviconFetchResult", result,
+      FaviconFetchResult::RESULT_MAX);
+}
+
+}  // namespace
+
 ContentSuggestionsService::ContentSuggestionsService(
     State state,
     SigninManagerBase* signin_manager,
@@ -151,6 +173,7 @@ void ContentSuggestionsService::FetchSuggestionFavicon(
   if (position == suggestions->end() || !large_icon_service_) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(callback, gfx::Image()));
+    RecordFaviconFetchResult(FaviconFetchResult::FAILURE);
     return;
   }
 
@@ -177,6 +200,11 @@ void ContentSuggestionsService::OnGetFaviconFromCacheFinished(
     const favicon_base::LargeIconImageResult& result) {
   if (!result.image.IsEmpty()) {

[Michael van Ouwerkerk, 2017/04/12 13:20:31]

Should there be an else clause to log FaviconFetchResult::FAILURE ?

[jkrcal, 2017/04/13 08:10:46]

The failure is recorded on line 218 / line 240.

The execution flow is
 - check from cache (potentially report success_cached)
 - check from google server (potentially report failure if we cannot send the
request - 218 - or if the response is empty - 240)
 - get the newly fetched result from the cache (report success_fetched)

     callback.Run(result.image);
+    // The icon is from cache if we haven't gone to Google server yet. The icon
+    // is freshly fetched, otherwise.
+    RecordFaviconFetchResult(continue_to_google_server
+                                 ? FaviconFetchResult::SUCCESS_CACHED
+                                 : FaviconFetchResult::SUCCESS_FETCHED);
     return;
   }
 
@@ -184,8 +212,10 @@ void ContentSuggestionsService::OnGetFaviconFromCacheFinished(
       (result.fallback_icon_style &&
        !result.fallback_icon_style->is_default_background_color)) {
     // We cannot download from the server if there is some small icon in the
-    // cache (resulting in non-default bakground color) or if we already did so.
+    // cache (resulting in non-default background color) or if we already did
+    // so.
     callback.Run(gfx::Image());
+    RecordFaviconFetchResult(FaviconFetchResult::FAILURE);
     return;
   }
 
@@ -207,6 +237,7 @@ void ContentSuggestionsService::OnGetFaviconFromGoogleServerFinished(
     bool success) {
   if (!success) {
     callback.Run(gfx::Image());
+    RecordFaviconFetchResult(FaviconFetchResult::FAILURE);
     return;
   }