CustomTabs: Base version for prerender switch

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 4c3eee8e7a1e3ffcf42febc2b7ac428fd24caedf..bd38e643e0fb5b0e0befffae414d74d3d0469bd7 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
@@ -83,7 +83,8 @@ public class CustomTabsConnection {
     @VisibleForTesting
     static final String DEBUG_OVERRIDE_KEY =
             "android.support.customtabs.maylaunchurl.DEBUG_OVERRIDE";
-    private static final int NO_OVERRIDE = 0;
+    @VisibleForTesting
+    static final int NO_OVERRIDE = 0;
     @VisibleForTesting
     static final int NO_PRERENDERING = 1;
     @VisibleForTesting
@@ -96,31 +97,30 @@ public class CustomTabsConnection {
     static final class SpeculationParams {
         public final CustomTabsSessionToken session;
         public final String url;
+        public final int debugOverrideValue;

[ahemery1, 2017/03/21 15:15:57]

Replaces prefetchOnly to have a record of what the speculation was in a more
general sense.

[Benoit L, 2017/03/21 15:56:41]

I would prefer a name like "mode", or something along these lines. Could take
the values of PRERENDER, PREFETCH_ONLY, and more soon.

PREFETCH_ONLY would be controlled by the value of DEBUG_OVERRIDE, and hidden tab
by the per-session setting, or when Chrome calls itself (from the tests).

 
         // Only for prerender.
         public final WebContents webContents;
         public final String referrer;
         public final Bundle extras;
 
-        public final boolean prefetchOnly;
+        static SpeculationParams forPrefetch(CustomTabsSessionToken session, String url) {

[ahemery1, 2017/03/21 15:15:58]

Just reordered.

[Benoit L, 2017/03/21 15:56:41]

why?
Not an issue, but why reordering code?

+            return new SpeculationParams(session, url, PREFETCH_ONLY, null, null, null);
+        }
 
         static SpeculationParams forPrerender(CustomTabsSessionToken session, String url,
                 WebContents webcontents, String referrer, Bundle extras) {
-            return new SpeculationParams(session, url, webcontents, referrer, extras, false);
-        }
-
-        static SpeculationParams forPrefetch(CustomTabsSessionToken session, String url) {
-            return new SpeculationParams(session, url, null, null, null, true);
+            return new SpeculationParams(session, url, NO_OVERRIDE, webcontents, referrer, extras);
         }
 
         private SpeculationParams(CustomTabsSessionToken session, String url,
-                WebContents webContents, String referrer, Bundle extras, boolean prefetchOnly) {
+                int debugOverrideValue, WebContents webContents, String referrer, Bundle extras) {
             this.session = session;
             this.url = url;
+            this.debugOverrideValue = debugOverrideValue;
             this.webContents = webContents;
             this.referrer = referrer;
             this.extras = extras;
-            this.prefetchOnly = prefetchOnly;
         }
     }
 
@@ -333,6 +333,8 @@ public class CustomTabsConnection {
                 didStartPrefetch = new ResourcePrefetchPredictor(profile).startPrefetching(url);
                 if (didStartPrefetch) mSpeculation = SpeculationParams.forPrefetch(session, url);
             } else if (debugOverrideValue != NO_PRERENDERING) {
+                // Last one wins and cancels the previous prerender.

[ahemery1, 2017/03/21 15:15:57]

This was moved from inside the prerenderUrl() function. It is private so we are
in control of where this is used and we should have no problem. Factoring
behavior common for both prerender and hidden tab.

[Benoit L, 2017/03/21 15:56:41]

nit: Is it possible to isolate the changes between the various CLs? Makes it
easier to review and understand.
Not a big issue here.

+                cancelPrerender(null);
                 didStartPrerender = prerenderUrl(session, url, extras, uid);
             }
         }
@@ -587,7 +589,7 @@ public class CustomTabsConnection {
             return null;
         }
 
-        if (mSpeculation.prefetchOnly) {
+        if (mSpeculation.debugOverrideValue == PREFETCH_ONLY) {
             Profile profile = Profile.getLastUsedProfile();
             new ResourcePrefetchPredictor(profile).stopPrefetching(mSpeculation.url);
             mSpeculation = null;
@@ -901,16 +903,27 @@ public class CustomTabsConnection {
     }
 
     /** Cancels a prerender for a given session, or any session if null. */
+    @VisibleForTesting
     void cancelPrerender(CustomTabsSessionToken session) {
-        ThreadUtils.assertOnUiThread();
-        if (mSpeculation != null && (session == null || session.equals(mSpeculation.session))
-                && mSpeculation.webContents != null) {
-            mExternalPrerenderHandler.cancelCurrentPrerender();
-            mSpeculation.webContents.destroy();
-            mSpeculation = null;
+        if (mSpeculation == null) return;
+        if (session != null && !session.equals(mSpeculation.session)) return;
+        switch (mSpeculation.debugOverrideValue) {
+            case NO_OVERRIDE:
+                cancelNoOverridePrerender(session);
+                break;
+            default:
+                return;
         }
     }
 
+    private void cancelNoOverridePrerender(CustomTabsSessionToken session) {
+        ThreadUtils.assertOnUiThread();
+        if (mSpeculation.webContents == null) return;
+        mExternalPrerenderHandler.cancelCurrentPrerender();
+        mSpeculation.webContents.destroy();
+        mSpeculation = null;
+    }
+
     /**
      * Tries to request a prerender for a given URL.
      *
@@ -923,12 +936,8 @@ public class CustomTabsConnection {
     private boolean prerenderUrl(
             CustomTabsSessionToken session, String url, Bundle extras, int uid) {
         ThreadUtils.assertOnUiThread();
-        // Ignores mayPrerender() for an empty URL, since it cancels an existing prerender.
-        if (!mayPrerender(session) && !TextUtils.isEmpty(url)) return false;

[ahemery1, 2017/03/21 15:15:57]

Unnecessary duplicate.

         if (!mWarmupHasBeenCalled.get()) return false;
-        // Last one wins and cancels the previous prerender.
-        cancelPrerender(null);
-        if (TextUtils.isEmpty(url)) return false;

[ahemery1, 2017/03/21 15:15:57]

Extracted because behavior is more general than just prerender. See comment line
336 of new version.

+
         boolean throttle = !shouldPrerenderOnCellularForSession(session);
         if (throttle && !mClientManager.isPrerenderingAllowed(uid)) return false;
 
@@ -947,18 +956,17 @@ public class CustomTabsConnection {
             referrer = getReferrerForSession(session).getUrl();
         }
         if (referrer == null) referrer = "";
-        Pair<WebContents, WebContents> webContentsPair = mExternalPrerenderHandler.addPrerender(
-                Profile.getLastUsedProfile(), url, referrer,
-                contentBounds,
-                shouldPrerenderOnCellularForSession(session));
+        Pair<WebContents, WebContents> webContentsPair =
+                mExternalPrerenderHandler.addPrerender(Profile.getLastUsedProfile(), url, referrer,
+                        contentBounds, shouldPrerenderOnCellularForSession(session));
         if (webContentsPair == null) return false;
         WebContents dummyWebContents = webContentsPair.first;
         if (webContentsPair.second != null) {
             mClientManager.resetPostMessageHandlerForSession(session, webContentsPair.second);
         }
         if (throttle) mClientManager.registerPrerenderRequest(uid, url);
-        mSpeculation = SpeculationParams.forPrerender(
-                session, url, dummyWebContents, referrer, extras);
+        mSpeculation =
+                SpeculationParams.forPrerender(session, url, dummyWebContents, referrer, extras);
 
         RecordHistogram.recordBooleanHistogram("CustomTabs.PrerenderSessionUsesDefaultParameters",
                 mClientManager.usesDefaultSessionParameters(session));