Added sandboxed process service.

Added sandboxed process service.


BUG=
TEST=


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

[Yaron, 2012-06-11 19:46:22]

Generally looks fine

http://codereview.chromium.org/10546079/diff/2001/content/app/android/sandbox...
File content/app/android/sandboxed_process_service.cc (right):

http://codereview.chromium.org/10546079/diff/2001/content/app/android/sandbox...
content/app/android/sandboxed_process_service.cc:69: #if
!defined(ANDROID_UPSTREAM_BRINGUP)
Please remove this. It has to refactored anyway (content knowing about chrome)
and won't land upstream like this.

http://codereview.chromium.org/10546079/diff/2001/content/content.gyp
File content/content.gyp (right):

http://codereview.chromium.org/10546079/diff/2001/content/content.gyp#newcode221
content/content.gyp:221: 'target_name': 'content_jni',
Remove this. You already have content_jni_headers below.

http://codereview.chromium.org/10546079/diff/2001/content/content.gyp#newcode226
content/content.gyp:226: 'target_name': 'common_aidl',
You shouldn't need to duplicate this. It's already in content_common and content
shoudl depend on it.

http://codereview.chromium.org/10546079/diff/2001/content/content_common.gypi
File content/content_common.gypi (right):

http://codereview.chromium.org/10546079/diff/2001/content/content_common.gypi...
content/content_common.gypi:107: 'common/android/surface_texture_peer.cc',
NIT: Sorting is a bit off

http://codereview.chromium.org/10546079/diff/2001/content/public/android/java...
File
content/public/android/java/org/chromium/content/app/SandboxedProcessService.java
(right):

http://codereview.chromium.org/10546079/diff/2001/content/public/android/java...
content/public/android/java/org/chromium/content/app/SandboxedProcessService.java:69:
mChromePakFd =
args.getParcelable(SandboxedProcessConnection.EXTRA_CHROME_PAK_FD);
Hmm. Not entirely sure how to fix this right now as it'll be changing when it's
upsstreamed. Probably best to just remove references to this for now.

[jam, 2012-06-11 20:13:17]

http://codereview.chromium.org/10546079/diff/2001/content/app/android/content...
File content/app/android/content_jni_registrar.cc (right):

http://codereview.chromium.org/10546079/diff/2001/content/app/android/content...
content/app/android/content_jni_registrar.cc:15: #include
"content/common/android/surface_callback.h"
nit: order

http://codereview.chromium.org/10546079/diff/2001/content/app/android/sandbox...
File content/app/android/sandboxed_process_service.cc (right):

http://codereview.chromium.org/10546079/diff/2001/content/app/android/sandbox...
content/app/android/sandboxed_process_service.cc:69: #if
!defined(ANDROID_UPSTREAM_BRINGUP)
On 2012/06/11 19:46:22, Yaron wrote:
> Please remove this. It has to refactored anyway (content knowing about chrome)
> and won't land upstream like this.

+1. content also doesn't know about how embedders store resources (i.e. pak
files). that's why it asks the embedder through ContentClient for any
strings/data

http://codereview.chromium.org/10546079/diff/2001/content/common/android/surf...
File content/common/android/surface_callback.h (right):

http://codereview.chromium.org/10546079/diff/2001/content/common/android/surf...
content/common/android/surface_callback.h:22: // This file implements support
for passing surface handles from Java
nit: usually there's a blank line after the namespace content before the
functions/comments

http://codereview.chromium.org/10546079/diff/2001/content/common/android/surf...
content/common/android/surface_callback.h:38:
content::SurfaceTexturePeer::SurfaceTextureTarget type,
nit: no need for content::

[michaelbai, 2012-06-11 20:30:15]

PTAL

http://codereview.chromium.org/10546079/diff/2001/content/app/android/content...
File content/app/android/content_jni_registrar.cc (right):

http://codereview.chromium.org/10546079/diff/2001/content/app/android/content...
content/app/android/content_jni_registrar.cc:15: #include
"content/common/android/surface_callback.h"
On 2012/06/11 20:13:17, John Abd-El-Malek wrote:
> nit: order

Done.

http://codereview.chromium.org/10546079/diff/2001/content/app/android/sandbox...
File content/app/android/sandboxed_process_service.cc (right):

http://codereview.chromium.org/10546079/diff/2001/content/app/android/sandbox...
content/app/android/sandboxed_process_service.cc:69: #if
!defined(ANDROID_UPSTREAM_BRINGUP)
On 2012/06/11 19:46:22, Yaron wrote:
> Please remove this. It has to refactored anyway (content knowing about chrome)
> and won't land upstream like this.

Done.

http://codereview.chromium.org/10546079/diff/2001/content/app/android/sandbox...
content/app/android/sandboxed_process_service.cc:69: #if
!defined(ANDROID_UPSTREAM_BRINGUP)
I left them here as a remainder here for somebody else try run this code.

