Refactoring ChildProcessConnection.

Refactoring ChildProcessConnection.

Splitting the ChildProcessConnectionImpl class in 2 and changing it to
an abstract class instead of an interface.
ChildProcessConnections can now be either:
- ImportantChildProcessConnection: bound BIND_IMPORTANT and used for
  the GPU process. This is equivalent to the previous
  alwaysInForeground ChildProcessConnectionImpl.
- ManagedChildProcessConnection: managed by the BindingManager.
With that split, only ManagedChildProcessConnection are now passed to
the BindingManager.

Also changing the ChildProcessLauncherHelper to keep a reference to the
ChildProcessConnection so that it can determine if it is OOM protected
without the help of the BindingManager. As a result, the BindingManager
does not keep ChildProcessConnections around when they are cleared.

Also changed BindingManagerImplTest to exercise the actual connection
code (instead of mocking some of the logic in the test).

BUG=689758
Review-Url: https://codereview.chromium.org/2828793002
Cr-Commit-Position: refs/heads/master@{#467166}
Committed: https://chromium.googlesource.com/chromium/src/+/faaae321d190728d93ff5b51c961c3d28c016ae5

[Jay Civelli, 2017-04-19 17:03:01]

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

[commit-bot: I haz the power, 2017-04-19 17:03:45]

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-04-19 18:00:31]

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

[commit-bot: I haz the power, 2017-04-19 18:00:32]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Jay Civelli, 2017-04-19 23:40:48]

Description was changed from

==========
Refactoring ChildProcessConnection.

Splitting the ChildProcessConnectionImpl class in 2, and changing it to
an abstract class instead of an interface.
There is now an ImportantChildProcessConnection that is used for the GPU
process for example and that is bound as BIND_IMPORTANT, and a
ManagedChildProcessConnection which is managed by the BindingManager.
This makes it such that only managed connections are now passed to the
BindingManager.

Also changing the ChildProcessLauncherHelper to keep a reference to the
ChildProcessConnection so that it can determine if it is OOM protected
without the help of the BindingManager. As a result, the BindingManager
does not keep ChildProcessConnection around when they are cleared.

BUG=689758
==========

to

==========
Refactoring ChildProcessConnection.

Splitting the ChildProcessConnectionImpl class in 2, and changing it to
an abstract class instead of an interface.
There is now an ImportantChildProcessConnection that is used for the GPU
process (for example) and that is bound as BIND_IMPORTANT. There is also
a ManagedChildProcessConnection class which is managed by the
BindingManager. Only managed connections are now passed to the
BindingManager.

Also changing the ChildProcessLauncherHelper to keep a reference to the
ChildProcessConnection so that it can determine if it is OOM protected
without the help of the BindingManager. As a result, the BindingManager
does not keep ChildProcessConnection around when they are cleared.

Also changed BindingManagerImplTest to test exercise the actual
connection code (instead of duplicating some of the logic in the test).

BUG=689758
==========

[Jay Civelli, 2017-04-19 23:43:26]

Patchset #1 (id:1) has been deleted

[Jay Civelli, 2017-04-19 23:43:33]

Patchset #1 (id:20001) has been deleted

[Jay Civelli, 2017-04-19 23:47:08]

Description was changed from

==========
Refactoring ChildProcessConnection.

Splitting the ChildProcessConnectionImpl class in 2, and changing it to
an abstract class instead of an interface.
There is now an ImportantChildProcessConnection that is used for the GPU
process (for example) and that is bound as BIND_IMPORTANT. There is also
a ManagedChildProcessConnection class which is managed by the
BindingManager. Only managed connections are now passed to the
BindingManager.

Also changing the ChildProcessLauncherHelper to keep a reference to the
ChildProcessConnection so that it can determine if it is OOM protected
without the help of the BindingManager. As a result, the BindingManager
does not keep ChildProcessConnection around when they are cleared.

Also changed BindingManagerImplTest to test exercise the actual
connection code (instead of duplicating some of the logic in the test).

BUG=689758
==========

to

==========
Refactoring ChildProcessConnection.

