Record NTP.LogoShownTime for timely refreshs only

chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java

Index: chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java b/chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java
index d38337333bab4fe38381f1fccce40f6e2d2b6d16..1012a7b35062bc846544c47ca65918c36a5cf7cc 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/ntp/LogoDelegateImpl.java
@@ -38,7 +38,8 @@ public class LogoDelegateImpl implements LogoView.Delegate {
     private String mOnLogoClickUrl;
     private String mAnimatedLogoUrl;
 
-    private boolean mHasRecordedLoadTime;
+    private boolean mShouldRecordLoadTime;
+    private boolean mHasSkippedFirstDoodleRefresh;
 
     private boolean mIsDestroyed;
 
@@ -51,6 +52,7 @@ public class LogoDelegateImpl implements LogoView.Delegate {
         mTab = tab;
         mLogoView = logoView;
         mLogoBridge = new LogoBridge(tab.getProfile());
+        mShouldRecordLoadTime = true;

[Marc Treib, 2017/04/26 10:17:16]

I think the initialization can happen inline with the declaration (though not
sure if that's the preferred style in Java?)

[fhorschig, 2017/04/27 10:31:43]

It's discouraged (especially for non-final fields):
https://chromium.googlesource.com/chromium/src/+/master/styleguide/java/java....

     }
 
     @Override
@@ -85,7 +87,7 @@ public class LogoDelegateImpl implements LogoView.Delegate {
 
         final long loadTimeStart = System.currentTimeMillis();
 
-        LogoObserver wrapperCallback = new LogoObserver() {
+        LogoObserver availableCallbackWrapper = new LogoObserver() {
             @Override
             public void onLogoAvailable(Logo logo, boolean fromCache) {
                 if (mIsDestroyed) return;
@@ -94,17 +96,28 @@ public class LogoDelegateImpl implements LogoView.Delegate {
                 if (logo != null) {
                     RecordHistogram.recordSparseSlowlyHistogram(LOGO_SHOWN_UMA_NAME,
                             logo.animatedLogoUrl == null ? STATIC_LOGO_SHOWN : CTA_IMAGE_SHOWN);
-                    if (!mHasRecordedLoadTime) {
+                    if (mShouldRecordLoadTime && !mHasSkippedFirstDoodleRefresh) {

[Marc Treib, 2017/04/26 10:17:16]

Hm. Won't onRefreshSkipped() get called before onLogoAvailable(cached_logo)? If
so, then we'll never record a time for cached logos now.

[fhorschig, 2017/04/27 10:31:43]

Gone.

+                        // Records the time only for cached logos and immediately fetched logos.
                         long loadTime = System.currentTimeMillis() - loadTimeStart;
                         RecordHistogram.recordMediumTimesHistogram(
                                 LOGO_SHOWN_TIME_UMA_NAME, loadTime, TimeUnit.MILLISECONDS);
-                        mHasRecordedLoadTime = true;
                     }
                 }
+                // Whether we recorded a time or not, do not wait for the next refresh.
+                mShouldRecordLoadTime = false;
                 logoObserver.onLogoAvailable(logo, fromCache);
             }
         };
 
-        mLogoBridge.getCurrentLogo(wrapperCallback);
+        LogoBridge.RefreshCallbackWrapper refreshCallbackWrapper =

[Marc Treib, 2017/04/26 10:17:16]

Also here: No wrapping involved

[fhorschig, 2017/04/27 10:31:43]

Gone.

+                new LogoBridge.RefreshCallbackWrapper() {
+                    @Override
+                    public void onRefreshSkipped() {
+                        if (mIsDestroyed) return;
+                        mHasSkippedFirstDoodleRefresh = true;

[Marc Treib, 2017/04/26 10:17:16]

nit: I'd remove the "First" from the name, since it'll get set for any non-first
skipped refreshes too.

[fhorschig, 2017/04/27 10:31:43]

Gone.

+                    }
+                };
+
+        mLogoBridge.getCurrentLogo(availableCallbackWrapper, refreshCallbackWrapper);
     }
 }