android: Java ChildProcessLauncherHelper instance

android: Java ChildProcessLauncherHelper instance

This is mostly meant to be a no-op refactor. There is a long term need
to have ChildProcessLauncher(Helper) to have a java _instance_ to save
state, as an alternative to putting things into global maps keyed by
the pid. See bug for specific use.

So create a java ChildProcessLauncherHelper, which is instantiated and
owned by native ChildProcessLauncherHelper. Then move all jni calls
from ChildProcessLauncher (which only had static methods) to
ChildProcessLauncherHelper. Also convert the jni calls from static
calls to instance calls where possible; the major except right is
Terminate/Stop, because it's used as a static method.

Replace the opaque "clientContext" native pointer in
ChildProcessLauncher with a java callback implemented by Helper.
Helper should hold a pointer directly to native peer, but due to
threading/ownership issues, it is set back to 0 before native side is
actually deleted.

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

[boliu, 2017-03-27 14:36:03]

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

[commit-bot: I haz the power, 2017-03-27 14:36:17]

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

[boliu, 2017-03-27 15:53:16]

The CQ bit was unchecked by boliu@chromium.org

[boliu, 2017-03-27 16:01:49]

Description was changed from

==========
android: Java ChildProcessLauncherHelper instance

WIP

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

to

==========
android: Java ChildProcessLauncherHelper instance

This is mostly meant to be a no-op refactor. There is a long term need
to have ChildProcessLauncher(Helper) to have a java _instance_ to save
state, as an alternative to putting things into global maps keyed by
the pid. See bug for specific use.

So create a java ChildProcessLauncherHelper, which is instantiated and
owned by native ChildProcessLauncherHelper. Then move all jni calls
from ChildProcessLauncher (which only had static methods) to
ChildProcessLauncherHelper. Also convert the jni calls from static
calls to instance calls where possible; the major except right is
Terminate/Stop, because it's used as a static method.

Replace the opaque "clientContext" native pointer in
ChildProcessLauncher with a java callback implemented by Helper.
Helper should hold a pointer directly to native peer in the future,
but due to threading/ownership issues, this is not the case, and it
holds a pointer to a reference of native peer instead.

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

[boliu, 2017-03-27 16:02:39]

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

[boliu, 2017-03-27 16:02:39]

ptal

jcivelli for the native parts
maria for the java parts

[boliu, 2017-03-27 16:02:43]

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

[commit-bot: I haz the power, 2017-03-27 16:03:26]

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-27 18:23:28]

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

[commit-bot: I haz the power, 2017-03-27 18:23:29]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Maria, 2017-03-27 19:22:52]

lgtm

https://codereview.chromium.org/2774163002/diff/60001/content/browser/child_p...
File content/browser/child_process_launcher_helper_android.cc (right):

https://codereview.chromium.org/2774163002/diff/60001/content/browser/child_p...
content/browser/child_process_launcher_helper_android.cc:214: delete
reinterpret_cast<scoped_refptr<ChildProcessLauncherHelper>*>(pointer);
these two lines look really odd to me. Do you want to add a comment on what you
are doing here?

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

https://codereview.chromium.org/2774163002/diff/60001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java:26:
// Note this is not the actual native pointer to ChildProcessLauncherHelper.
It might be nice to say what it IS in addition to what it isn't

[Jay Civelli, 2017-03-27 19:49:48]

https://codereview.chromium.org/2774163002/diff/60001/content/browser/child_p...
File content/browser/child_process_launcher_helper_android.cc (right):

https://codereview.chromium.org/2774163002/diff/60001/content/browser/child_p...
content/browser/child_process_launcher_helper_android.cc:214: delete
reinterpret_cast<scoped_refptr<ChildProcessLauncherHelper>*>(pointer);
Could OnChildProcessStarted be a non static method on
ChildProcessLauncherHelper.java?
Then you would manually increase the ref count in
ChildProcessLauncherHelper::LaunchProcessOnLauncherThread() and decrease it in
here and would not have to deal with passing references around.

[boliu, 2017-03-27 20:47:53]

https://codereview.chromium.org/2774163002/diff/60001/content/browser/child_p...
File content/browser/child_process_launcher_helper_android.cc (right):

https://codereview.chromium.org/2774163002/diff/60001/content/browser/child_p...
content/browser/child_process_launcher_helper_android.cc:214: delete
reinterpret_cast<scoped_refptr<ChildProcessLauncherHelper>*>(pointer);
On 2017/03/27 19:49:48, Jay Civelli wrote:
> Could OnChildProcessStarted be a non static method on
> ChildProcessLauncherHelper.java?
> Then you would manually increase the ref count in
> ChildProcessLauncherHelper::LaunchProcessOnLauncherThread() and decrease it in
> here and would not have to deal with passing references around.

