[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..0078629cdfe3d22481f932d07c498d4f5edabd9e 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;
@@ -127,6 +130,8 @@ public class CustomTabsConnection {
     private final AtomicBoolean mWarmupHasBeenCalled = new AtomicBoolean();
     private final AtomicBoolean mWarmupHasBeenFinished = new AtomicBoolean();
     private ExternalPrerenderHandler mExternalPrerenderHandler;
+    private long mOffsetTick;

[pasko, 2016/11/21 17:16:37]

saw this in Benoit's rebase, thoughts:

1. s/mOffsetTick/mNativeTickOffsetUs/ would be more explicit?

2. boolean members are false by default in Java. Explicit assignment here
generates
more code in <init> and afair is considered bad practice by Java community
(however, I failed to find any reference to that in 2 minutes).

+    private boolean mOffsetTickComputed = false;
 
     /**
      * <strong>DO NOT CALL</strong>
@@ -680,12 +685,24 @@ public class CustomTabsConnection {
      *
      * @param session Session identifier.
      * @param metricName Name of the page load metric.
+     * @param navigationStartTick Absolute navigation start time, as TimeTicks.

[pasko, 2016/11/21 17:16:37]

3. I guess it needs to say something about the tick being taken from the 'native
side'?

      * @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;
+
+        if (!mOffsetTickComputed) {
+            // Compute offset from time ticks to uptimeMillis.
+            mOffsetTickComputed = true;
+            long nativeNowUs = TimeUtils.nativeGetTimeTicksNowUs();
+            long javaNowUs = SystemClock.uptimeMillis() * 1000;
+            mOffsetTick = nativeNowUs - javaNowUs;

[pasko, 2016/11/21 17:16:37]

aren't these two coming from different syscalls? Are we confident that the
offset is going to stay the same during process lifetime?

[droger, 2016/11/21 17:28:02]

In practice, they will stay the same, although this may be considered as an
implementation detail and not guaranteed.
There was already a debate on this, so if you're ok let's not reopen it?

[pasko, 2016/11/21 18:56:11]

I do not quite remember the details of the discussion (perhaps I was not
participating), do you have a summary documented somewhere? If it's an
implementation detail, perhaps some pointer exists to show what particular part
of the implementation makes sure these remain with a constant offset?

I am less worried about this specific use, but more about this assumption being
copied around the codebase, where the exact guarantee would not be provided.
Oh, I am trying to learn the conclusion of the debate, not attempting to reopen
the debate. Sorry if it looks so.

[Bryan McQuade, 2016/11/21 19:05:53]

Yeah - I think the conclusion was that though both of these are currently backed
by the same clock currently (clock_gettime(CLOCK_MONOTONIC)), that could
potentially change and thus droger determined that we could control for that by
taking the delta here, which i think is a nice approach. the remaining
assumptions are that the clocks will continue to be monotonic, which seems
reasonable, and to a lesser extent that they'll both continue to not tick during
power saving mode, which i think is also reasonable. This was considered the
best overall tradeoff in providing a high quality reference time to the CCT
embedder, which doesn't have access to TimeTicks. we did not like using
non-monotonic clock such as epoch wall time as reference - even though that's
the most standardized clock available to consumers, it can suffer from clock
skew, and we felt that providing a high quality monotonic reference time was
more important here.

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