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..aae445d381b44f04474445608947ee3ae2027bfd 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
@@ -94,33 +94,39 @@ public class CustomTabsConnection {
     /** Holds the parameters for the current speculation. */
     @VisibleForTesting
     static final class SpeculationParams {
+        @VisibleForTesting
+        static final int NO_SPECULATION = 0;
+        @VisibleForTesting
+        static final int PREFETCH = 1;
+        @VisibleForTesting
+        static final int PRERENDER = 2;
+
         public final CustomTabsSessionToken session;
         public final String url;
+        public final int speculationMode;
 
         // 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) {
+            return new SpeculationParams(session, url, PREFETCH, 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, PRERENDER, webcontents, referrer, extras);
         }
 
-        private SpeculationParams(CustomTabsSessionToken session, String url,
-                WebContents webContents, String referrer, Bundle extras, boolean prefetchOnly) {
+        private SpeculationParams(CustomTabsSessionToken session, String url, int speculationMode,
+                WebContents webContents, String referrer, Bundle extras) {
             this.session = session;
             this.url = url;
+            this.speculationMode = speculationMode;
             this.webContents = webContents;
             this.referrer = referrer;
             this.extras = extras;
-            this.prefetchOnly = prefetchOnly;
         }
     }
 
@@ -189,7 +195,7 @@ public class CustomTabsConnection {
         ClientManager.DisconnectCallback onDisconnect = new ClientManager.DisconnectCallback() {
             @Override
             public void run(CustomTabsSessionToken session) {
-                cancelPrerender(session);
+                cancelSpeculation(session);
             }
         };
         PostMessageHandler handler = new PostMessageHandler(session);
@@ -315,7 +321,7 @@ public class CustomTabsConnection {
             int uid, String url, Bundle extras, List<Bundle> otherLikelyBundles) {
         ThreadUtils.assertOnUiThread();
         if (TextUtils.isEmpty(url)) {
-            cancelPrerender(session);
+            cancelSpeculation(session);
             return;
         }
 
@@ -326,19 +332,19 @@ public class CustomTabsConnection {
         int debugOverrideValue = NO_OVERRIDE;
         if (extras != null) debugOverrideValue = extras.getInt(DEBUG_OVERRIDE_KEY, NO_OVERRIDE);
 
-        boolean didStartPrerender = false, didStartPrefetch = false;
+        boolean didStartSpeculation = false, didStartPrefetch = false;
         boolean mayPrerender = mayPrerender(session);
         if (mayPrerender) {
             if (debugOverrideValue == PREFETCH_ONLY) {
                 didStartPrefetch = new ResourcePrefetchPredictor(profile).startPrefetching(url);
                 if (didStartPrefetch) mSpeculation = SpeculationParams.forPrefetch(session, url);
             } else if (debugOverrideValue != NO_PRERENDERING) {
-                didStartPrerender = prerenderUrl(session, url, extras, uid);
+                didStartSpeculation = startSpeculation(session, url, extras, uid);
             }
         }
         preconnectUrls(otherLikelyBundles);
         if (!didStartPrefetch) warmupManager.maybePreconnectUrlAndSubResources(profile, url);
-        if (!didStartPrerender) warmupManager.createSpareWebContents();
+        if (!didStartSpeculation) warmupManager.createSpareWebContents();
     }
 
     /**
@@ -587,7 +593,7 @@ public class CustomTabsConnection {
             return null;
         }
 
-        if (mSpeculation.prefetchOnly) {
+        if (mSpeculation.speculationMode == SpeculationParams.PREFETCH) {

[Benoit L, 2017/03/23 18:28:29]

Basically the speculation is stopped in two different places depending on the
current mode. Isn't it what cancelSpeculation should take care of?

[ahemery1, 2017/03/27 12:27:31]

You're absolutely right, missed it.

             Profile profile = Profile.getLastUsedProfile();
             new ResourcePrefetchPredictor(profile).stopPrefetching(mSpeculation.url);
             mSpeculation = null;
@@ -607,7 +613,7 @@ public class CustomTabsConnection {
             result = webContents;
             mSpeculation = null;
         } else {
-            cancelPrerender(session);
+            cancelSpeculation(session);
         }
         if (!mClientManager.usesDefaultSessionParameters(session) && webContents != null) {
             RecordHistogram.recordBooleanHistogram(
@@ -672,6 +678,15 @@ public class CustomTabsConnection {
         mClientManager.setSendNavigationInfoForSession(session, send);
     }
 
+    @VisibleForTesting

[Benoit L, 2017/03/23 18:28:29]

This is not strictly true that this is VisibleForTesting. I think that these
days VisibleForTesting a no-op, so can you remove it?

[ahemery1, 2017/03/27 12:27:32]

Changed to public.

+    void setSpeculationModeForSession(CustomTabsSessionToken session, int speculationMode) {
+        mClientManager.setSpeculationModeForSession(session, speculationMode);
+    }
+
+    public int getSpeculationModeForSession(CustomTabsSessionToken session) {
+        return mClientManager.getSpeculationModeForSession(session);
+    }
+
     /**
      * Extracts the creator package name from the intent.
      * @param intent The intent to get the package name from.
@@ -901,13 +916,34 @@ public class CustomTabsConnection {
     }
 
     /** Cancels a prerender for a given session, or any session if null. */
-    void cancelPrerender(CustomTabsSessionToken session) {
+    @VisibleForTesting
+    void cancelSpeculation(CustomTabsSessionToken session) {

[Benoit L, 2017/03/23 18:28:28]

nit: Please keep the assertOnUiThread() here.

[ahemery1, 2017/03/27 12:27:31]

Moved from subfunction to here.

+        if (mSpeculation == null) return;

[Benoit L, 2017/03/23 18:28:29]

nit: Why breaking up the condition here?

[ahemery1, 2017/03/27 12:27:31]

Felt like it had two different functions. First line checking that the cancel is
not redundant, and second line checking that the target is correct.
Wouldn't mind changing it however as it took me 5 minutes to write this
explanation.

+        if (session != null && !session.equals(mSpeculation.session)) return;

[Benoit L, 2017/03/23 18:28:29]

tiny nit: This reads a bit weird, what about:

if (session == null || !session.equals(...

?

[ahemery1, 2017/03/27 12:27:31]

Done.

+        switch (mSpeculation.speculationMode) {
+            case SpeculationParams.PRERENDER:
+                cancelPrerender(session);
+                break;
+            default:
+                return;
+        }
+    }
+
+    private 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.webContents == null) return;
+        mExternalPrerenderHandler.cancelCurrentPrerender();
+        mSpeculation.webContents.destroy();
+        mSpeculation = null;

[Benoit L, 2017/03/23 18:28:29]

All current paths will set mSpeculation to null, can you move this to
cancelSpeculation()?

[ahemery1, 2017/03/27 12:27:31]

Done.

+    }
+
+    @VisibleForTesting
+    boolean startSpeculation(CustomTabsSessionToken session, String url, Bundle extras, int uid) {
+        switch (getSpeculationModeForSession(session)) {

[Benoit L, 2017/03/23 18:28:29]

ditto, why isn't prefetch handled here as well?

[ahemery1, 2017/03/27 12:27:31]

I included prefetch in the same process, but doing so I had to move stuff around
a bit in highConfidenceMayLaunchUrl. Let me know if you think it is worth it.

+            case SpeculationParams.PRERENDER:
+                return prerenderUrl(session, url, extras, uid);
+            default:
+                return false;
         }
     }
 
@@ -923,12 +959,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;
         if (!mWarmupHasBeenCalled.get()) return false;
-        // Last one wins and cancels the previous prerender.
-        cancelPrerender(null);
-        if (TextUtils.isEmpty(url)) return false;

[Benoit L, 2017/03/23 18:28:28]

Was this check redundant?

[ahemery1, 2017/03/27 12:27:31]

It was indeed redundant. Already checked in highConfidenceMayLaunchUrl() line
317-320

+
         boolean throttle = !shouldPrerenderOnCellularForSession(session);
         if (throttle && !mClientManager.isPrerenderingAllowed(uid)) return false;
 
@@ -947,18 +979,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));