[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..9d4c600b658f34c89e990e6718297b4cdbeeb737 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
@@ -93,6 +93,7 @@ public class SnippetArticleViewHolder extends CardViewHolder implements Impressi
     private SnippetArticle mArticle;
     private SuggestionsCategoryInfo mCategoryInfo;
     private int mPublisherFaviconSizePx;
+    private long mFaviconFetchStartTime;

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

nit: rename to mFaviconFetchStartTimeMs

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

Done.

 
     /**
      * Constructs a {@link SnippetArticleViewHolder} item used to display snippets.
@@ -205,6 +206,7 @@ public class SnippetArticleViewHolder extends CardViewHolder implements Impressi
         // from moving later.
         setDefaultFaviconOnView();
         try {
+            mFaviconFetchStartTime = SystemClock.elapsedRealtime();

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

As onBindViewHolder can be called more than once, it seems better to scope the
start time as a member of the callback for the favicon request.

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

Thanks for spotting this! Done. 

The parameter is now all around but I see no better simple way how to do it.

             URI pageUrl = new URI(mArticle.mUrl);
             if (!article.isArticle() || !SnippetsConfig.isFaviconsFromNewServerEnabled()) {
                 // The old code path. Remove when the experiment is successful.
@@ -408,12 +410,28 @@ public class SnippetArticleViewHolder extends CardViewHolder implements Impressi
                 PUBLISHER_FAVICON_MINIMUM_SIZE_PX, /* desiredSizePx */ 0, new Callback<Bitmap>() {
                     @Override
                     public void onResult(Bitmap image) {
+                        recordFaviconFetchTime();

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

This will record the time for all results, whether they are from cache, null, or
success or failure from the network. Would it be useful to also log those cases
separately?

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

I believe that the simple solution is here good enough. I believe we will see
different peaks in the distribution for results from cache and for results from
the network. The purpose of this histogram is to check that there is no major
speed regression which should be visible from the mixed histogram.

(Logging it separately would need duplicate code in java, c++, similarly to the
fetch result histogram)

WDYT?

                         if (image == null) return;
                         setFaviconOnView(image);
                     }
                 });
     }
 
+    private void recordFaviconFetchTime() {
+        RecordHistogram.recordTimesHistogram(

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

recordTimesHistogram maxes out at 10 seconds, I think we'll exceed that often
for network requests in emerging markets. Please use recordMediumTimesHistogram
which allows for 3 minutes.

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

Done.

+                "NewTabPage.ContentSuggestions.ArticleFaviconFetchTime",
+                SystemClock.elapsedRealtime() - mFaviconFetchStartTime, TimeUnit.MILLISECONDS);
+    }
+
+    private void recordFaviconFetchResult(@FaviconFetchResult int result) {
+        // Record the histogram for articles only to have a fair comparision.
+        if (!mArticle.isArticle()) return;
+        RecordHistogram.recordEnumeratedHistogram(
+                "NewTabPage.ContentSuggestions.ArticleFaviconFetchResult", result,
+                FaviconFetchResult.RESULT_MAX);
+        recordFaviconFetchTime();
+    }
+
     private void fetchFaviconFromLocalCache(final URI snippetUri, final boolean fallbackToService) {
         mUiDelegate.getLocalFaviconImageForURL(
                 getSnippetDomain(snippetUri), mPublisherFaviconSizePx, new FaviconImageCallback() {
@@ -421,8 +439,13 @@ public class SnippetArticleViewHolder extends CardViewHolder implements Impressi
                     public void onFaviconAvailable(Bitmap image, String iconUrl) {
                         if (image != null) {
                             setFaviconOnView(image);
+                            recordFaviconFetchResult(fallbackToService
+                                            ? FaviconFetchResult.SUCCESS_CACHED
+                                            : FaviconFetchResult.SUCCESS_FETCHED);
                         } else if (fallbackToService) {
-                            fetchFaviconFromService(snippetUri);
+                            if (!fetchFaviconFromService(snippetUri)) {
+                                recordFaviconFetchResult(FaviconFetchResult.FAILURE);
+                            }
                         }
                         // Else do nothing, we already have the placeholder set.
                     }
@@ -431,10 +454,10 @@ 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) {
+        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 +466,15 @@ 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);
+                            return;
+                        }
                         // The download succeeded, the favicon is in the cache; fetch it.
                         fetchFaviconFromLocalCache(snippetUri, /*fallbackToService=*/false);
                     }
                 });
+        return true;
     }
 
     private int getFaviconServiceSupportedSize() {