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

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

Index: chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java b/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
index 2dad1b217748cba66943d746ed405674f9201c46..a5dc53af7e8699f24eabb2560294e79c92df18c5 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
+++ b/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();

[Michael van Ouwerkerk, 2017/04/13 10:16:22]

One way to slightly reduce the omnipresense of this variable would be to move
this statement into both fetchFaviconFromLocalCache and
fetchFaviconFromLocalCacheOrGoogleServer so they won't have it as an argument
and you can delete the variable here. Fwiw :-)

[jkrcal, 2017/04/13 11:37:31]

This does not work for fetchFaviconFromLocalCache as this can be (similarly to
the c++ flow) called twice per request and I need to drag the same timestamp
across these two calls.

             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() {