CustomTabs: Base version for prerender switch

chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.chrome.browser.customtabs;
 
 import android.app.ActivityManager;
 import android.app.Application;
 import android.app.PendingIntent;
 import android.content.Context;
@@ skipping 76 matching lines @@
     @VisibleForTesting
     static final int NO_PRERENDERING = 1;
     @VisibleForTesting
     static final int PREFETCH_ONLY = 2;
 
     private static AtomicReference<CustomTabsConnection> sInstance = new AtomicReference<>();
 
     /** 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;
         }
     }
 
     @VisibleForTesting
     SpeculationParams mSpeculation;
     protected final Application mApplication;
     protected final ClientManager mClientManager;
     private final boolean mLogRequests;
     private final AtomicBoolean mWarmupHasBeenCalled = new AtomicBoolean();
     private final AtomicBoolean mWarmupHasBeenFinished = new AtomicBoolean();
@@ skipping 48 matching lines @@
         if (mForcePrerenderForTesting) mClientManager.setPrerenderCellularForSession(session, true);
         logCall("newSession()", success);
         return success;
     }
 
     private boolean newSessionInternal(CustomTabsSessionToken session) {
         if (session == null) return false;
         ClientManager.DisconnectCallback onDisconnect = new ClientManager.DisconnectCallback() {
             @Override
             public void run(CustomTabsSessionToken session) {
-                cancelPrerender(session);
+                cancelSpeculation(session);
             }
         };
         PostMessageHandler handler = new PostMessageHandler(session);
         return mClientManager.newSession(session, Binder.getCallingUid(), onDisconnect, handler);
     }
 
     /** Warmup activities that should only happen once. */
     @SuppressFBWarnings("DM_EXIT")
     private static void initializeBrowser(final Application app) {
         ThreadUtils.assertOnUiThread();
@@ skipping 101 matching lines @@
         if (!allowedScheme) return null;
         return uri.toString();
     }
 
     /**
      * High confidence mayLaunchUrl() call, that is:
      * - Tries to prerender if possible.
      * - An empty URL cancels the current prerender if any.
      * - If prerendering is not possible, makes sure that there is a spare renderer.
      */
     private void highConfidenceMayLaunchUrl(CustomTabsSessionToken session,

[ahemery1, 2017/03/27 12:30:01]

Main rework for this patch is here, integrating prefetch into the now common
workflow

             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);
     }
 
     /**
      * Low confidence mayLaunchUrl() call, that is:
      * - Preconnects to the ordered list of URLs.
      * - Makes sure that there is a spare renderer.
      */
     @VisibleForTesting
     boolean lowConfidenceMayLaunchUrl(List<Bundle> likelyBundles) {
         ThreadUtils.assertOnUiThread();
@@ skipping 228 matching lines @@
      * @param url The URL the WebContents is for.
      * @param referrer The referrer to use for |url|.
      * @return The prerendered WebContents, or null.
      */
     WebContents takePrerenderedUrl(CustomTabsSessionToken session, String url, String referrer) {
         ThreadUtils.assertOnUiThread();
         if (mSpeculation == null || session == null || !session.equals(mSpeculation.session)) {
             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;
         }
 
         WebContents webContents = mSpeculation.webContents;
         String prerenderedUrl = mSpeculation.url;
         String prerenderReferrer = mSpeculation.referrer;
         if (referrer == null) referrer = "";
         boolean ignoreFragments = mClientManager.getIgnoreFragmentsForSession(session);
         boolean urlsMatch = TextUtils.equals(prerenderedUrl, url)
                 || (ignoreFragments
                         && UrlUtilities.urlsMatchIgnoringFragments(prerenderedUrl, url));
         WebContents result = null;
         if (urlsMatch && TextUtils.equals(prerenderReferrer, referrer)) {
             result = webContents;
             mSpeculation = null;
         } else {
-            cancelPrerender(session);
+            cancelSpeculation(session);
         }
         if (!mClientManager.usesDefaultSessionParameters(session) && webContents != null) {
             RecordHistogram.recordBooleanHistogram(
                     "CustomTabs.NonDefaultSessionPrerenderMatched", result != null);
         }
 
         return result;
     }
 
     /** Returns the URL prerendered for a session, or null. */
@@ skipping 44 matching lines @@
         mClientManager.setPrerenderCellularForSession(session, value);
     }
 
     /**
      * See {@link ClientManager#setSendNavigationInfoForSession(CustomTabsSessionToken, boolean)}.
      */
     void setSendNavigationInfoForSession(CustomTabsSessionToken session, boolean send) {
         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.
      * @return the package name which can be null.
      */
     String extractCreatorPackage(Intent intent) {
         return null;
     }
 
     /**
@@ skipping 195 matching lines @@
     @VisibleForTesting
     void cleanUpSession(final CustomTabsSessionToken session) {
         ThreadUtils.runOnUiThread(new Runnable() {
             @Override
             public void run() {
                 mClientManager.cleanupSession(session);
             }
         });
     }
 
-    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
         // tabs. Then PrivacyManager should be used to make the below check.
         if (!PrefServiceBridge.getInstance().getNetworkPredictionEnabled()) return false;
         if (DataReductionProxySettings.getInstance().isDataReductionProxyEnabled()) return false;
         ConnectivityManager cm =
                 (ConnectivityManager) mApplication.getApplicationContext().getSystemService(
                         Context.CONNECTIVITY_SERVICE);
         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.
      *
      * @param session Session the request comes from.
      * @param url URL to prerender.
      * @param extras extra parameters.
      * @param uid UID of the caller.
      * @return true if a prerender has been initiated.
      */
     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;
 
         // A prerender will be requested. Time to destroy the spare WebContents.
         WarmupManager.getInstance().destroySpareWebContents();
 
         Intent extrasIntent = new Intent();
         if (extras != null) extrasIntent.putExtras(extras);
         if (IntentHandler.getExtraHeadersFromIntent(extrasIntent) != null) return false;
         if (mExternalPrerenderHandler == null) {
             mExternalPrerenderHandler = new ExternalPrerenderHandler();
         }
         Rect contentBounds = ExternalPrerenderHandler.estimateContentSize(mApplication, true);
         String referrer = IntentHandler.getReferrerUrlIncludingExtraHeaders(extrasIntent);
         if (referrer == null && getReferrerForSession(session) != null) {
             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));
 
         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);
     }
 
     @VisibleForTesting
     void ban(Context context, int uid) {
         mClientManager.ban(uid);
     }
 
     @VisibleForTesting
     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);
+        }
+    }
 }