Since we are refactorying it in downstream, it should be fine to remove them.
On 2012/06/11 20:13:17, John Abd-El-Malek wrote:
> On 2012/06/11 19:46:22, Yaron wrote:
> > Please remove this. It has to refactored anyway (content knowing about
chrome)
> > and won't land upstream like this.
> 
> +1. content also doesn't know about how embedders store resources (i.e. pak
> files). that's why it asks the embedder through ContentClient for any
> strings/data

http://codereview.chromium.org/10546079/diff/2001/content/common/android/surf...
File content/common/android/surface_callback.h (right):

http://codereview.chromium.org/10546079/diff/2001/content/common/android/surf...
content/common/android/surface_callback.h:22: // This file implements support
for passing surface handles from Java
On 2012/06/11 20:13:17, John Abd-El-Malek wrote:
> nit: usually there's a blank line after the namespace content before the
> functions/comments

Done.

http://codereview.chromium.org/10546079/diff/2001/content/common/android/surf...
content/common/android/surface_callback.h:38:
content::SurfaceTexturePeer::SurfaceTextureTarget type,
On 2012/06/11 20:13:17, John Abd-El-Malek wrote:
> nit: no need for content::

Done.

http://codereview.chromium.org/10546079/diff/2001/content/content.gyp
File content/content.gyp (right):

http://codereview.chromium.org/10546079/diff/2001/content/content.gyp#newcode221
content/content.gyp:221: 'target_name': 'content_jni',
On 2012/06/11 19:46:22, Yaron wrote:
> Remove this. You already have content_jni_headers below.

Done.

http://codereview.chromium.org/10546079/diff/2001/content/content.gyp#newcode226
content/content.gyp:226: 'target_name': 'common_aidl',
Removed from content_common, as all targets are defined here.

On 2012/06/11 19:46:22, Yaron wrote:
> You shouldn't need to duplicate this. It's already in content_common and
content
> shoudl depend on it.

http://codereview.chromium.org/10546079/diff/2001/content/content_common.gypi
File content/content_common.gypi (right):

http://codereview.chromium.org/10546079/diff/2001/content/content_common.gypi...
content/content_common.gypi:107: 'common/android/surface_texture_peer.cc',
On 2012/06/11 19:46:22, Yaron wrote:
> NIT: Sorting is a bit off

Done.

http://codereview.chromium.org/10546079/diff/2001/content/public/android/java...
File
content/public/android/java/org/chromium/content/app/SandboxedProcessService.java
(right):

http://codereview.chromium.org/10546079/diff/2001/content/public/android/java...
content/public/android/java/org/chromium/content/app/SandboxedProcessService.java:69:
mChromePakFd =
args.getParcelable(SandboxedProcessConnection.EXTRA_CHROME_PAK_FD);
On 2012/06/11 19:46:22, Yaron wrote:
> Hmm. Not entirely sure how to fix this right now as it'll be changing when
it's
> upsstreamed. Probably best to just remove references to this for now.

Done.

[Yaron, 2012-06-11 20:45:50]

http://codereview.chromium.org/10546079/diff/2001/content/public/android/java...
File
content/public/android/java/org/chromium/content/app/SandboxedProcessService.java
(right):

http://codereview.chromium.org/10546079/diff/2001/content/public/android/java...
content/public/android/java/org/chromium/content/app/SandboxedProcessService.java:69:
mChromePakFd =
args.getParcelable(SandboxedProcessConnection.EXTRA_CHROME_PAK_FD);
On 2012/06/11 20:30:16, michaelbai wrote:
> On 2012/06/11 19:46:22, Yaron wrote:
> > Hmm. Not entirely sure how to fix this right now as it'll be changing when
> it's
> > upsstreamed. Probably best to just remove references to this for now.
> 
> Done.

So mLocalePakFd is part of the same problem. Please remove it too.

http://codereview.chromium.org/10546079/diff/9001/content/app/android/sandbox...
File content/app/android/sandboxed_process_service.cc (right):

http://codereview.chromium.org/10546079/diff/9001/content/app/android/sandbox...
content/app/android/sandboxed_process_service.cc:78: jint chrome_pak_fd,
You still have chrome in here.

http://codereview.chromium.org/10546079/diff/9001/content/common/DEPS
File content/common/DEPS (right):

http://codereview.chromium.org/10546079/diff/9001/content/common/DEPS#newcode4
content/common/DEPS:4: "+jni", # For generated JNI includes
I think we can just move this up to content/DEPS. Every sub-folder is going to
have JNI bindings. jam, is that reasonable?

http://codereview.chromium.org/10546079/diff/9001/content/public/android/java...
File
content/public/android/java/org/chromium/content/app/SandboxedProcessService.java
(right):

