[CustomTabs] Report the navigation start as absolute time

chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java

Index: chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java b/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
index 8a586b14d79574446bd4eed4ac43f8c9d10a8883..1e9d96e99496c506062026cabdef509c9d3a343b 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
@@ -19,6 +19,7 @@ import android.os.Build;
 import android.os.Bundle;
 import android.os.Process;
 import android.os.StrictMode;
+import android.os.SystemClock;
 import android.support.customtabs.CustomTabsCallback;
 import android.support.customtabs.CustomTabsIntent;
 import android.support.customtabs.CustomTabsService;
@@ -30,6 +31,7 @@ import org.chromium.base.CommandLine;
 import org.chromium.base.Log;
 import org.chromium.base.SysUtils;
 import org.chromium.base.ThreadUtils;
+import org.chromium.base.TimeUtils;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.base.annotations.SuppressFBWarnings;
 import org.chromium.base.library_loader.ProcessInitException;
@@ -41,6 +43,7 @@ import org.chromium.chrome.browser.IntentHandler;
 import org.chromium.chrome.browser.WarmupManager;
 import org.chromium.chrome.browser.device.DeviceClassManager;
 import org.chromium.chrome.browser.init.ChromeBrowserInitializer;
+import org.chromium.chrome.browser.metrics.PageLoadMetrics;
 import org.chromium.chrome.browser.net.spdyproxy.DataReductionProxySettings;
 import org.chromium.chrome.browser.preferences.PrefServiceBridge;
 import org.chromium.chrome.browser.prerender.ExternalPrerenderHandler;
@@ -680,12 +683,21 @@ public class CustomTabsConnection {
      *
      * @param session Session identifier.
      * @param metricName Name of the page load metric.
+     * @param navigationStartTick Absolute navigation start time, as TimeTicks.
      * @param offsetMs Offset in ms from navigationStart.
      */
-    boolean notifyPageLoadMetric(CustomTabsSessionToken session, String metricName, long offsetMs) {
+    boolean notifyPageLoadMetric(CustomTabsSessionToken session, String metricName,
+            long navigationStartTick, long offsetMs) {
         CustomTabsCallback callback = mClientManager.getCallbackForSession(session);
         if (callback == null) return false;
+
+        // Convert navigationstart from time ticks to uptimeMillis.
+        long nativeNowUs = TimeUtils.nativeGetTimeTicksNowUs();
+        long javaNowUs = SystemClock.uptimeMillis() * 1000;
+        long offsetTick = nativeNowUs - javaNowUs;

[Bernhard Bauer, 2016/11/18 11:53:44]

This offset will be ever so slightly different every time you run this code,
right? Would it make sense to calculate the offset once lazily and use that? You
could put that into TimeUtils as well.

[droger, 2016/11/18 13:43:39]

In practice yes, and the offset will also be 0, because uptimeMillis and
TimeTicks are the same thing.
However, the goal of doing that is to support the case where the implementations
are different.

So it all depends if we assume that the offset can change over time or not. I
don't really have a strong opinion on that.

[Bernhard Bauer, 2016/11/18 14:07:00]

Almost 0, because we call the two methods one after the other, right?

Basically, this seems like it adds jitter to the returned value for no good
reason, and for a client that is interested in timing, that seems less than
ideal. Caching the offset value also seems to be a pattern that is used in other
places when converting between different time representations; see for example
blink::DocumentLoadTiming::monotonicTimeToPseudoWallTime()
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Do...).

[droger, 2016/11/18 15:14:09]

Right, but I don't understand the point you're trying to make.
Although please realize that we're talking about sub-millisecond noise, which is
completely negligible on the scale of a page load time.
It would also affect only a client that would open a large number of CustomTabs
in the same run. I don't think such a client exist.
I gave the reason above: this is to account for uptimeMillis and TimeTicks
having potentially different implementations.
This is indeed arguable: maybe if one of the implementations change, they would
only change the time reference, and not when they suspend the clock. If we
assume this, then it is fine to cache the clock.
This creates the risk of errors if they start suspending their clock differently
though.

If you think this reason is not good, then fine with me.
Not caching the offset is also a pattern done in other places:
https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/...

Note also that in the specific example you pointed, that is a conversion between
TimeTicks and wall time, which explicitly don't have a constant offset, and thus
the timings will become wrong if the device goes to sleep in the middle. We
don't have this problem in this CL though, as long as uptimeMillis and TimeTicks
have a constant offset.


Anyway, I don't think any of this matters in practice, so done.

+
         Bundle args = new Bundle();
+        args.putLong(PageLoadMetrics.NAVIGATION_START, (navigationStartTick - offsetTick) / 1000);
         args.putLong(metricName, offsetMs);
         try {
             callback.extraCallback(PAGE_LOAD_METRICS_CALLBACK, args);