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

components/ntp_snippets/content_suggestions_service.cc

 // Copyright 2016 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/ntp_snippets/content_suggestions_service.h"
 
 #include <algorithm>
 #include <iterator>
 #include <set>
 #include <utility>
 
 #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"
 #include "base/values.h"
 #include "components/favicon/core/large_icon_service.h"
 #include "components/favicon_base/fallback_icon_style.h"
 #include "components/favicon_base/favicon_types.h"
 #include "components/ntp_snippets/content_suggestions_metrics.h"
 #include "components/ntp_snippets/pref_names.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 #include "ui/gfx/image/image.h"
 
 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,
     history::HistoryService* history_service,
     favicon::LargeIconService* large_icon_service,
     PrefService* pref_service,
     std::unique_ptr<CategoryRanker> category_ranker,
     std::unique_ptr<UserClassifier> user_classifier,
     std::unique_ptr<RemoteSuggestionsScheduler> remote_suggestions_scheduler)
     : state_(state),
@@ skipping 104 matching lines @@
   std::vector<ContentSuggestion>* suggestions =
       &suggestions_by_category_[suggestion_id.category()];
   auto position =
       std::find_if(suggestions->begin(), suggestions->end(),
                    [&suggestion_id](const ContentSuggestion& suggestion) {
                      return suggestion_id == suggestion.id();
                    });
   if (position == suggestions->end() || !large_icon_service_) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(callback, gfx::Image()));
+    RecordFaviconFetchResult(FaviconFetchResult::FAILURE);
     return;
   }
 
   const GURL& publisher_url = position->url().GetWithEmptyPath();
 
   // TODO(jkrcal): Create a general wrapper function in LargeIconService that
   // does handle the get-from-cache-and-fallback-to-google-server functionality
   // in one shot (for all clients that do not need to react in between).
   large_icon_service_->GetLargeIconImageOrFallbackStyle(
       publisher_url, minimum_size_in_pixel, desired_size_in_pixel,
       base::Bind(&ContentSuggestionsService::OnGetFaviconFromCacheFinished,
                  base::Unretained(this), publisher_url, minimum_size_in_pixel,
                  desired_size_in_pixel, callback,
                  /*continue_to_google_server=*/true),
       &favicons_task_tracker_);
 }
 
 void ContentSuggestionsService::OnGetFaviconFromCacheFinished(
     const GURL& publisher_url,
     int minimum_size_in_pixel,
     int desired_size_in_pixel,
     const ImageFetchedCallback& callback,
     bool continue_to_google_server,
     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;
   }
 
   if (!continue_to_google_server ||
       (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;
   }
 
   // Try to fetch the favicon from a Google favicon server.
   large_icon_service_
       ->GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
           publisher_url, minimum_size_in_pixel,
           base::Bind(
               &ContentSuggestionsService::OnGetFaviconFromGoogleServerFinished,
               base::Unretained(this), publisher_url, minimum_size_in_pixel,
               desired_size_in_pixel, callback));
 }
 
 void ContentSuggestionsService::OnGetFaviconFromGoogleServerFinished(
     const GURL& publisher_url,
     int minimum_size_in_pixel,
     int desired_size_in_pixel,
     const ImageFetchedCallback& callback,
     bool success) {
   if (!success) {
     callback.Run(gfx::Image());
+    RecordFaviconFetchResult(FaviconFetchResult::FAILURE);
     return;
   }
 
   // Get the freshly downloaded icon from the cache.
   large_icon_service_->GetLargeIconImageOrFallbackStyle(
       publisher_url, minimum_size_in_pixel, desired_size_in_pixel,
       base::Bind(&ContentSuggestionsService::OnGetFaviconFromCacheFinished,
                  base::Unretained(this), publisher_url, minimum_size_in_pixel,
                  desired_size_in_pixel, callback,
                  /*continue_to_google_server=*/false),
@@ skipping 401 matching lines @@
 void ContentSuggestionsService::StoreDismissedCategoriesToPrefs() {
   base::ListValue list;
   for (const auto& category_provider_pair : dismissed_providers_by_category_) {
     list.AppendInteger(category_provider_pair.first.id());
   }
 
   pref_service_->Set(prefs::kDismissedCategories, list);
 }
 
 }  // namespace ntp_snippets