hosted: Add two histograms tracking "mayLaunchUrl" usage.

chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java

Index: chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java
diff --git a/chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java b/chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java
index 363c99f412b4e13c16a1f8dc62272eb1139ee3e3..8545ec872150dcadb37129ca3a88bf272f783493 100644
--- a/chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java
+++ b/chrome/android/java_staging/src/org/chromium/chrome/browser/hosted/ChromeBrowserConnection.java
@@ -17,6 +17,7 @@ import android.os.Binder;
 import android.os.Bundle;
 import android.os.IBinder;
 import android.os.RemoteException;
+import android.os.SystemClock;
 import android.util.LongSparseArray;
 import android.util.SparseArray;
 
@@ -24,6 +25,7 @@ import org.chromium.base.Log;
 import org.chromium.base.ThreadUtils;
 import org.chromium.base.annotations.SuppressFBWarnings;
 import org.chromium.base.library_loader.ProcessInitException;
+import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.chrome.browser.ChromiumApplication;
 import org.chromium.chrome.browser.WarmupManager;
 import org.chromium.content.browser.ChildProcessLauncher;
@@ -33,6 +35,7 @@ import java.security.SecureRandom;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
 
 /**
@@ -42,6 +45,9 @@ class ChromeBrowserConnection extends IBrowserConnectionService.Stub {
     private static final String TAG = Log.makeTag("ChromeConnection");
     private static final long RESULT_OK = 0;
     private static final long RESULT_ERROR = -1;
+    private static final int NO_PREDICTION = 0;
+    private static final int GOOD_PREDICTION = 1;
+    private static final int BAD_PREDICTION = 2;
 
     private static final Object sConstructionLock = new Object();
     private static ChromeBrowserConnection sInstance;
@@ -49,10 +55,45 @@ class ChromeBrowserConnection extends IBrowserConnectionService.Stub {
     private final Application mApplication;
     private final AtomicBoolean mWarmupHasBeenCalled;
 
+    /** Per-sessionId values. */
+    private static class SessionParams {
+        public final int mUid;
+        private ServiceConnection mServiceConnection;
+        private String mPredictedUrl;
+        private long mLastMayLaunchUrlTimestamp;
+
+        public SessionParams(int uid) {
+            mUid = uid;
+            mServiceConnection = null;
+            mPredictedUrl = null;
+            mLastMayLaunchUrlTimestamp = 0;
+        }
+
+        public ServiceConnection getServiceConnection() {
+            return mServiceConnection;
+        }
+
+        public void setServiceConnection(ServiceConnection serviceConnection) {
+            mServiceConnection = serviceConnection;
+        }
+
+        public void setPredictionMetrics(String predictedUrl, long lastMayLaunchUrlTimestamp) {
+            mPredictedUrl = predictedUrl;
+            mLastMayLaunchUrlTimestamp = lastMayLaunchUrlTimestamp;
+        }
+
+        public String getPredictedUrl() {
+            return mPredictedUrl;
+        }
+
+        public long getLastMayLaunchUrlTimestamp() {
+            return mLastMayLaunchUrlTimestamp;
+        }
+    }
+
     private final Object mLock;
     private final SparseArray<IBrowserConnectionCallback> mUidToCallback;
