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..8c4923d477cdbf2c7170340d0bb237b435d952da 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);
+            return new SpeculationParams(session, url, PRERENDER, webcontents, referrer, extras);
         }
 
-        static SpeculationParams forPrefetch(CustomTabsSessionToken session, String url) {
-            return new SpeculationParams(session, url, null, null, null, true);
-        }
-
-        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,30 +321,34 @@ public class CustomTabsConnection {
             int uid, String url, Bundle extras, List<Bundle> otherLikelyBundles) {
         ThreadUtils.assertOnUiThread();
         if (TextUtils.isEmpty(url)) {
-            cancelPrerender(session);
+            cancelSpeculation(session);
             return;
         }
 
         WarmupManager warmupManager = WarmupManager.getInstance();
-        Profile profile = Profile.getLastUsedProfile();
 
         url = DataReductionProxySettings.getInstance().maybeRewriteWebliteUrl(url);
         int debugOverrideValue = NO_OVERRIDE;
         if (extras != null) debugOverrideValue = extras.getInt(DEBUG_OVERRIDE_KEY, NO_OVERRIDE);
 
-        boolean didStartPrerender = 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);
-            }
+        int baseSpeculationMode = getSpeculationModeForSession(session);
+        int temporarySpeculationMode =
+                getSpeculationModeFromDebugOverride(session, debugOverrideValue);
+        setSpeculationModeForSession(session, temporarySpeculationMode);

[Benoit L, 2017/03/27 13:18:15]

That's only a matter of preference, but I don't really like temporarily
modifying global state.
Since the modification is scopped, can you pass the value to the methods called
here?

Also, you could then do:

int mode = getSpeculationMode(session, overrideValue);

[ahemery1, 2017/03/27 14:47:33]

Done.

+
+        boolean didStartSpeculation = false;
+        if (maySpeculate(session)) {
+            didStartSpeculation = startSpeculation(session, url, extras, uid);
         }
         preconnectUrls(otherLikelyBundles);
-        if (!didStartPrefetch) warmupManager.maybePreconnectUrlAndSubResources(profile, url);
-        if (!didStartPrerender) warmupManager.createSpareWebContents();
+        if (temporarySpeculationMode != SpeculationParams.PREFETCH || !didStartSpeculation) {

[mattcary, 2017/03/27 13:11:35]

Shouldn't some of this logic go into startSpeculation?

I'm concerned that the logic handling is split up between two different places.
It makes it difficult to figure out what actually happens for a particular mode.

[ahemery1, 2017/03/27 14:47:33]

I hear you, however not sure about this one since it seems to me that the logic
is higher level. As in, done if the main speculation has failed or if we can
still do some other optimizations. I have added a comment to clarify the role of
this code. What's your opinion on that lizeb@ ?

[mattcary, 2017/03/27 15:10:13]

I was thinking of just pushing this all down into startSpeculation, which would
then read something like

switch (speculationMode) {
  case PREFETCH:
    launched = PrefetchUrl();
    preconnectUrls(...)
    if (!launched) createSpareWebContents()
    break;
   etc
}

There's maybe a couple of duplicated lines but the general flow is
comprehensible, and there's less coupling of state across functions (eg,
didStartSpeculation) and complicated logic (temporaryMode != PREFETCH ||
!didStart) which obscures the fairly straightforward idea of what we want to do
for each mode.

lizeb@ may have other opinions, of course.

[ahemery1, 2017/03/27 15:45:39]

Fair enough, I have modified it.

