Allow overlapping sync and async startup requests

content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java

Index: content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
diff --git a/content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java b/content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
index e6b3a6757a98298cd91f9da0681c4d76d8d0c527..394cb47b969e5f9ba4a6f0696a81826449d863e4 100644
--- a/content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
+++ b/content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
@@ -13,6 +13,8 @@ import com.google.common.annotations.VisibleForTesting;
 import org.chromium.base.CalledByNative;
 import org.chromium.base.JNINamespace;
 import org.chromium.base.ThreadUtils;
+import org.chromium.content.app.ContentMain;
+import org.chromium.content.app.LibraryLoader;
 import org.chromium.content.common.ProcessInitException;
 
 import java.util.ArrayList;
@@ -55,12 +57,13 @@ public class BrowserStartupController {
 
     private static boolean sBrowserMayStartAsynchronously = false;
 
-    private static void setAsynchronousStartupConfig() {
-        sBrowserMayStartAsynchronously = true;
+    private static void setAsynchronousStartupConfig(boolean enable) {
+        sBrowserMayStartAsynchronously = enable;
     }
 
+    @VisibleForTesting
     @CalledByNative
-    private static boolean browserMayStartAsynchonously() {
+    static boolean browserMayStartAsynchonously() {
         return sBrowserMayStartAsynchronously;
     }
 
@@ -79,13 +82,27 @@ public class BrowserStartupController {
     // The context is set on creation, but the reference is cleared after the browser process
     // initialization has been started, since it is not needed anymore. This is to ensure the
     // context is not leaked.
-    private Context mContext;
+    private final Context mContext;
 
     // Whether the async startup of the browser process has started.
     private boolean mHasStartedInitializingBrowserProcess;
 
     // Whether the async startup of the browser process is complete.
-    private boolean mAsyncStartupDone;
+    private boolean mStartupDone;
+
+    // Use single-process mode that runs the renderer on a separate thread in
+    // the main application.
+    public static final int MAX_RENDERERS_SINGLE_PROCESS = 0;
+
+    // Cap on the maximum number of renderer processes that can be requested.
+    // This is currently set to account for:
+    //  13: The maximum number of sandboxed processes we have available
+    // - 1: The regular New Tab Page
+    // - 1: The incognito New Tab Page
+    // - 1: A regular incognito tab
+    // - 1: Safety buffer (http://crbug.com/251279)
+    public static final int MAX_RENDERERS_LIMIT =
+            ChildProcessLauncher.MAX_REGISTERED_SANDBOXED_SERVICES - 4;
 
     // This field is set after startup has been completed based on whether the startup was a success
     // or not. It is used when later requests to startup come in that happen after the initial set
@@ -124,7 +141,7 @@ public class BrowserStartupController {
      */
     public void startBrowserProcessesAsync(final StartupCallback callback) {
         assert ThreadUtils.runningOnUiThread() : "Tried to start the browser on the wrong thread.";
-        if (mAsyncStartupDone) {
+        if (mStartupDone) {
             // Browser process initialization has already been completed, so we can immediately post
             // the callback.
             postStartupCompleted(callback);
@@ -139,42 +156,65 @@ public class BrowserStartupController {
             // flag that indicates that we have kicked off starting the browser process.
             mHasStartedInitializingBrowserProcess = true;
 
-            enableAsynchronousStartup();
-
             // Try to initialize the Android browser process.
-            tryToInitializeBrowserProcess();
+            try {

[nyquist, 2013/08/23 06:28:32]

Extract this to a small method, to make it easier to follow the high level flow
(the try and catch). This should be reused in the sync startup below.

For example:
if (!tryPrepareToStartBrowserProcess(MAX_RENDERERS_LIMIT)) return;

The method could look like:
private boolean tryPrepareToStartBrowserProcess(int maxRenderers) {
    try {
        prepareToStartBrowserProcess(maxRenderers);
        return true;
    } catch (ProcessInitException e) {
        Log.e(TAG, "Unable to load native library.", e);
        enqueueCallbackExecution(STARTUP_FAILURE, NOT_ALREADY_STARTED);
        return false;
    }
}

[aberent, 2013/08/23 11:40:21]

Done.

+                prepareToStartBrowserProcess(MAX_RENDERERS_LIMIT);
+            } catch (ProcessInitException e) {
+                Log.e(TAG, "Unable to load native library.", e);
+                enqueueCallbackExecution(STARTUP_FAILURE, NOT_ALREADY_STARTED);
+                return;
+            }
+
+            enableAsynchronousStartup();
+            contentStart();
         }
     }
 
-    private void tryToInitializeBrowserProcess() {
-        try {
-            assert mContext != null;
-            boolean wasAlreadyInitialized = initializeAndroidBrowserProcess();
-            // The context is not needed anymore, so clear the member field to not leak.
-            mContext = null;
-            if (wasAlreadyInitialized) {
-                // Something has already initialized the browser process before we got to setup the
-                // async startup. This means that we will never get a callback, so manually call
-                // them now, and just assume that the startup was successful.
-                Log.w(TAG, "Browser process was initialized without BrowserStartupController");
-                enqueueCallbackExecution(STARTUP_SUCCESS, ALREADY_STARTED);
+    /**
+     * Start the browser process synchronously. If the browser is already being started
+     * asynchronously then complete startup synchronously
+     *
+     * <p/>
+     * Note that this can only be called on the UI thread.
+     *
+     * @param max_renderers The maximum number of renderer processes the browser may
+     *                      create. Zero for single process mode.
+     * @return true if successfully started, false otherwise.
+     */
+    public boolean startBrowserProcessesSync(int max_renderers) {

[Yaron, 2013/08/22 06:00:26]

maxRenderers

[aberent, 2013/08/23 11:40:21]

Done.

+        if (mStartupDone) {
+            // Nothing to do
+            return mStartupSuccess;
+        }
+        if (!mHasStartedInitializingBrowserProcess) {

[nyquist, 2013/08/23 06:28:32]

Something like:
if (!mHasStartedInitializingBrowserProcess &&
!tryPrepareToStartBrowserProcess(maxRenderers))
  return false;

[aberent, 2013/08/23 11:40:21]

Done.

+            try {
+                    prepareToStartBrowserProcess(max_renderers);

[Yaron, 2013/08/22 06:00:26]

nit:indent

[aberent, 2013/08/23 11:40:21]

N/A, gone away.

+            } catch (ProcessInitException e) {
+                Log.e(TAG, "Unable to load native library.", e);
+                enqueueCallbackExecution(STARTUP_FAILURE, NOT_ALREADY_STARTED);
+                return false;
             }
-        } catch (ProcessInitException e) {
-            Log.e(TAG, "Unable to start browser process.", e);
-            // ProcessInitException could mean one of two things:
-            // 1) The LibraryLoader failed.
-            // 2) ContentMain failed to start.
-            // It is unclear whether the browser tasks have already been started, and in case they
-            // have not, post a message to execute all the callbacks. Whichever call to
-            // executeEnqueuedCallbacks comes first will trigger the callbacks, but since the list
-            // of callbacks is then cleared, they will only be called once.
-            enqueueCallbackExecution(STARTUP_FAILURE, NOT_ALREADY_STARTED);
         }
+
+        disableAsynchronousStartup();
+        contentStart();
+
+        // Startup should now be complete
+        assert(mStartupDone);

[nyquist, 2013/08/23 06:28:32]

Nit: I think the style is to usually drop ( and )

[aberent, 2013/08/23 11:40:21]

Done.

+        return mStartupSuccess;
+    }
+
+    /**
+     * Wrap ContentMain.start() for testing.
+     */
+    @VisibleForTesting
+    void contentStart() {
+        ContentMain.start();
     }
 
     public void addStartupCompletedObserver(StartupCallback callback) {
         ThreadUtils.assertOnUiThread();
-        if (mAsyncStartupDone)
+        if (mStartupDone)
             postStartupCompleted(callback);
         else
             mAsyncStartupCallbacks.add(callback);
@@ -182,13 +222,13 @@ public class BrowserStartupController {
 
     private void executeEnqueuedCallbacks(int startupResult, boolean alreadyStarted) {
         assert ThreadUtils.runningOnUiThread() : "Callback from browser startup from wrong thread.";
-        mAsyncStartupDone = true;
+        mStartupDone = true;
+        mStartupSuccess = (startupResult <= 0);
         for (StartupCallback asyncStartupCallback : mAsyncStartupCallbacks) {
-            if (startupResult > 0) {
-                asyncStartupCallback.onFailure();
-            } else {
-                mStartupSuccess = true;
+            if (mStartupSuccess) {

[nyquist, 2013/08/23 06:28:32]

Nit: Skip curly braces

[aberent, 2013/08/23 11:40:21]

The Java style guide (unlike the C++ style guide) requires curly braces here.
See http://source.android.com/source/code-style.html which says 

"We require braces around the statements for a conditional. Except, if the
entire conditional (the condition and the body) fit on one line, you may (but
are not obligated to) put it all on one line."

I have changed the other instances where they were missing.

                 asyncStartupCallback.onSuccess(alreadyStarted);
+            } else {
+                asyncStartupCallback.onFailure();
             }
         }
         // We don't want to hold on to any objects after we do not need them anymore.
@@ -222,14 +262,67 @@ public class BrowserStartupController {
      */
     @VisibleForTesting
     void enableAsynchronousStartup() {
-        setAsynchronousStartupConfig();
+        setAsynchronousStartupConfig(true);
+    }
+
+    void disableAsynchronousStartup() {

[nyquist, 2013/08/23 06:28:32]

private

[aberent, 2013/08/23 11:40:21]

Since this, and enableAsynchronousStartup() are no longer used by the tests, and
are one line functions, I don't think having them adds anything to readability.
I have removed them and replaced them with direct calls to
setAsynchronousStartupConfig (which I have renamed setAsynchronousStartup). 

+        setAsynchronousStartupConfig(false);
     }
 
     /**
-     * @return whether the process was already initialized, so native was not instructed to start.
+     * @param maxRendererProcesses
+     * @throws ProcessInitException
      */
     @VisibleForTesting
-    boolean initializeAndroidBrowserProcess() throws ProcessInitException {
-        return !AndroidBrowserProcess.init(mContext, AndroidBrowserProcess.MAX_RENDERERS_LIMIT);
+    void prepareToStartBrowserProcess(int maxRendererProcesses)
+            throws ProcessInitException {
+        Log.i(TAG, "Initializing chromium process, renderers=" + maxRendererProcesses);
+
+        // Normally Main.java will have kicked this off asynchronously for Chrome. But other
+        // ContentView apps like tests also need them so we make sure we've extracted resources
+        // here. We can still make it a little async (wait until the library is loaded).
+        ResourceExtractor resourceExtractor = ResourceExtractor.get(mContext);
+        resourceExtractor.startExtractingResources();
+
+        // Normally Main.java will have already loaded the library asynchronously, we only need
+        // to load it here if we arrived via another flow, e.g. bookmark access & sync setup.
+        LibraryLoader.ensureInitialized();
+
+        // TODO(yfriedman): Remove dependency on a command line flag for this.
+        DeviceUtils.addDeviceSpecificUserAgentSwitch(mContext);
+
+        Context appContext = mContext.getApplicationContext();
+        // Now we really need to have the resources ready.
+        resourceExtractor.waitForCompletion();
+
+        nativeSetCommandLineFlags(maxRendererProcesses,
+                nativeIsPluginEnabled() ? getPlugins(mContext) : null);
+        ContentMain.initApplicationContext(appContext);
     }
+
+    /**
+     * Initialization needed for tests. Mainly used by content browsertests.
+     */
+    public void initChromiumBrowserProcessForTests() {
+        ResourceExtractor resourceExtractor = ResourceExtractor.get(mContext);
+        resourceExtractor.startExtractingResources();
+        resourceExtractor.waitForCompletion();
+
+        // Having a single renderer should be sufficient for tests. We can't have more than
+        // MAX_RENDERERS_LIMIT.
+        nativeSetCommandLineFlags(1 /* maxRenderers */, null);
+    }
+
+    private static String getPlugins(final Context context) {

[nyquist, 2013/08/23 06:28:32]

Remove static and use mContext, since it is now final.

[aberent, 2013/08/23 11:40:21]

Done.

+        return PepperPluginManager.getPlugins(context);
+    }
+
+    private static native void nativeSetCommandLineFlags(int maxRenderProcesses,
+            String pluginDescriptor);
+
+    // Is this an official build of Chrome? Only native code knows for sure. Official build
+    // knowledge is needed very early in process startup.
+    private static native boolean nativeIsOfficialBuild();
+
+    private static native boolean nativeIsPluginEnabled();
 }