-    private final LongSparseArray<Integer> mSessionIdToUid;
-    private final LongSparseArray<ServiceConnection> mSessionIdToServiceConnection;
+    private final LongSparseArray<SessionParams> mSessionParams;
 
     private ChromeBrowserConnection(Application application) {
         super();
@@ -60,8 +101,7 @@ class ChromeBrowserConnection extends IBrowserConnectionService.Stub {
         mWarmupHasBeenCalled = new AtomicBoolean();
         mLock = new Object();
         mUidToCallback = new SparseArray<IBrowserConnectionCallback>();
-        mSessionIdToUid = new LongSparseArray<Integer>();
-        mSessionIdToServiceConnection = new LongSparseArray<ServiceConnection>();
+        mSessionParams = new LongSparseArray<SessionParams>();
     }
 
     /**
@@ -143,8 +183,8 @@ class ChromeBrowserConnection extends IBrowserConnectionService.Stub {
                 // Because Math.abs(Long.MIN_VALUE) == Long.MIN_VALUE.
                 if (sessionId == Long.MIN_VALUE) continue;
                 sessionId = Math.abs(sessionId);
-            } while (sessionId == 0 || mSessionIdToUid.get(sessionId) != null);
-            mSessionIdToUid.put(sessionId, Binder.getCallingUid());
+            } while (sessionId == 0 || mSessionParams.get(sessionId) != null);
+            mSessionParams.put(sessionId, new SessionParams(Binder.getCallingUid()));
             return sessionId;
         }
     }
@@ -155,9 +195,9 @@ class ChromeBrowserConnection extends IBrowserConnectionService.Stub {
         int uid = Binder.getCallingUid();
         if (!isUidForeground(uid)) return RESULT_ERROR;
         synchronized (mLock) {
-            if (mSessionIdToUid.get(sessionId) == null || mSessionIdToUid.get(sessionId) != uid) {
-                return RESULT_ERROR;
-            }
+            SessionParams sessionParams = mSessionParams.get(sessionId);
+            if (sessionParams == null || sessionParams.mUid != uid) return RESULT_ERROR;
+            sessionParams.setPredictionMetrics(url, SystemClock.elapsedRealtime());
         }
         ThreadUtils.postOnUiThread(new Runnable() {
             @Override
@@ -171,6 +211,35 @@ class ChromeBrowserConnection extends IBrowserConnectionService.Stub {
     }
 
     /**
+     * Registers a launch of a |url| for a given |sessionId|.
+     *
+     * This is used for accounting.
+     */
+    void registerLaunch(long sessionId, String url) {
+        int outcome;
+        long elapsedTimeMs = -1;
+        synchronized (mLock) {
+            SessionParams sessionParams = mSessionParams.get(sessionId);
+            if (sessionParams == null) {
+                outcome = NO_PREDICTION;
+            } else {
+                String predictedUrl = sessionParams.getPredictedUrl();
+                outcome = predictedUrl == null ? NO_PREDICTION
+                        : predictedUrl.equals(url) ? GOOD_PREDICTION : BAD_PREDICTION;
+                elapsedTimeMs = SystemClock.elapsedRealtime()
+                        - sessionParams.getLastMayLaunchUrlTimestamp();
+                sessionParams.setPredictionMetrics(null, 0);
+            }
+        }
+        RecordHistogram.recordEnumeratedHistogram(
+                "CustomTabs.PredictionStatus", outcome, BAD_PREDICTION + 1);

[Ilya Sherman, 2015/06/03 22:07:32]

Using "BAD_PREDICTION + 1" as the bound is fragile.  It would be easy for
someone to add another possible outcome later on, without realizing that they
need to update this particular line.  Instead, I'd recommend defining an enum
specifically for the values that can be emitted to this histogram, structured as
"enum Foo {A, B, C, FOO_COUNT}", and passing FOO_COUNT as the boundary value. 
Also, please add documentation to the enum noting that it is used to back a
histogram, and thus that it should be treated as append-only.

[Benoit L, 2015/06/04 11:16:29]

You're right, added the boundary value and the comment.
I'm not using a Java enum as it's against the recommended practices on Android
(see http://developer.android.com/training/articles/memory.html#Overhead ).
Done.

+        if (outcome == GOOD_PREDICTION) {
+            RecordHistogram.recordCustomTimesHistogram("CustomTabs.PredictionToLaunch",
+                    elapsedTimeMs, 0, 3 * 60 * 1000, TimeUnit.MILLISECONDS, 100);

[Ilya Sherman, 2015/06/03 22:07:31]

nit: Could you please spell "3 * 60 * 1000" as TimeUnit.SECONDS.toMillis(3)? 
That's much clearer for the reader.

[Ilya Sherman, 2015/06/03 22:07:31]

nit: Technically, the minimum value for a time histogram is 1, rather than 0 --
bucket 0 is the underflow bucket.

[Benoit L, 2015/06/04 11:16:29]

Done.

[Benoit L, 2015/06/04 11:16:29]

Done.

+        }
+    }
+
+    /**
      * Transfers a prerendered WebContents if one exists.
      *
      * This resets the internal WebContents; a subsequent call to this method
@@ -198,9 +267,9 @@ class ChromeBrowserConnection extends IBrowserConnectionService.Stub {
      */
     boolean deliverOnUserNavigationCallback(long sessionId, String url, Bundle extras) {
         synchronized (mLock) {
-            if (mSessionIdToUid.get(sessionId) == null) return false;
-            int uid = mSessionIdToUid.get(sessionId);
-            IBrowserConnectionCallback cb = mUidToCallback.get(uid);
+            SessionParams sessionParams = mSessionParams.get(sessionId);
+            if (sessionParams == null) return false;
+            IBrowserConnectionCallback cb = mUidToCallback.get(sessionParams.mUid);
             if (cb == null) return false;
             try {
                 cb.onUserNavigation(sessionId, url, extras);
@@ -227,16 +296,16 @@ class ChromeBrowserConnection extends IBrowserConnectionService.Stub {
         // be at least equal to the application's one. This binds to a dummy
         // service (no calls to this service are made).
         if (intent == null || intent.getComponent() == null) return false;
-        int uid;
+        SessionParams sessionParams;
         synchronized (mLock) {
-            if (mSessionIdToUid.get(sessionId) == null) return false;
-            uid = mSessionIdToUid.get(sessionId);
+            sessionParams = mSessionParams.get(sessionId);
+            if (sessionParams == null) return false;
         }
         String packageName = intent.getComponent().getPackageName();
         PackageManager pm = mApplication.getApplicationContext().getPackageManager();
         // Only binds to the application associated to this session ID.
+        int uid = sessionParams.mUid;
         if (!Arrays.asList(pm.getPackagesForUid(uid)).contains(packageName)) return false;
-
         Intent serviceIntent = new Intent().setComponent(intent.getComponent());
         // This ServiceConnection doesn't handle disconnects. This is on
         // purpose, as it occurs when the remote process has died. Since the
@@ -256,7 +325,7 @@ class ChromeBrowserConnection extends IBrowserConnectionService.Stub {
         } catch (SecurityException e) {
             return false;
         }
-        if (ok) mSessionIdToServiceConnection.put(sessionId, connection);
+        if (ok) sessionParams.setServiceConnection(connection);
         return ok;
     }
 
@@ -268,10 +337,14 @@ class ChromeBrowserConnection extends IBrowserConnectionService.Stub {
      * @param sessionId Session ID, as provided to {@link keepAliveForSessionId}.
      */
     void dontKeepAliveForSessionId(long sessionId) {
-        ServiceConnection connection = mSessionIdToServiceConnection.get(sessionId);
-        if (connection == null) return;
-        mSessionIdToServiceConnection.remove(sessionId);
-        mApplication.getApplicationContext().unbindService(connection);
+        SessionParams sessionParams;
+        synchronized (mLock) {
+            sessionParams = mSessionParams.get(sessionId);
+        }
+        if (sessionParams == null || sessionParams.getServiceConnection() == null) return;
+        ServiceConnection serviceConnection = sessionParams.getServiceConnection();
+        sessionParams.setServiceConnection(null);
+        mApplication.getApplicationContext().unbindService(serviceConnection);
     }
 
     /**
@@ -296,11 +369,11 @@ class ChromeBrowserConnection extends IBrowserConnectionService.Stub {
         List<Long> keysToRemove = new ArrayList<Long>();
         // TODO(lizeb): If iterating through all the session IDs is too costly,
         // use two mappings.
-        for (int i = 0; i < mSessionIdToUid.size(); i++) {
-            if (mSessionIdToUid.valueAt(i) == uid) keysToRemove.add(mSessionIdToUid.keyAt(i));
+        for (int i = 0; i < mSessionParams.size(); i++) {
+            if (mSessionParams.valueAt(i).mUid == uid) keysToRemove.add(mSessionParams.keyAt(i));
         }
         for (Long sessionId : keysToRemove) {
-            mSessionIdToUid.remove(sessionId);
+            mSessionParams.remove(sessionId);
         }
         mUidToCallback.remove(uid);
     }