Change ITestClient to be similar to IChildProcessService.

Change ITestClient to be similar to IChildProcessService.

Preparing the MultiprocessTestClientLauncher for transitioning to the
new base ChildProcessLauncher. More specifically, making ITestClient
similar to IChildProcessService. This requires moving extra methods to
a new interface ITestController, and creating a callback interface
ITestCallback that lets the service sets the ITestController interface
on the client.

BUG=702316
Review-Url: https://codereview.chromium.org/2781913002
Cr-Commit-Position: refs/heads/master@{#462683}
Committed: https://chromium.googlesource.com/chromium/src/+/9bd4d39997fab20c32cd0c7a75c45dbcf3f00642

[Jay Civelli, 2017-03-28 17:11:09]

Description was changed from

==========
Change ITestClient to be similar to IChildProcessService.

Preparing the MultiprocessTestClientLauncher for transitioning to the
new base ChildProcessLauncher. More specifically, making ITestClient
similar to IChildProcessService. This requires moving extra methods to
a new interface ITestController, and creating a callback interface
ITestCallback that lets the service sets the ITestController interface
on the client.

BUG=702316
==========

to

==========
Change ITestClient to be similar to IChildProcessService.

Preparing the MultiprocessTestClientLauncher for transitioning to the
new base ChildProcessLauncher. More specifically, making ITestClient
similar to IChildProcessService. This requires moving extra methods to
a new interface ITestController, and creating a callback interface
ITestCallback that lets the service sets the ITestController interface
on the client.

BUG=702316
==========

[Jay Civelli, 2017-03-28 17:11:09]

jcivelli@chromium.org changed reviewers:
+ nyquist@chromium.org

[Jay Civelli, 2017-03-28 17:11:14]

The CQ bit was checked by jcivelli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 17:11:49]

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

[Jay Civelli, 2017-03-29 20:11:55]

The CQ bit was checked by jcivelli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 20:12:53]

Dry run: 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-03-29 21:55:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-29 21:55:36]

Dry run: This issue passed the CQ dry run.

[nyquist, 2017-04-04 06:12:38]

https://codereview.chromium.org/2781913002/diff/20001/base/test/android/java/...
File base/test/android/java/src/org/chromium/base/ITestClient.aidl (right):

https://codereview.chromium.org/2781913002/diff/20001/base/test/android/java/...
base/test/android/java/src/org/chromium/base/ITestClient.aidl:17: boolean
bindToCaller();
Maybe I'm holding it wrong, but it seems to me that this method just returns
true and nothing else?

https://codereview.chromium.org/2781913002/diff/20001/base/test/android/java/...
base/test/android/java/src/org/chromium/base/ITestClient.aidl:26: int
setupConnection(in Bundle args, IBinder callback);
Should you document that this callback should be of type ITestCallback?

https://codereview.chromium.org/2781913002/diff/20001/base/test/android/java/...
File
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java
(right):

https://codereview.chromium.org/2781913002/diff/20001/base/test/android/java/...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:121:
Log.e(TAG, "Interrupted while waiting for connection PID.");
If this happens, should we still set mConnected = true, etc. below?

https://codereview.chromium.org/2781913002/diff/20001/base/test/android/java/...
File
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java
(right):

https://codereview.chromium.org/2781913002/diff/20001/base/test/android/java/...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:83:
// Required to unparcel FileDescriptorInfo.
Is this guaranteed to run on the main thread? Since you're posting to the main
thread for thread safety, I assume that'd always be the case? WDYT about adding
ThreadUtils.assertOnUiThread() here?

[Jay Civelli, 2017-04-04 17:06:22]

https://codereview.chromium.org/2781913002/diff/20001/base/test/android/java/...
File base/test/android/java/src/org/chromium/base/ITestClient.aidl (right):

https://codereview.chromium.org/2781913002/diff/20001/base/test/android/java/...
base/test/android/java/src/org/chromium/base/ITestClient.aidl:17: boolean
bindToCaller();
On 2017/04/04 06:12:37, nyquist wrote:
> Maybe I'm holding it wrong, but it seems to me that this method just returns
> true and nothing else?