Splitting the ChildProcessConnectionImpl class in 2, and changing it to
an abstract class instead of an interface.
ChildProcessConnections can now be:
- ImportantChildProcessConnection: they are bound BIND_IMPORTANT and
  used for the GPU process.
- ManagedChildProcessConnection: they are managed by the 
  BindingManager.
With that split, only ManagedChildProcessConnection are now passed to
the BindingManager.

Also changing the ChildProcessLauncherHelper to keep a reference to the
ChildProcessConnection so that it can determine if it is OOM protected
without the help of the BindingManager. As a result, the BindingManager
does not keep ChildProcessConnection around when they are cleared.

Also changed BindingManagerImplTest to exercise the actual connection code
(instead of mocking some of the logic in the test).

BUG=689758
==========

[Jay Civelli, 2017-04-19 23:48:05]

Description was changed from

==========
Refactoring ChildProcessConnection.

Splitting the ChildProcessConnectionImpl class in 2, and changing it to
an abstract class instead of an interface.
ChildProcessConnections can now be:
- ImportantChildProcessConnection: they are bound BIND_IMPORTANT and
  used for the GPU process.
- ManagedChildProcessConnection: they are managed by the 
  BindingManager.
With that split, only ManagedChildProcessConnection are now passed to
the BindingManager.

Also changing the ChildProcessLauncherHelper to keep a reference to the
ChildProcessConnection so that it can determine if it is OOM protected
without the help of the BindingManager. As a result, the BindingManager
does not keep ChildProcessConnection around when they are cleared.

Also changed BindingManagerImplTest to exercise the actual connection code
(instead of mocking some of the logic in the test).

BUG=689758
==========

to

==========
Refactoring ChildProcessConnection.

Splitting the ChildProcessConnectionImpl class in 2, and changing it to
an abstract class instead of an interface.
ChildProcessConnections can now be either:
- ImportantChildProcessConnection: bound BIND_IMPORTANT and used for
  the GPU process.
- ManagedChildProcessConnection: managed by the BindingManager.
With that split, only ManagedChildProcessConnection are now passed to
the BindingManager.

Also changing the ChildProcessLauncherHelper to keep a reference to the
ChildProcessConnection so that it can determine if it is OOM protected
without the help of the BindingManager. As a result, the BindingManager
does not keep ChildProcessConnection around when they are cleared.

Also changed BindingManagerImplTest to exercise the actual connection code
(instead of mocking some of the logic in the test).

BUG=689758
==========

[Jay Civelli, 2017-04-19 23:48:05]

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

[Jay Civelli, 2017-04-19 23:48:12]

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

[Jay Civelli, 2017-04-19 23:48:53]

Description was changed from

==========
Refactoring ChildProcessConnection.

Splitting the ChildProcessConnectionImpl class in 2, and changing it to
an abstract class instead of an interface.
ChildProcessConnections can now be either:
- ImportantChildProcessConnection: bound BIND_IMPORTANT and used for
  the GPU process.
- ManagedChildProcessConnection: managed by the BindingManager.
With that split, only ManagedChildProcessConnection are now passed to
the BindingManager.

Also changing the ChildProcessLauncherHelper to keep a reference to the
ChildProcessConnection so that it can determine if it is OOM protected
without the help of the BindingManager. As a result, the BindingManager
does not keep ChildProcessConnection around when they are cleared.

Also changed BindingManagerImplTest to exercise the actual connection code
(instead of mocking some of the logic in the test).

BUG=689758
==========

to

==========
Refactoring ChildProcessConnection.

Splitting the ChildProcessConnectionImpl class in 2, and changing it to
an abstract class instead of an interface.
ChildProcessConnections can now be either:
- ImportantChildProcessConnection: bound BIND_IMPORTANT and used for
  the GPU process.
- ManagedChildProcessConnection: managed by the BindingManager.
With that split, only ManagedChildProcessConnection are now passed to
the BindingManager.

Also changing the ChildProcessLauncherHelper to keep a reference to the
ChildProcessConnection so that it can determine if it is OOM protected
without the help of the BindingManager. As a result, the BindingManager
does not keep ChildProcessConnections around when they are cleared.

Also changed BindingManagerImplTest to exercise the actual connection
code (instead of mocking some of the logic in the test).

BUG=689758
==========