Ahh, good suggestion. I guess only down side is more OS_ANDROID defs in shared
header, but not that bad. Resolves both of maria's comments :p

[Jay Civelli, 2017-03-27 21:12:34]

lgtm

[boliu, 2017-03-27 21:18:33]

Description was changed from

==========
android: Java ChildProcessLauncherHelper instance

This is mostly meant to be a no-op refactor. There is a long term need
to have ChildProcessLauncher(Helper) to have a java _instance_ to save
state, as an alternative to putting things into global maps keyed by
the pid. See bug for specific use.

So create a java ChildProcessLauncherHelper, which is instantiated and
owned by native ChildProcessLauncherHelper. Then move all jni calls
from ChildProcessLauncher (which only had static methods) to
ChildProcessLauncherHelper. Also convert the jni calls from static
calls to instance calls where possible; the major except right is
Terminate/Stop, because it's used as a static method.

Replace the opaque "clientContext" native pointer in
ChildProcessLauncher with a java callback implemented by Helper.
Helper should hold a pointer directly to native peer in the future,
but due to threading/ownership issues, this is not the case, and it
holds a pointer to a reference of native peer instead.

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

to

==========
android: Java ChildProcessLauncherHelper instance

This is mostly meant to be a no-op refactor. There is a long term need
to have ChildProcessLauncher(Helper) to have a java _instance_ to save
state, as an alternative to putting things into global maps keyed by
the pid. See bug for specific use.

So create a java ChildProcessLauncherHelper, which is instantiated and
owned by native ChildProcessLauncherHelper. Then move all jni calls
from ChildProcessLauncher (which only had static methods) to
ChildProcessLauncherHelper. Also convert the jni calls from static
calls to instance calls where possible; the major except right is
Terminate/Stop, because it's used as a static method.

Replace the opaque "clientContext" native pointer in
ChildProcessLauncher with a java callback implemented by Helper.
Helper should hold a pointer directly to native peer, but due to
threading/ownership issues, it is set back to 0 before native side is
actually deleted.

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

[boliu, 2017-03-27 21:23:12]

The CQ bit was checked by boliu@chromium.org

[boliu, 2017-03-27 21:23:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2774163002/#ps80001
(title: "explicit AddRef")

[commit-bot: I haz the power, 2017-03-27 21:23:47]

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-27 22:49:38]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490649792031680,
"parent_rev": "e6d2c8d3b8904eac6d1204c5ec2d738e4d6d43eb", "commit_rev":
"4ff655334a321362a3611d0e1d0f1c1c123dac08"}

[commit-bot: I haz the power, 2017-03-27 22:50:26]

Description was changed from

==========
android: Java ChildProcessLauncherHelper instance

This is mostly meant to be a no-op refactor. There is a long term need
to have ChildProcessLauncher(Helper) to have a java _instance_ to save
state, as an alternative to putting things into global maps keyed by
the pid. See bug for specific use.

So create a java ChildProcessLauncherHelper, which is instantiated and
owned by native ChildProcessLauncherHelper. Then move all jni calls
from ChildProcessLauncher (which only had static methods) to
ChildProcessLauncherHelper. Also convert the jni calls from static
calls to instance calls where possible; the major except right is
Terminate/Stop, because it's used as a static method.

Replace the opaque "clientContext" native pointer in
ChildProcessLauncher with a java callback implemented by Helper.
Helper should hold a pointer directly to native peer, but due to
threading/ownership issues, it is set back to 0 before native side is
actually deleted.

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

to

==========
android: Java ChildProcessLauncherHelper instance

This is mostly meant to be a no-op refactor. There is a long term need
to have ChildProcessLauncher(Helper) to have a java _instance_ to save
state, as an alternative to putting things into global maps keyed by
the pid. See bug for specific use.

So create a java ChildProcessLauncherHelper, which is instantiated and
owned by native ChildProcessLauncherHelper. Then move all jni calls
from ChildProcessLauncher (which only had static methods) to
ChildProcessLauncherHelper. Also convert the jni calls from static
calls to instance calls where possible; the major except right is
Terminate/Stop, because it's used as a static method.

Replace the opaque "clientContext" native pointer in
ChildProcessLauncher with a java callback implemented by Helper.
Helper should hold a pointer directly to native peer, but due to
threading/ownership issues, it is set back to 0 before native side is
actually deleted.

BUG=689758

Review-Url: https://codereview.chromium.org/2774163002
Cr-Commit-Position: refs/heads/master@{#459916}
Committed:
https://chromium.googlesource.com/chromium/src/+/4ff655334a321362a3611d0e1d0f...
==========

[commit-bot: I haz the power, 2017-03-27 22:50:27]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/4ff655334a321362a3611d0e1d0f...