Moving some CPL logic to CPLH.
In preparation of the move of some of the child process launcher logic to
base, moved the code responsible for building the parameters passed to
the service from ChildProcessLauncher to ChildProcessLauncherHelper.
The ChildSpawnData class now contains both bundles used when starting the
service, and it's the responsibility of the ChildProcessLauncherHelper to
include the command line and file descriptors in the bundles.
BUG=702316
Review-Url: https://codereview.chromium.org/2882823002
Cr-Commit-Position: refs/heads/master@{#472494}
Committed: https://chromium.googlesource.com/chromium/src/+/9cf3fd03d8f4d885b6e1c88cf3ae6ecc8d5542b0
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_android/builds/267378)
3 years, 7 months ago
(2017-05-13 05:32:58 UTC)
#4
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_swarming_rel/builds/178289)
3 years, 7 months ago
(2017-05-15 18:05:36 UTC)
#8
Description was changed from ========== Moving some CPL logic to CPLH. BUG= ========== to ========== ...
3 years, 7 months ago
(2017-05-15 19:40:04 UTC)
#11
Description was changed from
==========
Moving some CPL logic to CPLH.
BUG=
==========
to
==========
Moving some CPL logic to CPLH.
In preparation of the move of some of the child process launcher logic to
base, moved the code responsible for building the parameters passed to
the service from ChildProcessLauncher to ChildProcessLauncherHelper.
The ChildSpawnData class now contains both bundles used when starting the
service, and it's the responsibility of the ChildProcessLauncherHelper to
include the command line and file descriptors in the bundles.
BUG=702316
==========
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_swarming_rel/builds/178431)
3 years, 7 months ago
(2017-05-15 19:53:38 UTC)
#14
3 years, 7 months ago
(2017-05-15 20:12:52 UTC)
#15
Patchset #1 (id:1) has been deleted
Jay Civelli
Patchset #1 (id:20001) has been deleted
3 years, 7 months ago
(2017-05-15 20:13:04 UTC)
#16
Patchset #1 (id:20001) has been deleted
Jay Civelli
3 years, 7 months ago
(2017-05-15 20:15:28 UTC)
#17
boliu
looks like there are some test issues? https://codereview.chromium.org/2882823002/diff/40001/content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java File content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java (right): https://codereview.chromium.org/2882823002/diff/40001/content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java#newcode167 content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java:167: final Bundle ...
3 years, 7 months ago
(2017-05-15 21:38:23 UTC)
#18
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_swarming_rel/builds/178764)
3 years, 7 months ago
(2017-05-16 00:23:34 UTC)
#22
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_swarming_rel/builds/179488)
3 years, 7 months ago
(2017-05-16 18:30:18 UTC)
#26
https://codereview.chromium.org/2882823002/diff/40001/content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java File content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java (right): https://codereview.chromium.org/2882823002/diff/40001/content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java#newcode167 content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java:167: final Bundle mBundle; On 2017/05/15 21:38:22, boliu wrote: > ...
3 years, 7 months ago
(2017-05-17 16:42:50 UTC)
#29
https://codereview.chromium.org/2882823002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java
(right):
https://codereview.chromium.org/2882823002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java:167:
final Bundle mBundle;
On 2017/05/15 21:38:22, boliu wrote:
> name this mConnectionBundle
Done.
https://codereview.chromium.org/2882823002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java
(right):
https://codereview.chromium.org/2882823002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java:115:
public static ChildProcessLauncherHelper createAndStartForTesting(long
nativePointer,
On 2017/05/15 21:38:22, boliu wrote:
> nit: put test-only methods at the very end
Done.
https://codereview.chromium.org/2882823002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java:155:
try {
On 2017/05/15 21:38:22, boliu wrote:
> hmm, I guess this is the earliest in this class. But I'd still say move it to
> above the native call
Done.
https://codereview.chromium.org/2882823002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java:223:
private static ChromiumLinkerParams getLinkerParamsForNewConnection() {
On 2017/05/15 21:38:22, boliu wrote:
> static method, can we have a thread assert?
Done.
https://codereview.chromium.org/2882823002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java:273:
bundle.putBundle(Linker.EXTRA_LINKER_SHARED_RELROS,
Linker.getInstance().getSharedRelros());
On 2017/05/15 21:38:22, boliu wrote:
> I don't know this code, but does this need to be ordered with
> getLinkerParamsForNewConnection? should we assert sLinkerInitialized. Previous
> code constructed bundles in steps, and the getLinkerParamsForNewConnection
step
> always happens before setup connnection. But now this is no longer clear.
Good point. Now initializing the Linker in the constructor to make the
initialization order clearer and added asserts.
(createConnectionBundle is only used from tests outside of this class, so we
should be safe)
Jay Civelli
On 2017/05/17 16:42:50, Jay Civelli wrote: > https://codereview.chromium.org/2882823002/diff/40001/content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java > File > content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java > (right): > ...
3 years, 7 months ago
(2017-05-17 16:45:10 UTC)
#30
On 2017/05/17 16:42:50, Jay Civelli wrote:
>
https://codereview.chromium.org/2882823002/diff/40001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java
> (right):
>
>
https://codereview.chromium.org/2882823002/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java:167:
> final Bundle mBundle;
> On 2017/05/15 21:38:22, boliu wrote:
> > name this mConnectionBundle
>
> Done.
>
>
https://codereview.chromium.org/2882823002/diff/40001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java
> (right):
>
>
https://codereview.chromium.org/2882823002/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java:115:
> public static ChildProcessLauncherHelper createAndStartForTesting(long
> nativePointer,
> On 2017/05/15 21:38:22, boliu wrote:
> > nit: put test-only methods at the very end
>
> Done.
>
>
https://codereview.chromium.org/2882823002/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java:155:
> try {
> On 2017/05/15 21:38:22, boliu wrote:
> > hmm, I guess this is the earliest in this class. But I'd still say move it
to
> > above the native call
>
> Done.
>
>
https://codereview.chromium.org/2882823002/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java:223:
> private static ChromiumLinkerParams getLinkerParamsForNewConnection() {
> On 2017/05/15 21:38:22, boliu wrote:
> > static method, can we have a thread assert?
>
> Done.
>
>
https://codereview.chromium.org/2882823002/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java:273:
> bundle.putBundle(Linker.EXTRA_LINKER_SHARED_RELROS,
> Linker.getInstance().getSharedRelros());
> On 2017/05/15 21:38:22, boliu wrote:
> > I don't know this code, but does this need to be ordered with
> > getLinkerParamsForNewConnection? should we assert sLinkerInitialized.
Previous
> > code constructed bundles in steps, and the getLinkerParamsForNewConnection
> step
> > always happens before setup connnection. But now this is no longer clear.
>
> Good point. Now initializing the Linker in the constructor to make the
> initialization order clearer and added asserts.
> (createConnectionBundle is only used from tests outside of this class, so we
> should be safe)
Actually that comment is not entirely accurate anymore. It's a bit messy right
now how we initialize the linker because of the warm-up case.
I should be able to clean that up in the end once connection allocation and
warm-up is moved to ChildProcessLauncherHelper.
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
3 years, 7 months ago
(2017-05-17 16:57:18 UTC)
#31
CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495042402483590, "parent_rev": "5ba0161ded8becc9b3bd00a98250b4decb643bc5", "commit_rev": "9cf3fd03d8f4d885b6e1c88cf3ae6ecc8d5542b0"}
3 years, 7 months ago
(2017-05-17 18:09:30 UTC)
#36
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495042402483590,
"parent_rev": "5ba0161ded8becc9b3bd00a98250b4decb643bc5", "commit_rev":
"9cf3fd03d8f4d885b6e1c88cf3ae6ecc8d5542b0"}
commit-bot: I haz the power
Description was changed from ========== Moving some CPL logic to CPLH. In preparation of the ...
3 years, 7 months ago
(2017-05-17 18:09:40 UTC)
#37
Message was sent while issue was closed.
Description was changed from
==========
Moving some CPL logic to CPLH.
In preparation of the move of some of the child process launcher logic to
base, moved the code responsible for building the parameters passed to
the service from ChildProcessLauncher to ChildProcessLauncherHelper.
The ChildSpawnData class now contains both bundles used when starting the
service, and it's the responsibility of the ChildProcessLauncherHelper to
include the command line and file descriptors in the bundles.
BUG=702316
==========
to
==========
Moving some CPL logic to CPLH.
In preparation of the move of some of the child process launcher logic to
base, moved the code responsible for building the parameters passed to
the service from ChildProcessLauncher to ChildProcessLauncherHelper.
The ChildSpawnData class now contains both bundles used when starting the
service, and it's the responsibility of the ChildProcessLauncherHelper to
include the command line and file descriptors in the bundles.
BUG=702316
Review-Url: https://codereview.chromium.org/2882823002
Cr-Commit-Position: refs/heads/master@{#472494}
Committed:
https://chromium.googlesource.com/chromium/src/+/9cf3fd03d8f4d885b6e1c88cf3ae...
==========
commit-bot: I haz the power
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/chromium/src/+/9cf3fd03d8f4d885b6e1c88cf3ae6ecc8d5542b0
3 years, 7 months ago
(2017-05-17 18:09:42 UTC)
#38
Issue 2882823002: Moving some CPL logic to CPLH.
(Closed)
Created 3 years, 7 months ago by Jay Civelli
Modified 3 years, 7 months ago
Reviewers: boliu
Base URL:
Comments: 10