Custom Tabs: add speculation status histograms.

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 0e71b3ea77e622d1756d6bca214cac7c0d676f32..b50f5f849ff29cf6a3890ae0d8b1e718633d76b2 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
@@ -80,6 +80,32 @@ public class CustomTabsConnection {
     @VisibleForTesting
     static final String PAGE_LOAD_METRICS_CALLBACK = "NavigationMetrics";
 
+    // For CustomTabs.SpeculationStatus, see histograms.xml. Append only.
+    private static final String SPECULATION_STATUS_TAG = "CustomTabs.SpeculationStatus";
+    // Speculation is allowed, but may not have been used.
+    private static final int SPECULATION_STATUS_ALLOWED = 0;
+    // What kind of speculation was started, counted in addition to
+    // SPECULATION_STATUS_ALLOWED.
+    private static final int SPECULATION_STATUS_PREFETCH = 1;
+    private static final int SPECULATION_STATUS_PRERENDER = 2;
+    private static final int SPECULATION_STATUS_BACKGROUND_TAB = 3;

[pasko, 2017/05/15 12:56:14]

Putting these into the same histogram makes it hard for interpretation because:

1. need to sum up a select of buckets to see discrepancy caused by browser kills
   between allowing and taking.

2. it is not clear about the time of histogram recording. Describing when each
   histogram is recorded is very important for histogram maintenance, otherwise
   there are a lot of confusions and mis-interpretation of the data, we need to
   keep these descriptions short and somewhat intuitive.

For the reasons above I am leaning towards having two histograms:
1. CustomTabs.SpeculationStatusOnStart
   SPECULATION_STATUS_ALLOWED (not necessary since it is
PREFETCH+PRERENDER+BACKGROUND_TAB)
   PREFETCH
   PRERENDER
   BACKGROUND_TAB
   ABORTED_DEVICE_CLASS
   ABORTED_BLOCK_3RD_PARTY_COOKIES
   NETWORK_PREDICTION_DISABLED // nit: s/NETWORK/ABORTED_NETWORK/
   DATA_REDUCTION_ENABLED // nit: s/DATA/ABORTED_DATA/
   ABORTED_NETWORK_METERED
2. CustomTabs.SpeculationStatusOnSwap
   remaining

Having ALLOWED seems more readable, and the overhead is just one bucket, so
let's have it for these reasons. Having to sum up 3 buckets on the dashboard to
get the number is not hard, this option would have been okay as well (modulo a
longer description and knowing what to sum up).

Note: we did not include prerender timeouts and other FinalStatus-es, so the
total counts would not match easily with those on the Prerender level. Seems
like not a big problem, just wanted to bring it in case you have thoughts on why
it may complicate interpretation.

mattcary@ wrote:
Clear separation of the _time_ when the histogram is recorded is critically
important. Having 2 histograms does not partition too much, one can observe them
in one dashboard page, with almost the same UX as one histogram, but with extra
good separation in naming and clearer descriptions. Please do.

[mattcary, 2017/05/15 13:47:19]

What exactly do you mean here? Can you provide an example?

[pasko, 2017/05/15 15:26:26]

we have two potentially distant moments in time:
1. the call to maySpeculate()
2. the call to takePrerender() // or its analogy for other types

if the browser were never not killed in between those, we could say 'hey, the
count(maySpeculate) - count(takePrerender) associates with renderer OOM /
timeouts, other FinalStatus-es', which we would then be able to split by reason
using flavors of Prerender.FinalStatus. But we would have a discrepancy when (1)
happens, but (2) does not in quite a few cases .. like chrome going to bg, being
killed there, having time to successfully flush the uma logs with info from (1),
but loosing either the logs from (2) or actually not executing it. 

Now given that we have two separate histograms for the 2 moments, we would have
a good way to tell how many maySpeculate()s did not follow up with a
takePrerender(). Neat?

+    // Whether a background tab is used. These are counted in addition to
+    // SPECULATION_STATUS_ALLOWED.
+    private static final int SPECULATION_STATUS_BACKGROUND_TAB_TAKEN = 4;
+    private static final int SPECULATION_STATUS_BACKGROUND_TAB_NOT_MATCHED = 5;
+    // Status of a prerender. These are counted in addition to SPECULATION_STATUS_ALLOWED.
+    private static final int SPECULATION_STATUS_PRERENDER_NOT_STARTED = 6;
+    private static final int SPECULATION_STATUS_PRERENDER_TAKEN = 7;
+    private static final int SPECULATION_STATUS_PRERENDER_NOT_MATCHED = 8;
+    // The following describe reasons why a speculation was aborted at some point, and are
+    // counted instead of SPECULATION_STATUS_ALLOWED.
+    private static final int SPECULATION_STATUS_ABORTED_DEVICE_CLASS = 9;
+    private static final int SPECULATION_STATUS_ABORTED_BLOCK_3RD_PARTY_COOKIES = 10;
+    private static final int SPECULATION_STATUS_ABORTED_NETWORK_PREDICTION_DISABLED = 11;
+    private static final int SPECULATION_STATUS_ABORTED_DATA_REDUCTION_ENABLED = 12;
+    private static final int SPECULATION_STATUS_ABORTED_NETWORK_METERED = 13;
+    private static final int SPECULATION_STATUS_MAX = 14;
+
     // For testing only, DO NOT USE.
     @VisibleForTesting
     static final String DEBUG_OVERRIDE_KEY =
@@ -616,9 +642,13 @@ public class CustomTabsConnection {
                         && UrlUtilities.urlsMatchIgnoringFragments(prerenderedUrl, url));
         WebContents result = null;
         if (urlsMatch && TextUtils.equals(prerenderReferrer, referrer)) {
+            RecordHistogram.recordEnumeratedHistogram(SPECULATION_STATUS_TAG,

[pasko, 2017/05/15 12:56:14]

it would be nice to shorten this common call as something like
RecordSpeculationStatus(SPECULATION_STATUS_PRERENDER_TAKEN). This would be
shorter and less error prone with the use of TAG and MAX.

[mattcary, 2017/05/16 07:51:10]

Done.

+                    SPECULATION_STATUS_PRERENDER_TAKEN, SPECULATION_STATUS_MAX);
             result = webContents;
             mSpeculation = null;
         } else {
+            RecordHistogram.recordEnumeratedHistogram(SPECULATION_STATUS_TAG,
+                    SPECULATION_STATUS_PRERENDER_NOT_MATCHED, SPECULATION_STATUS_MAX);
             cancelSpeculation(session);
         }
         if (!mClientManager.usesDefaultSessionParameters(session) && webContents != null) {
@@ -671,8 +701,12 @@ public class CustomTabsConnection {
                                    && UrlUtilities.urlsMatchIgnoringFragments(speculatedUrl, url));
                 if (referrer == null) referrer = "";
                 if (urlsMatch && TextUtils.equals(speculationReferrer, referrer)) {
+                    RecordHistogram.recordEnumeratedHistogram(SPECULATION_STATUS_TAG,
+                            SPECULATION_STATUS_BACKGROUND_TAB_TAKEN, SPECULATION_STATUS_MAX);
                     return tab;
                 } else {
+                    RecordHistogram.recordEnumeratedHistogram(SPECULATION_STATUS_TAG,
+                            SPECULATION_STATUS_BACKGROUND_TAB_NOT_MATCHED, SPECULATION_STATUS_MAX);
                     tab.destroy();
                 }
             }
@@ -953,19 +987,37 @@ public class CustomTabsConnection {
     }
 
     @VisibleForTesting
-    boolean maySpeculate(CustomTabsSessionToken session) {
-        if (!DeviceClassManager.enablePrerendering()) return false;
+    int maySpeculateWithResult(CustomTabsSessionToken session) {
+        if (!DeviceClassManager.enablePrerendering()) {
+            return SPECULATION_STATUS_ABORTED_DEVICE_CLASS;
+        }
         PrefServiceBridge prefs = PrefServiceBridge.getInstance();
-        if (prefs.isBlockThirdPartyCookiesEnabled()) return false;
+        if (prefs.isBlockThirdPartyCookiesEnabled()) {
+            return SPECULATION_STATUS_ABORTED_BLOCK_3RD_PARTY_COOKIES;
+        }
         // TODO(yusufo): The check for prerender in PrivacyManager now checks for the network

[pasko, 2017/05/15 12:56:14]

I think Yusuf wanted to say 'PrivacyPreferencesManager', please change it this
way

[mattcary, 2017/05/16 07:51:10]

Done.

         // connection type as well, we should either change that or add another check for custom
         // tabs. Then PrivacyManager should be used to make the below check.
-        if (!prefs.getNetworkPredictionEnabled()) return false;
-        if (DataReductionProxySettings.getInstance().isDataReductionProxyEnabled()) return false;
+        if (!prefs.getNetworkPredictionEnabled()) {
+            return SPECULATION_STATUS_ABORTED_NETWORK_PREDICTION_DISABLED;
+        }
+        if (DataReductionProxySettings.getInstance().isDataReductionProxyEnabled()) {
+            return SPECULATION_STATUS_ABORTED_DATA_REDUCTION_ENABLED;
+        }
         ConnectivityManager cm =
                 (ConnectivityManager) mApplication.getApplicationContext().getSystemService(
                         Context.CONNECTIVITY_SERVICE);
-        return !cm.isActiveNetworkMetered() || shouldPrerenderOnCellularForSession(session);
+        if (cm.isActiveNetworkMetered() && !shouldPrerenderOnCellularForSession(session)) {
+            return SPECULATION_STATUS_ABORTED_NETWORK_METERED;
+        }
+        return SPECULATION_STATUS_ALLOWED;
+    }
+
+    boolean maySpeculate(CustomTabsSessionToken session) {
+        int speculationResult = maySpeculateWithResult(session);
+        RecordHistogram.recordEnumeratedHistogram(
+                SPECULATION_STATUS_TAG, speculationResult, SPECULATION_STATUS_MAX);
+        return speculationResult == SPECULATION_STATUS_ALLOWED;
     }
 
     /** Cancels the speculation for a given session, or any session if null. */
@@ -1005,15 +1057,25 @@ public class CustomTabsConnection {
         }
         switch (speculationMode) {
             case SpeculationParams.PREFETCH:
+                RecordHistogram.recordEnumeratedHistogram(SPECULATION_STATUS_TAG,
+                        SPECULATION_STATUS_PREFETCH, SPECULATION_STATUS_MAX);
                 boolean didPrefetch = new ResourcePrefetchPredictor(profile).startPrefetching(url);
                 if (didPrefetch) mSpeculation = SpeculationParams.forPrefetch(session, url);
                 preconnect = !didPrefetch;
                 break;
             case SpeculationParams.PRERENDER:
+                RecordHistogram.recordEnumeratedHistogram(SPECULATION_STATUS_TAG,
+                        SPECULATION_STATUS_PRERENDER, SPECULATION_STATUS_MAX);
                 boolean didPrerender = prerenderUrl(session, url, extras, uid);
+                if (!didPrerender) {
+                    RecordHistogram.recordEnumeratedHistogram(SPECULATION_STATUS_TAG,
+                            SPECULATION_STATUS_PRERENDER_NOT_STARTED, SPECULATION_STATUS_MAX);
+                }
                 createSpareWebContents = !didPrerender;
                 break;
             case SpeculationParams.HIDDEN_TAB:
+                RecordHistogram.recordEnumeratedHistogram(SPECULATION_STATUS_TAG,
+                        SPECULATION_STATUS_BACKGROUND_TAB, SPECULATION_STATUS_MAX);
                 launchUrlInHiddenTab(session, url, extras);
                 break;
             default: