Record NTP.LogoShownTime for timely refreshs only

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

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.chrome.browser.ntp;
 
 import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.chrome.browser.ntp.LogoBridge.Logo;
 import org.chromium.chrome.browser.ntp.LogoBridge.LogoObserver;
 import org.chromium.chrome.browser.tab.Tab;
@@ skipping 20 matching lines @@
     private static final int CTA_IMAGE_CLICKED = 1;
     private static final int ANIMATED_LOGO_CLICKED = 2;
 
     private final Tab mTab;
     private LogoView mLogoView;
 
     private LogoBridge mLogoBridge;
     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;
 
     /**
      * Construct a new {@link LogoDelegateImpl}.
      * @param tab The tab showing the logo view.
      * @param logoView The view that shows the search provider logo.
      */
     public LogoDelegateImpl(Tab tab, LogoView logoView) {
         mTab = tab;
         mLogoView = logoView;
         mLogoBridge = new LogoBridge(tab.getProfile());
+        mShouldRecordLoadTime = true;
     }
 
     @Override
     public void destroy() {
         mIsDestroyed = true;
     }
 
     @Override
     public void onLogoClicked(boolean isAnimatedLogoShowing) {
         if (mIsDestroyed) return;
@@ skipping 23 matching lines @@
 
         LogoObserver wrapperCallback = new LogoObserver() {
             @Override
             public void onLogoAvailable(Logo logo, boolean fromCache) {
                 if (mIsDestroyed) return;
                 mOnLogoClickUrl = logo != null ? logo.onClickUrl : null;
                 mAnimatedLogoUrl = logo != null ? logo.animatedLogoUrl : null;
                 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);
             }
         };
 
         mLogoBridge.getCurrentLogo(wrapperCallback);
     }
 }