http://codereview.chromium.org/10546079/diff/9001/content/public/android/java...
content/public/android/java/org/chromium/content/app/SandboxedProcessService.java:118:
mChromePakFd.detachFd(), mLocalePakFd.detachFd());
You're guaranteeing crashes here. Just pass NULL/0 and remove the member
variable or remove the params entirely (since you need to update the cc code
too)

[michaelbai, 2012-06-11 21:00:10]

PTAL

http://codereview.chromium.org/10546079/diff/9001/content/app/android/sandbox...
File content/app/android/sandboxed_process_service.cc (right):

http://codereview.chromium.org/10546079/diff/9001/content/app/android/sandbox...
content/app/android/sandboxed_process_service.cc:78: jint chrome_pak_fd,
On 2012/06/11 20:45:51, Yaron wrote:
> You still have chrome in here.

Removed

http://codereview.chromium.org/10546079/diff/9001/content/common/DEPS
File content/common/DEPS (right):

http://codereview.chromium.org/10546079/diff/9001/content/common/DEPS#newcode4
content/common/DEPS:4: "+jni", # For generated JNI includes
On 2012/06/11 20:45:51, Yaron wrote:
> I think we can just move this up to content/DEPS. Every sub-folder is going to
> have JNI bindings. jam, is that reasonable?

Wait for jam's comment

http://codereview.chromium.org/10546079/diff/9001/content/public/android/java...
File
content/public/android/java/org/chromium/content/app/SandboxedProcessService.java
(right):

http://codereview.chromium.org/10546079/diff/9001/content/public/android/java...
content/public/android/java/org/chromium/content/app/SandboxedProcessService.java:118:
mChromePakFd.detachFd(), mLocalePakFd.detachFd());
On 2012/06/11 20:45:51, Yaron wrote:
> You're guaranteeing crashes here. Just pass NULL/0 and remove the member
> variable or remove the params entirely (since you need to update the cc code
> too)

Removed them.

[Yaron, 2012-06-11 21:09:39]

lgtm modulo the one comment

http://codereview.chromium.org/10546079/diff/9001/content/app/android/sandbox...
File content/app/android/sandboxed_process_service.cc (right):

http://codereview.chromium.org/10546079/diff/9001/content/app/android/sandbox...
content/app/android/sandboxed_process_service.cc:68: }
Did you intentionally omit:

SurfaceTexturePeer::InitInstance(                                               
                                                             
      new SurfaceTexturePeerSandboxedImpl(service));

[michaelbai, 2012-06-11 21:21:30]

http://codereview.chromium.org/10546079/diff/9001/content/app/android/sandbox...
File content/app/android/sandboxed_process_service.cc (right):

http://codereview.chromium.org/10546079/diff/9001/content/app/android/sandbox...
content/app/android/sandboxed_process_service.cc:68: }
Yes, this code was missing.

On 2012/06/11 21:09:39, Yaron wrote:
> Did you intentionally omit:
> 
> SurfaceTexturePeer::InitInstance(                                             
 
>                                                              
>       new SurfaceTexturePeerSandboxedImpl(service));

[jam, 2012-06-11 23:51:16]

lgtm

http://codereview.chromium.org/10546079/diff/9001/content/common/DEPS
File content/common/DEPS (right):

http://codereview.chromium.org/10546079/diff/9001/content/common/DEPS#newcode4
content/common/DEPS:4: "+jni", # For generated JNI includes
On 2012/06/11 21:00:10, michaelbai wrote:
> On 2012/06/11 20:45:51, Yaron wrote:
> > I think we can just move this up to content/DEPS. Every sub-folder is going
to
> > have JNI bindings. jam, is that reasonable?
> 
> Wait for jam's comment

agreed, if you're going to have this in every subdir of content, just put it in
content/deps

[michaelbai, 2012-06-12 01:07:41]

Moved '+jni'

[commit-bot: I haz the power, 2012-06-12 01:08:06]

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

[commit-bot: I haz the power, 2012-06-12 01:21:47]

Try job failure for 10546079-10007 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-06-12 03:26:42]

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

[commit-bot: I haz the power, 2012-06-12 03:26:59]

Failed to apply patch for content/shell/android/AndroidManifest.xml:
While running patch -p1 --forward --force;
patching file content/shell/android/AndroidManifest.xml
Hunk #1 FAILED at 24.
1 out of 1 hunk FAILED -- saving rejects to file
content/shell/android/AndroidManifest.xml.rej

[commit-bot: I haz the power, 2012-06-12 03:32:56]

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

[commit-bot: I haz the power, 2012-06-12 05:00:07]

Change committed as 141618

[Ted C, 2012-06-13 16:58:34]

lgtm

[jurogan17, 2016-01-01 17:33:24]

jurogan17@gmail.com changed reviewers:
+ jurogan17@gmail.com

[jurogan17, 2016-01-01 17:33:24]

lgtm