Added sandboxed process launcher

Added sandboxed process launcher


BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141764

[jam, 2012-06-12 17:24:11]

http://codereview.chromium.org/10541110/diff/1/content/app/android/content_jn...
File content/app/android/content_jni_registrar.cc (right):

http://codereview.chromium.org/10541110/diff/1/content/app/android/content_jn...
content/app/android/content_jni_registrar.cc:17: #include
"content/common/android/surface_callback.h"
nit: order

http://codereview.chromium.org/10541110/diff/1/content/browser/android/sandbo...
File content/browser/android/sandboxed_process_launcher.cc (right):

http://codereview.chromium.org/10541110/diff/1/content/browser/android/sandbo...
content/browser/android/sandboxed_process_launcher.cc:20:
reinterpret_cast<content::SandboxedProcessClient*>(
nit: indentation si off

http://codereview.chromium.org/10541110/diff/1/content/browser/android/sandbo...
File content/browser/android/sandboxed_process_launcher.h (right):

http://codereview.chromium.org/10541110/diff/1/content/browser/android/sandbo...
content/browser/android/sandboxed_process_launcher.h:16: class
SandboxedProcessClient {
nit: now that we have the powerful bind callbacks, we avoid callback interfaces
with one function. just make this be a callback parameter to
StartSandboxedProcess

http://codereview.chromium.org/10541110/diff/1/content/browser/android/sandbo...
content/browser/android/sandboxed_process_launcher.h:20: virtual void
OnSandboxedProcessStarted(base::ProcessHandle handle) = 0;
nit: since you say "process id" above, you probably want this to be ProcessId
(which is typedef's to pid just like ProcessHandle on posix)

http://codereview.chromium.org/10541110/diff/1/content/browser/android/sandbo...
content/browser/android/sandboxed_process_launcher.h:33: void
CancelStartSandboxedProcess(
nit: add comment

[Yaron, 2012-06-12 17:39:35]

lgtm aside from jam's comments. Please be sure to re-downstream once you land
this

http://codereview.chromium.org/10541110/diff/1/content/public/android/java/or...
File
content/public/android/java/org/chromium/content/browser/SandboxedProcessConnection.java
(left):

http://codereview.chromium.org/10541110/diff/1/content/public/android/java/or...
content/public/android/java/org/chromium/content/browser/SandboxedProcessConnection.java:316:
ParcelFileDescriptor.fromFd(mConnectionParams.mChromePakFd);
I assume this just need to be rebased on top of your previous change

[michaelbai, 2012-06-12 19:44:26]

PTAL

http://codereview.chromium.org/10541110/diff/1/content/app/android/content_jn...
File content/app/android/content_jni_registrar.cc (right):

http://codereview.chromium.org/10541110/diff/1/content/app/android/content_jn...
content/app/android/content_jni_registrar.cc:17: #include
"content/common/android/surface_callback.h"
On 2012/06/12 17:24:11, John Abd-El-Malek wrote:
> nit: order

Done.

http://codereview.chromium.org/10541110/diff/1/content/browser/android/sandbo...
File content/browser/android/sandboxed_process_launcher.cc (right):

http://codereview.chromium.org/10541110/diff/1/content/browser/android/sandbo...
content/browser/android/sandboxed_process_launcher.cc:20:
reinterpret_cast<content::SandboxedProcessClient*>(
On 2012/06/12 17:24:11, John Abd-El-Malek wrote:
> nit: indentation si off

Done.

http://codereview.chromium.org/10541110/diff/1/content/browser/android/sandbo...
File content/browser/android/sandboxed_process_launcher.h (right):

http://codereview.chromium.org/10541110/diff/1/content/browser/android/sandbo...
content/browser/android/sandboxed_process_launcher.h:16: class
SandboxedProcessClient {
On 2012/06/12 17:24:11, John Abd-El-Malek wrote:
> nit: now that we have the powerful bind callbacks, we avoid callback
interfaces
> with one function. just make this be a callback parameter to
> StartSandboxedProcess

Done.

http://codereview.chromium.org/10541110/diff/1/content/browser/android/sandbo...
content/browser/android/sandboxed_process_launcher.h:20: virtual void
OnSandboxedProcessStarted(base::ProcessHandle handle) = 0;
On 2012/06/12 17:24:11, John Abd-El-Malek wrote:
> nit: since you say "process id" above, you probably want this to be ProcessId
> (which is typedef's to pid just like ProcessHandle on posix)

Done.

http://codereview.chromium.org/10541110/diff/1/content/browser/android/sandbo...
content/browser/android/sandboxed_process_launcher.h:33: void
CancelStartSandboxedProcess(
On 2012/06/12 17:24:11, John Abd-El-Malek wrote:
> nit: add comment

Done.

http://codereview.chromium.org/10541110/diff/1/content/public/android/java/or...
File
content/public/android/java/org/chromium/content/browser/SandboxedProcessConnection.java
(left):

http://codereview.chromium.org/10541110/diff/1/content/public/android/java/or...
content/public/android/java/org/chromium/content/browser/SandboxedProcessConnection.java:316:
ParcelFileDescriptor.fromFd(mConnectionParams.mChromePakFd);
On 2012/06/12 17:39:35, Yaron wrote:
> I assume this just need to be rebased on top of your previous change

Sorry, this part was still in my previous CL.

[jam, 2012-06-12 20:44:45]

lgtm with the one change below

http://codereview.chromium.org/10541110/diff/3002/content/browser/android/san...
File content/browser/android/sandboxed_process_launcher.h (right):

http://codereview.chromium.org/10541110/diff/3002/content/browser/android/san...
content/browser/android/sandboxed_process_launcher.h:17: typedef
base::ProcessHandle ProcessID;
don't typedef this, just use base::ProcessId

[commit-bot: I haz the power, 2012-06-12 20:58:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/10541110/10

[commit-bot: I haz the power, 2012-06-12 22:49:41]

Change committed as 141764

[jam, 2012-06-13 16:18:12]

http://codereview.chromium.org/10541110/diff/10/content/browser/android/sandb...
File content/browser/android/sandboxed_process_launcher.h (right):

http://codereview.chromium.org/10541110/diff/10/content/browser/android/sandb...
content/browser/android/sandboxed_process_launcher.h:17: typedef
base::Callback<void(base::ProcessHandle)> StartSandboxedProcessCallback;
why are you using ProcessHandle? I've been trying to get you to use
base::ProcessId?

[michaelbai, 2012-06-13 16:23:30]

http://codereview.chromium.org/10541110/diff/10/content/browser/android/sandb...
File content/browser/android/sandboxed_process_launcher.h (right):

http://codereview.chromium.org/10541110/diff/10/content/browser/android/sandb...
content/browser/android/sandboxed_process_launcher.h:17: typedef
base::Callback<void(base::ProcessHandle)> StartSandboxedProcessCallback;
I misunderstood your comment, and will have a another CL to fix this.

Thanks for look it again.


On 2012/06/13 16:18:12, John Abd-El-Malek wrote:
> why are you using ProcessHandle? I've been trying to get you to use
> base::ProcessId?