+            Profile profile = Profile.getLastUsedProfile();
+            warmupManager.maybePreconnectUrlAndSubResources(profile, url);
+        }
+        if (temporarySpeculationMode != SpeculationParams.PRERENDER || !didStartSpeculation) {
+            warmupManager.createSpareWebContents();
+        }
+        setSpeculationModeForSession(session, baseSpeculationMode);
     }
 
     /**
@@ -587,10 +597,8 @@ public class CustomTabsConnection {
             return null;
         }
 
-        if (mSpeculation.prefetchOnly) {
-            Profile profile = Profile.getLastUsedProfile();
-            new ResourcePrefetchPredictor(profile).stopPrefetching(mSpeculation.url);
-            mSpeculation = null;
+        if (mSpeculation.speculationMode == SpeculationParams.PREFETCH) {
+            cancelSpeculation(session);
             return null;
         }
 
@@ -607,7 +615,7 @@ public class CustomTabsConnection {
             result = webContents;
             mSpeculation = null;
         } else {
-            cancelPrerender(session);
+            cancelSpeculation(session);
         }
         if (!mClientManager.usesDefaultSessionParameters(session) && webContents != null) {
             RecordHistogram.recordBooleanHistogram(
@@ -672,6 +680,14 @@ public class CustomTabsConnection {
         mClientManager.setSendNavigationInfoForSession(session, send);
     }
 
+    public void setSpeculationModeForSession(CustomTabsSessionToken session, int speculationMode) {

[Benoit L, 2017/03/27 13:18:15]

nit: For consistency with the setters / getters above, make these methods
package private.

[ahemery1, 2017/03/27 14:47:33]

Done.

+        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.
@@ -887,7 +903,7 @@ public class CustomTabsConnection {
         });
     }
 
-    private boolean mayPrerender(CustomTabsSessionToken session) {
+    private boolean maySpeculate(CustomTabsSessionToken session) {
         if (!DeviceClassManager.enablePrerendering()) return false;
         // TODO(yusufo): The check for prerender in PrivacyManager now checks for the network
         // connection type as well, we should either change that or add another check for custom
@@ -900,17 +916,48 @@ public class CustomTabsConnection {
         return !cm.isActiveNetworkMetered() || shouldPrerenderOnCellularForSession(session);
     }
 
-    /** Cancels a prerender for a given session, or any session if null. */
-    void cancelPrerender(CustomTabsSessionToken session) {
+    /** Cancels the speculation for a given session, or any session if null. */
+    public void cancelSpeculation(CustomTabsSessionToken session) {

[Benoit L, 2017/03/27 13:18:15]

nit: Please keep this method package private.

         ThreadUtils.assertOnUiThread();
-        if (mSpeculation != null && (session == null || session.equals(mSpeculation.session))
-                && mSpeculation.webContents != null) {
-            mExternalPrerenderHandler.cancelCurrentPrerender();
-            mSpeculation.webContents.destroy();
+        if (mSpeculation == null) return;
+        if (session == null || session.equals(mSpeculation.session)) {
+            switch (mSpeculation.speculationMode) {
+                case SpeculationParams.PRERENDER:
+                    cancelPrerender(session);
+                    break;
+                case SpeculationParams.PREFETCH:
+                    cancelPrefetch(session);
+                    break;
+                default:
+                    return;
+            }
             mSpeculation = null;
         }
     }
 
+    private void cancelPrerender(CustomTabsSessionToken session) {

[Benoit L, 2017/03/27 13:18:14]

These two cancel*() methods are private, trivial and used only once.
Can you inline them above?

[ahemery1, 2017/03/27 14:47:33]

Done.

+        if (mSpeculation.webContents == null) return;
+        mExternalPrerenderHandler.cancelCurrentPrerender();
+        mSpeculation.webContents.destroy();
+    }
+
+    private void cancelPrefetch(CustomTabsSessionToken session) {
+        Profile profile = Profile.getLastUsedProfile();
+        new ResourcePrefetchPredictor(profile).stopPrefetching(mSpeculation.url);
+    }
+
+    @VisibleForTesting

[Benoit L, 2017/03/27 13:18:15]

This method doesn't seem to be called in tests, why is it @VisibleForTesting
then?
Also, if it's not used in test, please keep it private.

[ahemery1, 2017/03/27 14:47:33]

Done.

+    boolean startSpeculation(CustomTabsSessionToken session, String url, Bundle extras, int uid) {
+        switch (getSpeculationModeForSession(session)) {
+            case SpeculationParams.PRERENDER:
+                return prerenderUrl(session, url, extras, uid);
+            case SpeculationParams.PREFETCH:
+                return prefetchUrl(session, url);
+            default:
+                return false;
+        }
+    }
+
     /**
      * Tries to request a prerender for a given URL.
      *
@@ -923,12 +970,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;
+
         boolean throttle = !shouldPrerenderOnCellularForSession(session);
         if (throttle && !mClientManager.isPrerenderingAllowed(uid)) return false;
 
@@ -947,18 +990,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));
@@ -966,6 +1008,13 @@ public class CustomTabsConnection {
         return true;
     }
 
+    private boolean prefetchUrl(CustomTabsSessionToken session, String url) {
+        Profile profile = Profile.getLastUsedProfile();
+        boolean didStartPrefetching = new ResourcePrefetchPredictor(profile).startPrefetching(url);
+        if (didStartPrefetching) SpeculationParams.forPrefetch(session, url);
+        return didStartPrefetching;
+    }
+
     @VisibleForTesting
     void resetThrottling(Context context, int uid) {
         mClientManager.resetThrottling(uid);
@@ -980,4 +1029,16 @@ public class CustomTabsConnection {
     void setForcePrerender(boolean force) {
         mForcePrerenderForTesting = force;
     }
+
+    private int getSpeculationModeFromDebugOverride(
+            CustomTabsSessionToken session, int debugOverrideValue) {
+        switch (debugOverrideValue) {
+            case PREFETCH_ONLY:
+                return SpeculationParams.PREFETCH;
+            case NO_PRERENDERING:
+                return SpeculationParams.NO_SPECULATION;
+            default:
+                return getSpeculationModeForSession(session);
+        }
+    }
 }