In this implementation it does. The goal of this CL is to use the same AIDL
between this and the non test version (IChildProcessService), so when we are
done moving ChildProcessLauncher.java from content/ to base/, the test code can
be switched to using the more elaborate version.

https://codereview.chromium.org/2781913002/diff/20001/base/test/android/java/...
base/test/android/java/src/org/chromium/base/ITestClient.aidl:26: int
setupConnection(in Bundle args, IBinder callback);
On 2017/04/04 06:12:37, nyquist wrote:
> Should you document that this callback should be of type ITestCallback?

Added a comment that this is a work in progress to get to use a unique process
launcher (between tests and non tests).

https://codereview.chromium.org/2781913002/diff/20001/base/test/android/java/...
File
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java
(right):

https://codereview.chromium.org/2781913002/diff/20001/base/test/android/java/...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientLauncher.java:121:
Log.e(TAG, "Interrupted while waiting for connection PID.");
On 2017/04/04 06:12:37, nyquist wrote:
> If this happens, should we still set mConnected = true, etc. below?

Probably. I don't think we expect to be interrupted. Now rethrowing.

https://codereview.chromium.org/2781913002/diff/20001/base/test/android/java/...
File
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java
(right):

https://codereview.chromium.org/2781913002/diff/20001/base/test/android/java/...
base/test/android/java/src/org/chromium/base/MultiprocessTestClientService.java:83:
// Required to unparcel FileDescriptorInfo.
On 2017/04/04 06:12:37, nyquist wrote:
> Is this guaranteed to run on the main thread? Since you're posting to the main
> thread for thread safety, I assume that'd always be the case? WDYT about
adding
> ThreadUtils.assertOnUiThread() here?

This is not called on the main thread but on a background thread (since this is
a bound service method call).

[nyquist, 2017-04-05 17:59:41]

lgtm

[Jay Civelli, 2017-04-05 18:01:03]

The CQ bit was checked by jcivelli@chromium.org

[commit-bot: I haz the power, 2017-04-05 18:02:34]

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-04-05 18:12:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 18:12:43]

Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[Jay Civelli, 2017-04-06 22:05:44]

The CQ bit was checked by jcivelli@chromium.org

[Jay Civelli, 2017-04-06 22:05:45]

The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/2781913002/#ps60001
(title: "Fix compile error   sync")

[commit-bot: I haz the power, 2017-04-06 22:07:08]

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-04-06 23:37:20]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491516344940930,
"parent_rev": "2fd7014105d562d587191d8875f98e448ce45508", "commit_rev":
"9bd4d39997fab20c32cd0c7a75c45dbcf3f00642"}

[commit-bot: I haz the power, 2017-04-06 23:38:21]

Description was changed from

==========
Change ITestClient to be similar to IChildProcessService.

Preparing the MultiprocessTestClientLauncher for transitioning to the
new base ChildProcessLauncher. More specifically, making ITestClient
similar to IChildProcessService. This requires moving extra methods to
a new interface ITestController, and creating a callback interface
ITestCallback that lets the service sets the ITestController interface
on the client.

BUG=702316
==========

to

==========
Change ITestClient to be similar to IChildProcessService.

Preparing the MultiprocessTestClientLauncher for transitioning to the
new base ChildProcessLauncher. More specifically, making ITestClient
similar to IChildProcessService. This requires moving extra methods to
a new interface ITestController, and creating a callback interface
ITestCallback that lets the service sets the ITestController interface
on the client.

BUG=702316

Review-Url: https://codereview.chromium.org/2781913002
Cr-Commit-Position: refs/heads/master@{#462683}
Committed:
https://chromium.googlesource.com/chromium/src/+/9bd4d39997fab20c32cd0c7a75c4...
==========

[commit-bot: I haz the power, 2017-04-06 23:38:22]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/9bd4d39997fab20c32cd0c7a75c4...