Allow overlapping sync and async startup requests

Allow overlapping sync and async startup requests

On Android we can get a second request to start the browser while
the an asynchronous request is in progress. Since the second
request may be synchronous, we may have switch to completing
initialization synchronously. This patch handles this by tracking
which initialization tasks have been run, and running the remaining
initialization tasks.

BUG=260574
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=219795

[aberent, 2013-08-08 18:27:20]

This fixes a problem with my previous CL
(https://codereview.chromium.org/22691002/) which is causing a number of Android
downstream tests to crash or fail. Please review today if possible, since I am
on vacation next week, and would like to get this in for the M30 branch.

[joth, 2013-08-08 18:44:38]

aw/ and content/ lgtm assuming it's safe to switch from
BrowserStartupConfig.setAsync() mode to setSync() mode part way through startup?
(I didn't read any other code to convince myself of that)

https://codereview.chromium.org/22691002/diff/2001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java
(right):

https://codereview.chromium.org/22691002/diff/2001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java:99:
BrowserStartupConfig.setSync();
as BrowserStartupConfig is now driven from here, sounds like that class should
now be package protected rather than public?
BrowserStartupConfig.StartupCallback is the one snag, but may just mean the
interface needs duplicating on here. (Or use ValueCallback<Int> for the internal
one or something.

https://codereview.chromium.org/22691002/diff/2001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java:110:
public static boolean init(Context context, int maxRendererProcesses)
would make calling code easier to read if you had 2 public methods:

initSync(context, maxProcess) & initAsync(context, maxProcess, callback)

(they can share the internal private method taking a bool or whatever)

[jam, 2013-08-08 20:54:12]

This seems a bit complicated. Why can't we, when "starting" for the second time,
just get the existing StartupTaskRunner object and run all the tasks on it?

[aberent, 2013-08-08 21:29:20]

On 2013/08/08 20:54:12, jam wrote:
> This seems a bit complicated. Why can't we, when "starting" for the second
time,
> just get the existing StartupTaskRunner object and run all the tasks on it?

Possible, but means that we have change how the lifetime of a StartupTaskRunner
is managed. At the moment the StartupTaskRunner is created using as a refcounted
object (as seems be encouraged, although not required, for objects used by Bind;
see the documentation in callback.h) and as such deletes itself when the last
task has finished. BrowserMainLoop can, in effect, do a fire and forget on
StartupTaskRunner. To re-use it we would either have the StartupTaskRunner owned
by BrowserMainLoop, or keep a weak pointer to it in BrowserMainLoop.

I am not totally against the first option (making owned by BrowserMainLoop), and
will make this change experimentally to see what it looks like, but I don't
promise to go this way.

[aberent, 2013-08-19 16:05:30]

On 2013/08/08 21:29:20, aberent wrote:
> On 2013/08/08 20:54:12, jam wrote:
> > This seems a bit complicated. Why can't we, when "starting" for the second
> time,
> > just get the existing StartupTaskRunner object and run all the tasks on it?
> 
> Possible, but means that we have change how the lifetime of a
StartupTaskRunner
> is managed. At the moment the StartupTaskRunner is created using as a
refcounted
> object (as seems be encouraged, although not required, for objects used by
Bind;
> see the documentation in callback.h) and as such deletes itself when the last
> task has finished. BrowserMainLoop can, in effect, do a fire and forget on
> StartupTaskRunner. To re-use it we would either have the StartupTaskRunner
owned
> by BrowserMainLoop, or keep a weak pointer to it in BrowserMainLoop.
> 
> I am not totally against the first option (making owned by BrowserMainLoop),
and
> will make this change experimentally to see what it looks like, but I don't
> promise to go this way.

The new version I have landed is for information, since Tommy Nyquist is looking
at some things in the same area. I am still tidying this up and testing it.

Joth - I am aware that there are two comments in patch set 2 that I haven't yet
replied to. I still have to look at these.

[Yaron, 2013-08-19 16:56:22]

On 2013/08/19 16:06:27, aberent wrote:

aberent: is this ready for review?

[aberent, 2013-08-19 17:07:25]

On 2013/08/19 16:56:22, Yaron wrote:
> On 2013/08/19 16:06:27, aberent wrote:
> 
> aberent: is this ready for review?

Review comments welcome, but I am still testing, and want to take another look
at some bits myself. Also nyquist@ is probably landing an overlapping patch
first (https://codereview.chromium.org/22272006/) which will require some
changes to this patch.

[aberent, 2013-08-19 20:05:57]

On 2013/08/19 17:07:25, aberent wrote:
> On 2013/08/19 16:56:22, Yaron wrote:
> > On 2013/08/19 16:06:27, aberent wrote:
> > 
> > aberent: is this ready for review?
> 
> Review comments welcome, but I am still testing, and want to take another look
> at some bits myself. Also nyquist@ is probably landing an overlapping patch
> first (https://codereview.chromium.org/22272006/) which will require some
> changes to this patch.

Now ready for review.

[Yaron, 2013-08-20 06:08:52]

At a high-level looks fine. It's a shame to have to keep both this and the
multiple async startups flow from Tommy's patch but I'm not sure how to avoid
it. It seems like because of ChromeBrowserProvider, we can't fully use the async
flow.

https://codereview.chromium.org/22691002/diff/31001/chrome/android/testshell/...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java
(right):

https://codereview.chromium.org/22691002/diff/31001/chrome/android/testshell/...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:68:
if (!AndroidBrowserProcess.init(this, AndroidBrowserProcess.MAX_RENDERERS_LIMIT,
true,
Revert file. Covered by Tommy's patch

https://codereview.chromium.org/22691002/diff/31001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupConfig.java
(right):

https://codereview.chromium.org/22691002/diff/31001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupConfig.java:21:
private static LinkedList<StartupCallback> sCallbackList = null;
Ya, this should go away after rebasing on Tommy's patch

https://codereview.chromium.org/22691002/diff/31001/content/shell/android/she...
File
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java
(right):

https://codereview.chromium.org/22691002/diff/31001/content/shell/android/she...
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java:96:
if (!AndroidBrowserProcess.init(this, AndroidBrowserProcess.MAX_RENDERERS_LIMIT,
Revert file

[aberent, 2013-08-20 13:32:17]

https://codereview.chromium.org/22691002/diff/31001/chrome/android/testshell/...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java
(right):

https://codereview.chromium.org/22691002/diff/31001/chrome/android/testshell/...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:68:
if (!AndroidBrowserProcess.init(this, AndroidBrowserProcess.MAX_RENDERERS_LIMIT,
true,
On 2013/08/20 06:08:52, Yaron wrote:
> Revert file. Covered by Tommy's patch

I don't think it is. AndroidBrowserProcess.init() can be called multiple times;
and in particular will be called twice if Chrome gets an async startup request
followed by synch startup request. To handle this correctly init() has to know
whether C++ startup is complete. To do this it wraps the callback for async
requests in a callback that tells AndroidBrowserProcess that C++ startup is
complete. To make this possible I switched to passing the callback down to
init(), and setting up the callback within init().

To remove this I think we would need to completely merge
AndroidBrowserProcess.init() into Tommy's BrowserStartupController, which is
possible, and probably the right thing to do, but is more difficult to stage
across the Chromium/Clank interface (since init() is called directly from
Clank).

https://codereview.chromium.org/22691002/diff/31001/content/shell/android/she...
File
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java
(right):

https://codereview.chromium.org/22691002/diff/31001/content/shell/android/she...
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java:96:
if (!AndroidBrowserProcess.init(this, AndroidBrowserProcess.MAX_RENDERERS_LIMIT,
On 2013/08/20 06:08:52, Yaron wrote:
> Revert file

See my reply to your comment on ChromiumTestShellActivity.java

[aberent, 2013-08-21 21:15:06]

On 2013/08/20 13:32:17, aberent wrote:
>
https://codereview.chromium.org/22691002/diff/31001/chrome/android/testshell/...
> File
>
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java
> (right):
> 
>
https://codereview.chromium.org/22691002/diff/31001/chrome/android/testshell/...
>
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:68:
> if (!AndroidBrowserProcess.init(this,
AndroidBrowserProcess.MAX_RENDERERS_LIMIT,
> true,
> On 2013/08/20 06:08:52, Yaron wrote:
> > Revert file. Covered by Tommy's patch
> 
> I don't think it is. AndroidBrowserProcess.init() can be called multiple
times;
> and in particular will be called twice if Chrome gets an async startup request
> followed by synch startup request. To handle this correctly init() has to know
> whether C++ startup is complete. To do this it wraps the callback for async
> requests in a callback that tells AndroidBrowserProcess that C++ startup is
> complete. To make this possible I switched to passing the callback down to
> init(), and setting up the callback within init().
> 
> To remove this I think we would need to completely merge
> AndroidBrowserProcess.init() into Tommy's BrowserStartupController, which is
> possible, and probably the right thing to do, but is more difficult to stage
> across the Chromium/Clank interface (since init() is called directly from
> Clank).
> 
>
https://codereview.chromium.org/22691002/diff/31001/content/shell/android/she...
> File
>
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java
> (right):
> 
>
https://codereview.chromium.org/22691002/diff/31001/content/shell/android/she...
>
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java:96:
> if (!AndroidBrowserProcess.init(this,
AndroidBrowserProcess.MAX_RENDERERS_LIMIT,
> On 2013/08/20 06:08:52, Yaron wrote:
> > Revert file
> 
> See my reply to your comment on ChromiumTestShellActivity.java

New version uploaded, merges with Tommy's CL. I have merged
AndroidBrowserProcess into BrowserStartupController, leaving only a stub that I
will remove once the Clank side CL has landed.

Please take another look, since there are significant changes on the Java side
(and some fixes on the C++ side).

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

mostly lg. just one question and Tommy should review BrowserSTartupController
and test chanegs

https://codereview.chromium.org/22691002/diff/40001/content/app/android/conte...
File content/app/android/content_main.cc (right):

https://codereview.chromium.org/22691002/diff/40001/content/app/android/conte...
content/app/android/content_main.cc:38: static void Start(JNIEnv* env, jclass
clazz) {
Doesn't this hide potential errors? I thought we had to add this support,
possibly Michael Bai did, to report errors while doing some early
initialization.

https://codereview.chromium.org/22691002/diff/40001/content/browser/browser_m...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/22691002/diff/40001/content/browser/browser_m...
content/browser/browser_main_loop.cc:901: // Alert the clipboard class to which
threads are allowed to access the
Nit: indentation

https://codereview.chromium.org/22691002/diff/40001/content/browser/browser_m...
File content/browser/browser_main_runner.cc (right):

https://codereview.chromium.org/22691002/diff/40001/content/browser/browser_m...
content/browser/browser_main_runner.cc:156: // True if the runner has been
initialized.
Nit: update

https://codereview.chromium.org/22691002/diff/40001/content/browser/startup_t...
File content/browser/startup_task_runner.cc (right):

https://codereview.chromium.org/22691002/diff/40001/content/browser/startup_t...
content/browser/startup_task_runner.cc:18: // This is needed to satisfy the
coding style checks done by the Mac compiler,
Nit: comment is unnecessary. It's standard style for chromium

https://codereview.chromium.org/22691002/diff/40001/content/browser/startup_t...
File content/browser/startup_task_runner.h (right):

https://codereview.chromium.org/22691002/diff/40001/content/browser/startup_t...
content/browser/startup_task_runner.h:11: #include "base/memory/ref_counted.h"
Not needed anymore

https://codereview.chromium.org/22691002/diff/40001/content/browser/startup_t...
File content/browser/startup_task_runner_unittest.cc (right):

https://codereview.chromium.org/22691002/diff/40001/content/browser/startup_t...
content/browser/startup_task_runner_unittest.cc:119:
scoped_ptr<StartupTaskRunner> runner(
Nit: why not allocate on stack?

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:184:
public boolean startBrowserProcessesSync(int max_renderers) {
maxRenderers

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:191:
prepareToStartBrowserProcess(max_renderers);
nit:indent

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

https://codereview.chromium.org/22691002/diff/40001/content/browser/android/b...
File content/browser/android/browser_startup_controller.cc (right):

https://codereview.chromium.org/22691002/diff/40001/content/browser/android/b...
content/browser/android/browser_startup_controller.cc:9: #include
"base/debug/debugger.h"
unused import?

https://codereview.chromium.org/22691002/diff/40001/content/browser/android/b...
content/browser/android/browser_startup_controller.cc:10: #include
"base/logging.h"
unused import?

https://codereview.chromium.org/22691002/diff/40001/content/browser/android/b...
content/browser/android/browser_startup_controller.cc:12: #include
"content/public/common/content_constants.h"
unused import?

https://codereview.chromium.org/22691002/diff/40001/content/browser/startup_t...
File content/browser/startup_task_runner.cc (right):

https://codereview.chromium.org/22691002/diff/40001/content/browser/startup_t...
content/browser/startup_task_runner.cc:30: if
(!startup_complete_callback_.is_null()) {
Nit: curly braces unnecessary.

https://codereview.chromium.org/22691002/diff/40001/content/browser/startup_t...
content/browser/startup_task_runner.cc:46: if (result > 0) {
Nit: curly braces unnecessary.

https://codereview.chromium.org/22691002/diff/40001/content/browser/startup_t...
content/browser/startup_task_runner.cc:50: if
(!startup_complete_callback_.is_null()) {
Nit: curly braces unnecessary.

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:160:
try {
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;
    }
}

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:189:
if (!mHasStartedInitializingBrowserProcess) {
Something like:
if (!mHasStartedInitializingBrowserProcess &&
!tryPrepareToStartBrowserProcess(maxRenderers))
  return false;

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:203:
assert(mStartupDone);
Nit: I think the style is to usually drop ( and )

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:228:
if (mStartupSuccess) {
Nit: Skip curly braces

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:268:
void disableAsynchronousStartup() {
private

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:316:
private static String getPlugins(final Context context) {
Remove static and use mContext, since it is now final.

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
File
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java
(right):

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:28:
if (!mLibraryLoadSucceeds) {
Nit: Unnecessary braces

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:37:

Nit: Unnecessary newline.

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:107:
// Asynchronous mode should have been set.
Nit: Unnecessary comment. If the assert message is unclear, update that instead
of adding a separate comment. Both a comment and an assert message seems
unnecessary. Please apply this across all of the tests in this file.

Naturally, for some of these, the message might get too long then, and if so
keep the comment too. I don't think there should be many of that type though.

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:111:
assertEquals("The browser process should have been initialized one time.", 1,
Nit: I know it is room for the 1 on the first line, but for the sake of
readability, could you put it at the start of the next line?

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:337:
assertTrue("Browser should have started succesfully",
Nit: successfully

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:349:
@SmallTest
Nit: Missing newline

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:379:
callback.mAlreadyStarted);
Question: How do you think about adding a simple extension of this test, that
ensures that following async requests also work as expected after the
synchronous startup, to ensure that all state is correct after this?

[aberent, 2013-08-23 11:40:20]

New patch will follow once I have tested these changes.

https://codereview.chromium.org/22691002/diff/2001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java
(right):

https://codereview.chromium.org/22691002/diff/2001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java:99:
BrowserStartupConfig.setSync();
On 2013/08/08 18:44:38, joth wrote:
> as BrowserStartupConfig is now driven from here, sounds like that class should
> now be package protected rather than public?
> BrowserStartupConfig.StartupCallback is the one snag, but may just mean the
> interface needs duplicating on here. (Or use ValueCallback<Int> for the
internal
> one or something. 

Now N/A. This class has gone away.

https://codereview.chromium.org/22691002/diff/2001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/AndroidBrowserProcess.java:110:
public static boolean init(Context context, int maxRendererProcesses)
On 2013/08/08 18:44:38, joth wrote:
> would make calling code easier to read if you had 2 public methods:
> 
> initSync(context, maxProcess) & initAsync(context, maxProcess, callback)
> 
> (they can share the internal private method taking a bool or whatever)
> 

Now N/A, this class has gone away. The replacement code in
BrowserStartupController does, however, follow your suggested pattern.

https://codereview.chromium.org/22691002/diff/31001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupConfig.java
(right):

https://codereview.chromium.org/22691002/diff/31001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupConfig.java:21:
private static LinkedList<StartupCallback> sCallbackList = null;
On 2013/08/20 06:08:52, Yaron wrote:
> Ya, this should go away after rebasing on Tommy's patch

Done.

https://codereview.chromium.org/22691002/diff/40001/content/app/android/conte...
File content/app/android/content_main.cc (right):

https://codereview.chromium.org/22691002/diff/40001/content/app/android/conte...
content/app/android/content_main.cc:38: static void Start(JNIEnv* env, jclass
clazz) {
On 2013/08/22 06:00:26, Yaron wrote:
> Doesn't this hide potential errors? I thought we had to add this support,
> possibly Michael Bai did, to report errors while doing some early
> initialization. 

Not any more. The only places such errors can come from are the startup tasks in
BrowserMainLoop. On Android there will always be a callback for startup
completion (even if we are doing sync. startup) and any error will be reported
by the callback, and handled by BrowserStartupController.

Given, however, that this is a content layer implementation detail, and is not
guaranteed by the interface, I was possibly overenthusiastic in changing this
and will change this back.

https://codereview.chromium.org/22691002/diff/40001/content/browser/android/b...
File content/browser/android/browser_startup_controller.cc (right):

https://codereview.chromium.org/22691002/diff/40001/content/browser/android/b...
content/browser/android/browser_startup_controller.cc:9: #include
"base/debug/debugger.h"
On 2013/08/23 06:28:32, nyquist wrote:
> unused import?

Done.

Sorry, copied from old android_browser_process.cc

https://codereview.chromium.org/22691002/diff/40001/content/browser/android/b...
content/browser/android/browser_startup_controller.cc:10: #include
"base/logging.h"
On 2013/08/23 06:28:32, nyquist wrote:
> unused import?

Done.

https://codereview.chromium.org/22691002/diff/40001/content/browser/android/b...
content/browser/android/browser_startup_controller.cc:12: #include
"content/public/common/content_constants.h"
On 2013/08/23 06:28:32, nyquist wrote:
> unused import?

Done.

https://codereview.chromium.org/22691002/diff/40001/content/browser/browser_m...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/22691002/diff/40001/content/browser/browser_m...
content/browser/browser_main_loop.cc:901: // Alert the clipboard class to which
threads are allowed to access the
On 2013/08/22 06:00:26, Yaron wrote:
> Nit: indentation

Done.

https://codereview.chromium.org/22691002/diff/40001/content/browser/browser_m...
File content/browser/browser_main_runner.cc (right):

https://codereview.chromium.org/22691002/diff/40001/content/browser/browser_m...
content/browser/browser_main_runner.cc:156: // True if the runner has been
initialized.
On 2013/08/22 06:00:26, Yaron wrote:
> Nit: update

Done.

https://codereview.chromium.org/22691002/diff/40001/content/browser/startup_t...
File content/browser/startup_task_runner.cc (right):

https://codereview.chromium.org/22691002/diff/40001/content/browser/startup_t...
content/browser/startup_task_runner.cc:18: // This is needed to satisfy the
coding style checks done by the Mac compiler,
On 2013/08/22 06:00:26, Yaron wrote:
> Nit: comment is unnecessary. It's standard style for chromium

Done.

https://codereview.chromium.org/22691002/diff/40001/content/browser/startup_t...
content/browser/startup_task_runner.cc:30: if
(!startup_complete_callback_.is_null()) {
On 2013/08/23 06:28:32, nyquist wrote:
> Nit: curly braces unnecessary.

Not done. 

The style guide is explicitly neutral on this point "In general, curly braces
are not required for single-line statements, but they are allowed if you like
them".

In the past I have found that not using curly braces in such situations is a
source of future bugs. What typically happens is that someone later tries to put
another statement (e.g. a trace statement) at the top of the conditional block
without noticing that there are no braces, hence changing the flow of control.

https://codereview.chromium.org/22691002/diff/40001/content/browser/startup_t...
content/browser/startup_task_runner.cc:46: if (result > 0) {
On 2013/08/23 06:28:32, nyquist wrote:
> Nit: curly braces unnecessary.

Done and break moved to line with if.

https://codereview.chromium.org/22691002/diff/40001/content/browser/startup_t...
content/browser/startup_task_runner.cc:50: if
(!startup_complete_callback_.is_null()) {
On 2013/08/23 06:28:32, nyquist wrote:
> Nit: curly braces unnecessary.

Not done. See comment on line 30.

https://codereview.chromium.org/22691002/diff/40001/content/browser/startup_t...
File content/browser/startup_task_runner.h (right):

https://codereview.chromium.org/22691002/diff/40001/content/browser/startup_t...
content/browser/startup_task_runner.h:11: #include "base/memory/ref_counted.h"
On 2013/08/22 06:00:26, Yaron wrote:
> Not needed anymore

Done.

https://codereview.chromium.org/22691002/diff/40001/content/browser/startup_t...
File content/browser/startup_task_runner_unittest.cc (right):

https://codereview.chromium.org/22691002/diff/40001/content/browser/startup_t...
content/browser/startup_task_runner_unittest.cc:119:
scoped_ptr<StartupTaskRunner> runner(
On 2013/08/22 06:00:26, Yaron wrote:
> Nit: why not allocate on stack?

Done.

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:160:
try {
On 2013/08/23 06:28:32, nyquist wrote:
> 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;
>     }
> }
> 

Done.

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:184:
public boolean startBrowserProcessesSync(int max_renderers) {
On 2013/08/22 06:00:26, Yaron wrote:
> maxRenderers

Done.

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:189:
if (!mHasStartedInitializingBrowserProcess) {
On 2013/08/23 06:28:32, nyquist wrote:
> Something like:
> if (!mHasStartedInitializingBrowserProcess &&
> !tryPrepareToStartBrowserProcess(maxRenderers))
>   return false;

Done.

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:191:
prepareToStartBrowserProcess(max_renderers);
On 2013/08/22 06:00:26, Yaron wrote:
> nit:indent

N/A, gone away.

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:203:
assert(mStartupDone);
On 2013/08/23 06:28:32, nyquist wrote:
> Nit: I think the style is to usually drop ( and )

Done.

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:228:
if (mStartupSuccess) {
On 2013/08/23 06:28:32, nyquist wrote:
> Nit: Skip curly braces

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.

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:268:
void disableAsynchronousStartup() {
On 2013/08/23 06:28:32, nyquist wrote:
> private

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).

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:316:
private static String getPlugins(final Context context) {
On 2013/08/23 06:28:32, nyquist wrote:
> Remove static and use mContext, since it is now final.

Done.

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
File
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java
(right):

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:28:
if (!mLibraryLoadSucceeds) {
On 2013/08/23 06:28:32, nyquist wrote:
> Nit: Unnecessary braces

Required by Java Code Style document.

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:37:

On 2013/08/23 06:28:32, nyquist wrote:
> Nit: Unnecessary newline.

Done.

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:107:
// Asynchronous mode should have been set.
On 2013/08/23 06:28:32, nyquist wrote:
> Nit: Unnecessary comment. If the assert message is unclear, update that
instead
> of adding a separate comment. Both a comment and an assert message seems
> unnecessary. Please apply this across all of the tests in this file.
> 
> Naturally, for some of these, the message might get too long then, and if so
> keep the comment too. I don't think there should be many of that type though.

Done.

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:111:
assertEquals("The browser process should have been initialized one time.", 1,
On 2013/08/23 06:28:32, nyquist wrote:
> Nit: I know it is room for the 1 on the first line, but for the sake of
> readability, could you put it at the start of the next line?

Done.

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:337:
assertTrue("Browser should have started succesfully",
On 2013/08/23 06:28:32, nyquist wrote:
> Nit: successfully

Done.

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:349:
@SmallTest
On 2013/08/23 06:28:32, nyquist wrote:
> Nit: Missing newline

Done.

https://codereview.chromium.org/22691002/diff/40001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:379:
callback.mAlreadyStarted);
On 2013/08/23 06:28:32, nyquist wrote:
> Question: How do you think about adding a simple extension of this test, that
> ensures that following async requests also work as expected after the
> synchronous startup, to ensure that all state is correct after this?

Done. Probably a sensible extra case.

[nyquist, 2013-08-23 15:58:03]

lgtm
I like the end result of this!

https://codereview.chromium.org/22691002/diff/58001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/22691002/diff/58001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:162:
if( contentStart() > 0) {
Nit: The ( is misplaced

https://codereview.chromium.org/22691002/diff/58001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:170:
*
Nit: This comment doesn't help a lot. I vote for removing it.

https://codereview.chromium.org/22691002/diff/58001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:205:
if( contentStart() > 0) {
Nit: The ( is misplaced.

https://codereview.chromium.org/22691002/diff/58001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:272:
* @param maxRendererProcesses
Nit: This Javadoc in fact manages to give less information than the method
signature itself, so I vote for just fully removing it.

[aberent, 2013-08-23 16:36:19]

https://codereview.chromium.org/22691002/diff/58001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/22691002/diff/58001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:162:
if( contentStart() > 0) {
On 2013/08/23 15:58:04, nyquist wrote:
> Nit: The ( is misplaced

Done.

https://codereview.chromium.org/22691002/diff/58001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:170:
*
On 2013/08/23 15:58:04, nyquist wrote:
> Nit: This comment doesn't help a lot. I vote for removing it.

Done.

https://codereview.chromium.org/22691002/diff/58001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:205:
if( contentStart() > 0) {
On 2013/08/23 15:58:04, nyquist wrote:
> Nit: The ( is misplaced.

Done.

https://codereview.chromium.org/22691002/diff/58001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:272:
* @param maxRendererProcesses
On 2013/08/23 15:58:04, nyquist wrote:
> Nit: This Javadoc in fact manages to give less information than the method
> signature itself, so I vote for just fully removing it.

Done.

[Yaron, 2013-08-23 17:03:28]

lgtm

[jam, 2013-08-26 13:37:20]

lgtm

[commit-bot: I haz the power, 2013-08-27 09:01:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aberent@chromium.org/22691002/54002

[commit-bot: I haz the power, 2013-08-27 15:30:02]

Change committed as 219795