[commit-bot: I haz the power, 2017-04-19 23:48:59]

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-04-19 23:53:50]

Description was changed from

==========
Refactoring ChildProcessConnection.

Splitting the ChildProcessConnectionImpl class in 2, and changing it to
an abstract class instead of an interface.
ChildProcessConnections can now be either:
- ImportantChildProcessConnection: bound BIND_IMPORTANT and used for
  the GPU process.
- ManagedChildProcessConnection: managed by the BindingManager.
With that split, only ManagedChildProcessConnection are now passed to
the BindingManager.

Also changing the ChildProcessLauncherHelper to keep a reference to the
ChildProcessConnection so that it can determine if it is OOM protected
without the help of the BindingManager. As a result, the BindingManager
does not keep ChildProcessConnections around when they are cleared.

Also changed BindingManagerImplTest to exercise the actual connection
code (instead of mocking some of the logic in the test).

BUG=689758
==========

to

==========
Refactoring ChildProcessConnection.

Splitting the ChildProcessConnectionImpl class in 2 and changing it to
an abstract class instead of an interface.
ChildProcessConnections can now be either:
- ImportantChildProcessConnection: bound BIND_IMPORTANT and used for
  the GPU process. This is equivalent to the previous
  alwaysInForeground ChildProcessConnectionImpl.
- ManagedChildProcessConnection: managed by the BindingManager.
With that split, only ManagedChildProcessConnection are now passed to
the BindingManager.

Also changing the ChildProcessLauncherHelper to keep a reference to the
ChildProcessConnection so that it can determine if it is OOM protected
without the help of the BindingManager. As a result, the BindingManager
does not keep ChildProcessConnections around when they are cleared.

Also changed BindingManagerImplTest to exercise the actual connection
code (instead of mocking some of the logic in the test).

BUG=689758
==========

[commit-bot: I haz the power, 2017-04-20 00:44:40]

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

[commit-bot: I haz the power, 2017-04-20 00:44:41]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Jay Civelli, 2017-04-20 15:10:34]

I still need to fix a couple of tests but should be ready for review.

[boliu, 2017-04-20 22:33:34]

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
File content/public/android/BUILD.gn (right):

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
content/public/android/BUILD.gn:78:
"java/src/org/chromium/content/browser/BaseChildProcessConnection.java",
order

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/BaseChildProcessConnection.java
(right):

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BaseChildProcessConnection.java:165:
private final Context mContext;
style nit: no blank lines in between

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BaseChildProcessConnection.java:198:
private final Object mLock = new Object();
I think it only became clear after this refactor, but this lock is not needed.
stop and onSetupConnectionResult both already happen on launcher thread (and
should have asserts added)

Java reference and int assignment are atomic, so there is no need to protect
getService, getPid, isConnected

And then there's only crashServiceForTesting and we can just fix the test

