Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(156)

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
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, Peter Beverloo, jam, darin-cc_chromium.org, agrieve+watch_chromium.org, jochen+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 : Moving some CPL logic to CPLH. #

Total comments: 10

Patch Set 2 : Addressed boliu@'s comments. #

Patch Set 3 : Fixed warm-up and tests. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+334 lines, -272 lines) Patch
M content/browser/child_process_launcher_helper_android.cc View 1 chunk +1 line, -1 line 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/ChildConnectionAllocator.java View 4 chunks +6 lines, -10 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java View 1 8 chunks +22 lines, -36 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java View 1 10 chunks +31 lines, -138 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java View 1 2 4 chunks +180 lines, -8 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/ChildSpawnData.java View 2 chunks +10 lines, -10 lines 0 comments Download
M content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherIntegrationTest.java View 1 chunk +2 lines, -3 lines 0 comments Download
M content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java View 1 2 9 chunks +55 lines, -53 lines 0 comments Download
M content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestHelperService.java View 2 chunks +11 lines, -7 lines 0 comments Download
M content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestUtils.java View 2 chunks +16 lines, -6 lines 0 comments Download

Messages

Total messages: 38 (31 generated)
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
Jay Civelli
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
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
boliu
lgtm
3 years, 7 months ago (2017-05-17 17:20:11 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2882823002/80001
3 years, 7 months ago (2017-05-17 17:34:33 UTC) #35
commit-bot: I haz the power
3 years, 7 months ago (2017-05-17 18:09:42 UTC) #38
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/9cf3fd03d8f4d885b6e1c88cf3ae...

Powered by Google App Engine
This is Rietveld 408576698