Address follow-up comments from https://codereview.chromium.org/2626413004/.

Address follow-up comments from https://codereview.chromium.org/2626413004/.

This also de-flakes the testBindServiceFromMultipleProcesses test.

BUG=683133, 685052
R=boliu@chromium.org

Review-Url: https://codereview.chromium.org/2652293002
Cr-Commit-Position: refs/heads/master@{#446069}
Committed: https://chromium.googlesource.com/chromium/src/+/7e926b67a55af0b521951664cb67b95947a10eab

[Robert Sesek, 2017-01-25 16:13:52]

(comments from original CL below)

https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java
(right):

https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:181:
if (mStartCallback != null) {
On 2017/01/24 17:37:34, boliu wrote:
> nit: could do this check first, so it's not repeated and easier to read

Done.

https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:187:
}
On 2017/01/24 17:37:34, boliu wrote:
> can set mStartCallback to null after? the closure seem to have a lot of
state..

Done.

https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:730:
AsyncTask.THREAD_POOL_EXECUTOR.execute(new Runnable() {
On 2017/01/24 17:37:34, boliu wrote:
> can we just use the launcher thread (through native code I guess..) AsyncTask
> thread pool is shared with the app, so can be backed up, and launching child
> process is pretty time sensitive?
> 
> or maybe not, this is only webview when app is using this from multiple
> processes, so they are doing something already

I would prefer to do this on the ProcessLauncher thread, but there's no Java
handle to it. Is it worth round-tripping through JNI to avoid this? (We're
already operating in an edge condition here).

https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
File
content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java
(right):

https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:406:
ChildProcessLauncher.getServiceMapForTesting();
On 2017/01/24 17:37:34, boliu wrote:
> I guess ideally, this should wait for the actual nativeOnChildProcessStarted
> call. But mocking that might be hard.

Right, we're interested in the native callback getting run, but there is no
plumbing for testing that.

[boliu, 2017-01-25 16:58:57]

On 2017/01/25 16:13:52, Robert Sesek wrote:
> (comments from original CL below)
> 
>
https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java
> (right):
> 
>
https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:181:
> if (mStartCallback != null) {
> On 2017/01/24 17:37:34, boliu wrote:
> > nit: could do this check first, so it's not repeated and easier to read
> 
> Done.
> 
>
https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:187:
> }
> On 2017/01/24 17:37:34, boliu wrote:
> > can set mStartCallback to null after? the closure seem to have a lot of
> state..
> 
> Done.
> 
>
https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
> (right):
> 
>
https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:730:
> AsyncTask.THREAD_POOL_EXECUTOR.execute(new Runnable() {
> On 2017/01/24 17:37:34, boliu wrote:
> > can we just use the launcher thread (through native code I guess..)
AsyncTask
> > thread pool is shared with the app, so can be backed up, and launching child
> > process is pretty time sensitive?
> > 
> > or maybe not, this is only webview when app is using this from multiple
> > processes, so they are doing something already
> 
> I would prefer to do this on the ProcessLauncher thread, but there's no Java
> handle to it. Is it worth round-tripping through JNI to avoid this? (We're
> already operating in an edge condition here).

Although it's a "not-official-supported" case, it's not that uncommon, when
gmscore and facebook does it (facebook might have fixed it or not, can't
remember..)

Also it's a bit less cognitive load for me at least, that we are not introducing
another random thread into the mix.

So I do think that change is worth it.

> 
>
https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
> File
>
content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java
> (right):
> 
>
https://codereview.chromium.org/2626413004/diff/150001/content/public/android...
>
content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:406:
> ChildProcessLauncher.getServiceMapForTesting();
> On 2017/01/24 17:37:34, boliu wrote:
> > I guess ideally, this should wait for the actual nativeOnChildProcessStarted
> > call. But mocking that might be hard.
> 
> Right, we're interested in the native callback getting run, but there is no
> plumbing for testing that.

[boliu, 2017-01-25 17:41:09]

existing changes lgtm, maybe you wanna land this now to green up the test, and
then do launcher thread change in another follow up

[Robert Sesek, 2017-01-25 17:58:22]

On 2017/01/25 17:41:09, boliu wrote:
> existing changes lgtm, maybe you wanna land this now to green up the test, and
> then do launcher thread change in another follow up

Sure. Almost have a fix but am stuck on some JNI business.

[Robert Sesek, 2017-01-25 17:58:26]

The CQ bit was checked by rsesek@chromium.org

[commit-bot: I haz the power, 2017-01-25 17:59:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 18:55:03]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1485367106117950, "parent_rev":
"7a635db552dea8279eb7655e876555fad7e3aea2", "commit_rev":
"7e926b67a55af0b521951664cb67b95947a10eab"}

[commit-bot: I haz the power, 2017-01-25 18:55:33]

Description was changed from

==========
Address follow-up comments from https://codereview.chromium.org/2626413004/.

This also de-flakes the testBindServiceFromMultipleProcesses test.

BUG=683133,685052
R=boliu@chromium.org
==========

to

==========
Address follow-up comments from https://codereview.chromium.org/2626413004/.

This also de-flakes the testBindServiceFromMultipleProcesses test.

BUG=683133,685052
R=boliu@chromium.org

Review-Url: https://codereview.chromium.org/2652293002
Cr-Commit-Position: refs/heads/master@{#446069}
Committed:
https://chromium.googlesource.com/chromium/src/+/7e926b67a55af0b521951664cb67...
==========

[commit-bot: I haz the power, 2017-01-25 18:55:37]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/7e926b67a55af0b521951664cb67...