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

We have two code paths that download favicons from different favicon
servers. We can switch between them based on a field trial feature.

This CL adds logging of success rates for both the code-paths.

BUG=709498

Review-Url: https://codereview.chromium.org/2812243002
Cr-Commit-Position: refs/heads/master@{#464395}

chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java

@@ -205,14 +205,15 @@ public class SnippetArticleViewHolder extends CardViewHolder implements Impressi
         // from moving later.
         setDefaultFaviconOnView();
         try {
+            long faviconFetchStartTimeMs = SystemClock.elapsedRealtime();
             URI pageUrl = new URI(mArticle.mUrl);
             if (!article.isArticle() || !SnippetsConfig.isFaviconsFromNewServerEnabled()) {
                 // The old code path. Remove when the experiment is successful.
                 // Currently, we have to use this for non-articles, due to privacy.
-                fetchFaviconFromLocalCache(pageUrl, true);
+                fetchFaviconFromLocalCache(pageUrl, true, faviconFetchStartTimeMs);
             } else {
                 // The new code path.
-                fetchFaviconFromLocalCacheOrGoogleServer();
+                fetchFaviconFromLocalCacheOrGoogleServer(faviconFetchStartTimeMs);
             }
         } catch (URISyntaxException e) {
             // Do nothing, stick to the default favicon.
@@ -402,27 +403,52 @@ public class SnippetArticleViewHolder extends CardViewHolder implements Impressi
         transitionDrawable.startTransition(FADE_IN_ANIMATION_TIME_MS);
     }
 
-    private void fetchFaviconFromLocalCacheOrGoogleServer() {
+    private void fetchFaviconFromLocalCacheOrGoogleServer(final long faviconFetchStartTimeMs) {
         // Set the desired size to 0 to specify we do not want to resize in c++, we'll resize here.
         mUiDelegate.getSuggestionsSource().fetchSuggestionFavicon(mArticle,
                 PUBLISHER_FAVICON_MINIMUM_SIZE_PX, /* desiredSizePx */ 0, new Callback<Bitmap>() {
                     @Override
                     public void onResult(Bitmap image) {
+                        recordFaviconFetchTime(faviconFetchStartTimeMs);
                         if (image == null) return;
                         setFaviconOnView(image);
                     }
                 });
     }
 
-    private void fetchFaviconFromLocalCache(final URI snippetUri, final boolean fallbackToService) {
+    private void recordFaviconFetchTime(long faviconFetchStartTimeMs) {
+        RecordHistogram.recordMediumTimesHistogram(
+                "NewTabPage.ContentSuggestions.ArticleFaviconFetchTime",
+                SystemClock.elapsedRealtime() - faviconFetchStartTimeMs, TimeUnit.MILLISECONDS);
+    }
+
+    private void recordFaviconFetchResult(
+            @FaviconFetchResult int result, long faviconFetchStartTimeMs) {
+        // Record the histogram for articles only to have a fair comparision.
+        if (!mArticle.isArticle()) return;
+        RecordHistogram.recordEnumeratedHistogram(
+                "NewTabPage.ContentSuggestions.ArticleFaviconFetchResult", result,
+                FaviconFetchResult.COUNT);
+        recordFaviconFetchTime(faviconFetchStartTimeMs);
+    }
+
+    private void fetchFaviconFromLocalCache(final URI snippetUri, final boolean fallbackToService,
+            final long faviconFetchStartTimeMs) {
         mUiDelegate.getLocalFaviconImageForURL(
                 getSnippetDomain(snippetUri), mPublisherFaviconSizePx, new FaviconImageCallback() {
                     @Override
                     public void onFaviconAvailable(Bitmap image, String iconUrl) {
                         if (image != null) {
                             setFaviconOnView(image);
+                            recordFaviconFetchResult(fallbackToService
+                                            ? FaviconFetchResult.SUCCESS_CACHED
+                                            : FaviconFetchResult.SUCCESS_FETCHED,
+                                    faviconFetchStartTimeMs);
                         } else if (fallbackToService) {
-                            fetchFaviconFromService(snippetUri);
+                            if (!fetchFaviconFromService(snippetUri, faviconFetchStartTimeMs)) {
+                                recordFaviconFetchResult(
+                                        FaviconFetchResult.FAILURE, faviconFetchStartTimeMs);
+                            }
                         }
                         // Else do nothing, we already have the placeholder set.
                     }
@@ -431,10 +457,11 @@ public class SnippetArticleViewHolder extends CardViewHolder implements Impressi
 
     // TODO(crbug.com/635567): Fix this properly.
     @SuppressLint("DefaultLocale")
-    private void fetchFaviconFromService(final URI snippetUri) {
-        if (!mUseFaviconService) return;
+    private boolean fetchFaviconFromService(
+            final URI snippetUri, final long faviconFetchStartTimeMs) {
+        if (!mUseFaviconService) return false;
         int sizePx = getFaviconServiceSupportedSize();
-        if (sizePx == 0) return;
+        if (sizePx == 0) return false;
 
         // Replace the default icon by another one from the service when it is fetched.
         mUiDelegate.ensureIconIsAvailable(
@@ -443,11 +470,17 @@ public class SnippetArticleViewHolder extends CardViewHolder implements Impressi
                 /*useLargeIcon=*/false, /*isTemporary=*/true, new IconAvailabilityCallback() {
                     @Override
                     public void onIconAvailabilityChecked(boolean newlyAvailable) {
-                        if (!newlyAvailable) return;
+                        if (!newlyAvailable) {
+                            recordFaviconFetchResult(
+                                    FaviconFetchResult.FAILURE, faviconFetchStartTimeMs);
+                            return;
+                        }
                         // The download succeeded, the favicon is in the cache; fetch it.
-                        fetchFaviconFromLocalCache(snippetUri, /*fallbackToService=*/false);
+                        fetchFaviconFromLocalCache(
+                                snippetUri, /*fallbackToService=*/false, faviconFetchStartTimeMs);
                     }
                 });
+        return true;
     }
 
     private int getFaviconServiceSupportedSize() {

components/ntp_snippets/BUILD.gn

@@ -133,6 +133,7 @@ if (is_android) {
       "category.h",
       "category_info.h",
       "category_status.h",
+      "content_suggestions_service.cc",
     ]
   }
 }

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"
@@ -26,6 +27,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,
+  COUNT = 3
+};
+
+void RecordFaviconFetchResult(FaviconFetchResult result) {
+  UMA_HISTOGRAM_ENUMERATION(
+      "NewTabPage.ContentSuggestions.ArticleFaviconFetchResult", result,
+      FaviconFetchResult::COUNT);
+}
+
+}  // namespace
+
 ContentSuggestionsService::ContentSuggestionsService(
     State state,
     SigninManagerBase* signin_manager,
@@ -148,6 +170,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;
   }
 
@@ -175,6 +198,11 @@ void ContentSuggestionsService::OnGetFaviconFromCacheFinished(
     const favicon_base::LargeIconImageResult& result) {
   if (!result.image.IsEmpty()) {
     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;
   }
 
@@ -182,8 +210,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;
   }
 
@@ -205,6 +235,7 @@ void ContentSuggestionsService::OnGetFaviconFromGoogleServerFinished(
     bool success) {
   if (!success) {
     callback.Run(gfx::Image());
+    RecordFaviconFetchResult(FaviconFetchResult::FAILURE);
     return;
   }
 

tools/metrics/histograms/histograms.xml

@@ -41455,6 +41455,24 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="NewTabPage.ContentSuggestions.ArticleFaviconFetchResult"
+    enum="FaviconFetchResult">
+  <owner>jkrcal@chromium.org</owner>
+  <summary>
+    Android: Result of fetching a favicon for an article suggestion on the New
+    Tab Page.
+  </summary>
+</histogram>
+
+<histogram name="NewTabPage.ContentSuggestions.ArticleFaviconFetchTime"
+    units="ms">
+  <owner>jkrcal@chromium.org</owner>
+  <summary>
+    Android: Time it takes to fetch a favicon for an article suggestion on the
+    New Tab Page.
+  </summary>
+</histogram>
+
 <histogram name="NewTabPage.ContentSuggestions.BackgroundFetchTrigger"
     enum="BackgroundFetchTrigger">
   <owner>jkrcal@chromium.org</owner>
@@ -93448,6 +93466,16 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </int>
 </enum>
 
+<enum name="FaviconFetchResult" type="int">
+  <int value="0" label="SUCCESS_CACHED">
+    Success - favicon found in local cache
+  </int>
+  <int value="1" label="SUCCESS_FETCHED">
+    Success - favicon fetched from the server
+  </int>
+  <int value="2" label="FAILURE">Failure - favicon not available</int>
+</enum>
+
 <enum name="FeatureObserver" type="int">
 <!-- Generated from third_party/WebKit/Source/core/frame/UseCounter.h -->