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..dfa740b77ad11cda84e2a76f3628ac3beb47d227 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
@@ -103,6 +103,15 @@ public class LogoDelegateImpl implements LogoView.Delegate {
                 }
                 logoObserver.onLogoAvailable(logo, fromCache);
             }
+
+            @Override
+            public void onLogoRefreshSkipped() {

[tschumann, 2017/04/20 18:53:11]

omg, this is really convoluted.
Is there any good reason why the metric does not live in the doodle service
where we can properly connect the triggering and completion of a request?

(1) If the metrics need to live in Java, couldn't we just collect the load-time
in the doodle service and pass it into onLogoAvailable?

(2) This would also allow to only register a single observer (no need to
unregister the old ones in every call to getCurrentLogo() -- which is also quite
unexpected). Looking at the call sites of getSearchProviderLogo() it doesn't
seem like there's an actual need to provide new instances every time.

I might be missing something, of course. As we intent to merge this into M59,
I'd suggest to do (2) in a separate CL.

[fhorschig, 2017/04/24 13:35:56]

That's why I really don't want to defend this. But...
As far as I understood, this metric really wants to capture the end-to-end
experience from "This NTP is shown" to "The logo is finally visible".
We do have separate metrics for Doodle.ConfigDownloadTime and 
NewTabPage.LogoDownloadTime.
Moving this into the service would capture a similar value but exempt the
drawing of the UI.
The problem is, that we (afaik) need this metric to measure the _change_
that the new Doodle component brings compared to the old LogoTracker. 
Therefore, moving it into the Service requires us to duplicate the 
measurement code for the legacy LogoTracker as well.
See the reason above. This definitely makes a lot of sense when the legacy code
gets removed. Currently, we could still do this, with a lot of code duplication.
WDYT is more appropriate here?

BTW: We still need a separate callback/observer/boolean to notify about the 
skipped refresh. Passing the load time along the result would not suffice to
fix what this CL currently fixes.
It is indeed unexpected to see observers frequently reregistered. 
Maybe it would have been a better idea to call it CallbackWrapper (which is 
the case on the Java side and makes their use much clearer).
I am not sure about the side effects of making the OnceCallbacks persistent.
At least, the CallbackWrapper initializes a member for metrics on construction.
I agree. In fact, changing the observer to pass the loading time requires a lot
of changes in different places. Therefore, I would really like to go for a
simple
solution first.

+                if (mIsDestroyed) return;
+                if (mIsDestroyed) return;
+                // If the cache was empty but the refresh was skipped, we won't get a better logo
+                // than the logo we don't have.
+                mHasRecordedLoadTime = true;
+            }
         };
 
         mLogoBridge.getCurrentLogo(wrapperCallback);