But maybe should do this in a separate CL first, rather than in this big
refactor.

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BaseChildProcessConnection.java:251:
public Context getContext() {
mark methods like this that are not meant to be overridden with final?

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java
(right):

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:71:
// This will be handled by |mDelayedClearer
what happened to this comment?

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:453:
mManagedConnections.remove(pid);
\o/

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:454:
managedConnection.clearConnection();
clearConnection can be outside of lock

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java
(right):

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java:89:
if (mChildProcessConnection == null) {
hmm, this is "kind of" thread safe, only because mChildProcessConnection is set
at most once, and can only go from null to non-null.

either add a lock, or document this?

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ImportantChildProcessConnection.java
(right):

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ImportantChildProcessConnection.java:42:
protected void initBinding() {
nothing actually overrides this?

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ManagedChildProcessConnection.java
(right):

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ManagedChildProcessConnection.java:81:
protected void initBindings() {
again, nothing is overriding this? can just be part of constructor?

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ManagedChildProcessConnection.java:84:
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N && getCreationParams() !=
null
avoid duplicating this if condition, just expand isExportedService with these
other checks as well?

[Jay Civelli, 2017-04-25 00:38:32]

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

[commit-bot: I haz the power, 2017-04-25 00:39:05]

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-04-25 02:32:51]

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

[commit-bot: I haz the power, 2017-04-25 02:32:52]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Jay Civelli, 2017-04-25 06:00:26]

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

[commit-bot: I haz the power, 2017-04-25 06:00:41]

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-04-25 06:02:47]

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
File content/public/android/BUILD.gn (right):

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
content/public/android/BUILD.gn:78:
"java/src/org/chromium/content/browser/BaseChildProcessConnection.java",
On 2017/04/20 22:33:34, boliu wrote:
> order

Done.

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/BaseChildProcessConnection.java
(right):

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BaseChildProcessConnection.java:165:
private final Context mContext;
On 2017/04/20 22:33:34, boliu wrote:
> style nit: no blank lines in between

I removed some of them. I am not sure I understand the logic of when to add
blank lines between them. In the files I looked at it seems more or less
arbitrary.

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BaseChildProcessConnection.java:198:
private final Object mLock = new Object();
On 2017/04/20 22:33:34, boliu wrote:
> I think it only became clear after this refactor, but this lock is not needed.
> stop and onSetupConnectionResult both already happen on launcher thread (and
> should have asserts added)
> 
> Java reference and int assignment are atomic, so there is no need to protect
> getService, getPid, isConnected
> 
> And then there's only crashServiceForTesting and we can just fix the test
> 
> But maybe should do this in a separate CL first, rather than in this big
> refactor.

Will do in a separate CL. Filed bug and added TODO.

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BaseChildProcessConnection.java:251:
public Context getContext() {
On 2017/04/20 22:33:34, boliu wrote:
> mark methods like this that are not meant to be overridden with final?

Done.

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java
(right):

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:71:
// This will be handled by |mDelayedClearer
On 2017/04/20 22:33:34, boliu wrote:
> what happened to this comment?
Fixed.

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:453:
mManagedConnections.remove(pid);
On 2017/04/20 22:33:34, boliu wrote:
> \o/

Acknowledged.

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:454:
managedConnection.clearConnection();
On 2017/04/20 22:33:34, boliu wrote:
> clearConnection can be outside of lock

Done.

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java
(right):

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java:89:
if (mChildProcessConnection == null) {
On 2017/04/20 22:33:34, boliu wrote:
> hmm, this is "kind of" thread safe, only because mChildProcessConnection is
set
> at most once, and can only go from null to non-null.
> 
> either add a lock, or document this?
As you mentioned before, reference assignation are atomic, so the lock would not
give us much (since once it is non null it stays that way).
Added comment.

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ImportantChildProcessConnection.java
(right):

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ImportantChildProcessConnection.java:42:
protected void initBinding() {
On 2017/04/20 22:33:34, boliu wrote:
> nothing actually overrides this?

Removed @VisibleForTesting.

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ManagedChildProcessConnection.java
(right):

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ManagedChildProcessConnection.java:81:
protected void initBindings() {
On 2017/04/20 22:33:34, boliu wrote:
> again, nothing is overriding this? can just be part of constructor?

Oh right, we can do this now. (I used that originally in tests to disable actual
bindings before I abstracted out the ServiceConnection in
BaseChildProcessConnection).

https://codereview.chromium.org/2828793002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ManagedChildProcessConnection.java:84:
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N && getCreationParams() !=
null
On 2017/04/20 22:33:34, boliu wrote:
> avoid duplicating this if condition, just expand isExportedService with these
> other checks as well?

Done.

[commit-bot: I haz the power, 2017-04-25 07:35:41]

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

[commit-bot: I haz the power, 2017-04-25 07:35:42]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[boliu, 2017-04-25 15:53:49]

lgtm

https://codereview.chromium.org/2828793002/diff/90001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ManagedChildProcessConnection.java
(right):

https://codereview.chromium.org/2828793002/diff/90001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ManagedChildProcessConnection.java:78:
synchronized (mBindingLock) {
nit: no need to synchronize in constructor. it's the constructor..

[Jay Civelli, 2017-04-25 18:54:11]

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

[commit-bot: I haz the power, 2017-04-25 18:54:57]

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-04-25 20:04:21]

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

[commit-bot: I haz the power, 2017-04-25 20:04:22]

Dry run: This issue passed the CQ dry run.

[Jay Civelli, 2017-04-25 20:53:23]

The CQ bit was checked by jcivelli@chromium.org

[Jay Civelli, 2017-04-25 20:53:23]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2828793002/#ps110001
(title: "More test fixing.")

[commit-bot: I haz the power, 2017-04-25 20:54:15]

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-25 21:05:17]

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

[commit-bot: I haz the power, 2017-04-25 21:05:19]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Jay Civelli, 2017-04-25 21:19:54]

jcivelli@chromium.org changed reviewers:
+ dtrainor@chromium.org, jbudorick@chromium.org

[Jay Civelli, 2017-04-25 21:20:53]

Adding OWNER reviewers:
dtrainor@ for
	chrome/android/javatests/src/org/chromium/chrome/browser/BindingManagerIntegrationTest.java
jbudorick@ for build/android/lint/suppressions.xml

[jbudorick, 2017-04-25 21:24:41]

On 2017/04/25 21:20:53, Jay Civelli wrote:
> Adding OWNER reviewers:
> dtrainor@ for
>
	chrome/android/javatests/src/org/chromium/chrome/browser/BindingManagerIntegrationTest.java
> jbudorick@ for build/android/lint/suppressions.xml

lgtm

[David Trainor- moved to gerrit, 2017-04-25 23:44:39]

BindingManagerIntegrationTest.java lgtm

[Jay Civelli, 2017-04-25 23:46:22]

The CQ bit was checked by jcivelli@chromium.org

[commit-bot: I haz the power, 2017-04-25 23:47:17]

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-25 23:55:55]

CQ is committing da patch.

Bot data: {"patchset_id": 110001, "attempt_start_ts": 1493163982820840,
"parent_rev": "212d4283c7de2e8120cdf8c986dda3fcade13048", "commit_rev":
"faaae321d190728d93ff5b51c961c3d28c016ae5"}

[commit-bot: I haz the power, 2017-04-25 23:56:07]

Description was changed from

==========
Refactoring ChildProcessConnection.

Splitting the ChildProcessConnectionImpl class in 2 and changing it to
an abstract class instead of an interface.
ChildProcessConnections can now be either:
- ImportantChildProcessConnection: bound BIND_IMPORTANT and used for
  the GPU process. This is equivalent to the previous
  alwaysInForeground ChildProcessConnectionImpl.
- ManagedChildProcessConnection: managed by the BindingManager.
With that split, only ManagedChildProcessConnection are now passed to
the BindingManager.

Also changing the ChildProcessLauncherHelper to keep a reference to the
ChildProcessConnection so that it can determine if it is OOM protected
without the help of the BindingManager. As a result, the BindingManager
does not keep ChildProcessConnections around when they are cleared.

Also changed BindingManagerImplTest to exercise the actual connection
code (instead of mocking some of the logic in the test).

BUG=689758
==========

to

==========
Refactoring ChildProcessConnection.

Splitting the ChildProcessConnectionImpl class in 2 and changing it to
an abstract class instead of an interface.
ChildProcessConnections can now be either:
- ImportantChildProcessConnection: bound BIND_IMPORTANT and used for
  the GPU process. This is equivalent to the previous
  alwaysInForeground ChildProcessConnectionImpl.
- ManagedChildProcessConnection: managed by the BindingManager.
With that split, only ManagedChildProcessConnection are now passed to
the BindingManager.

Also changing the ChildProcessLauncherHelper to keep a reference to the
ChildProcessConnection so that it can determine if it is OOM protected
without the help of the BindingManager. As a result, the BindingManager
does not keep ChildProcessConnections around when they are cleared.

Also changed BindingManagerImplTest to exercise the actual connection
code (instead of mocking some of the logic in the test).

BUG=689758

Review-Url: https://codereview.chromium.org/2828793002
Cr-Commit-Position: refs/heads/master@{#467166}
Committed:
https://chromium.googlesource.com/chromium/src/+/faaae321d190728d93ff5b51c961...
==========

[commit-bot: I haz the power, 2017-04-25 23:56:12]

Committed patchset #5 (id:110001) as
https://chromium.googlesource.com/chromium/src/+/faaae321d190728d93ff5b51c961...