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..97ced8b7cf5881c4a2907a8aad3e8c205feaa2a1 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,7 @@ public class LogoDelegateImpl implements LogoView.Delegate {
     private String mOnLogoClickUrl;
     private String mAnimatedLogoUrl;
 
-    private boolean mHasRecordedLoadTime;
+    private boolean mShouldRecordLoadTime;

[Bernhard Bauer, 2017/04/28 10:13:28]

Initialize this here?

[fhorschig, 2017/04/28 12:36:51]

Done.

Just out of curiosity: Shouldn't the code style discussion [1] apply here?
I would understand if the class was final (which seems reasonable, btw).

[1]
https://chromium.googlesource.com/chromium/src/+/master/styleguide/java/java....

 
     private boolean mIsDestroyed;
 
@@ -51,6 +51,7 @@ public class LogoDelegateImpl implements LogoView.Delegate {
         mTab = tab;
         mLogoView = logoView;
         mLogoBridge = new LogoBridge(tab.getProfile());
+        mShouldRecordLoadTime = true;
     }
 
     @Override
@@ -94,13 +95,14 @@ 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) {
                         long loadTime = System.currentTimeMillis() - loadTimeStart;
                         RecordHistogram.recordMediumTimesHistogram(
                                 LOGO_SHOWN_TIME_UMA_NAME, loadTime, TimeUnit.MILLISECONDS);
-                        mHasRecordedLoadTime = true;
                     }
                 }
+                // If we haven't received a logo on demand, don't wait for random refreshes.

[tschumann, 2017/04/27 11:52:08]

nit: this comment implies that the first call of this function is always
connected with the on-demand fetch.

Is this guaranteed? If so, we should also document this at GetCurrentLogo(). If
not, we should write a bit more here why this (usually?) works.

[fhorschig, 2017/04/27 11:55:42]

It is guaranteed. Added that.

[Marc Treib, 2017/04/27 11:56:09]

It's not really guaranteed, but it doesn't really matter. How about:
// If there currently is no Doodle, don't record the time if a refresh happens
later.

[fhorschig, 2017/04/27 12:12:20]

Done. (Sorry, I updated the wrong comment before. Here it is not guaranteed,
true.)

+                mShouldRecordLoadTime = false;
                 logoObserver.onLogoAvailable(logo, fromCache);
             }
         };