Move WebApkSandboxedProcessService to webapk/shell_apk

Move WebApkSandboxedProcessService to webapk/shell_apk

BUG=None

Committed: https://crrev.com/9348eeca076a7c4b57833720efad8ca7f6897971
Cr-Commit-Position: refs/heads/master@{#403330}

[pkotwicz, 2016-06-27 21:56:25]

pkotwicz@chromium.org changed reviewers:
+ hanxi@chromium.org

[pkotwicz, 2016-06-27 21:56:25]

Xi can you please take a look?

As a result of having the org.chromium.content.browser.SANDBOXED_SERVICES_NAME
meta-data property we no longer need to have WebApkSandboxedProcessService in
webapk/libs/common

[Xi Han, 2016-06-28 15:35:06]

It sounds great, but since chrome_apk doesn't depend on shell_apk:webapk target,
are you sure the following code:
Class<? extends Service> service = (Class<? extends Service>)
Class.forName(serviceName);
(https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro...)
doesn't throw runtime ClassNotFoundException?

[pkotwicz, 2016-06-28 17:19:27]

Xi can you please take another look? I have changed ChildProcessLauncher so that
it does not throw a ClassNotFoundException.

If we decide to keep WebApkSandboxedProcessService in webapk/libs/common we will
need to change proguard.flags so that proguard does not remove it from
chrome_apk (I have not checked but I think that proguard would remove it from
chrome_apk)

https://codereview.chromium.org/2107573002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/2107573002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:301:
if (serviceName != null) {
I am not sure if this is useful or not

[pkotwicz, 2016-06-28 17:19:34]

Patchset #2 (id:20001) has been deleted

[Xi Han, 2016-06-28 17:36:31]

https://codereview.chromium.org/2107573002/diff/40001/chrome/android/webapk/s...
File
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkSandboxedProcessService.java
(right):

https://codereview.chromium.org/2107573002/diff/40001/chrome/android/webapk/s...
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkSandboxedProcessService.java:34:
Log.e("ABCD", "WebApkSandboxedService onCreate()");
Remove the debug log.

https://codereview.chromium.org/2107573002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/2107573002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:302:
// Check that the service exists.
I think we can remove this block and just return the |serviceName|.

[pkotwicz, 2016-06-28 18:09:51]

Xi can you please take another look?

[Xi Han, 2016-06-28 18:11:08]

lgtm

[pkotwicz, 2016-06-28 18:12:44]

pkotwicz@chromium.org changed reviewers:
+ mariakhomenko@chromium.org

[pkotwicz, 2016-06-28 18:12:47]

mariakhomenko@ can you please take a look?

Moving WebApkSandboxedProcessService into chrome/android/webapk/shell_apk makes
things cleaner since chrome_apk does not use WebApkSandboxedProcessService
directly

[Maria, 2016-06-28 18:24:59]

lgtm

[pkotwicz, 2016-06-28 18:30:41]

pkotwicz@chromium.org changed reviewers:
+ sievers@chromium.org

[pkotwicz, 2016-06-28 18:30:42]

Adding sievers@ who may also have some opinions

[no sievers, 2016-06-28 22:50:39]

https://codereview.chromium.org/2107573002/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(left):

https://codereview.chromium.org/2107573002/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:304:
(Class<? extends Service>) Class.forName(serviceName);
Isn't trying this useful though incl. the exception below?
I.e. if you give a name it better find a valid class for it.

[pkotwicz, 2016-06-28 23:48:27]

https://codereview.chromium.org/2107573002/diff/80001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(left):

https://codereview.chromium.org/2107573002/diff/80001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:304:
(Class<? extends Service>) Class.forName(serviceName);
In patch set #2, I do some validation in ChildProcessLauncher. Do you prefer
that?

[no sievers, 2016-06-29 18:10:50]

On 2016/06/28 23:48:27, pkotwicz wrote:
>
https://codereview.chromium.org/2107573002/diff/80001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
> (left):
> 
>
https://codereview.chromium.org/2107573002/diff/80001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:304:
> (Class<? extends Service>) Class.forName(serviceName);
> In patch set #2, I do some validation in ChildProcessLauncher. Do you prefer
> that?

yea that lgtm.

I think it's worth checking since we silently fall back to
'SandboxedProcessService' which is almost certainly not the intention if
somebody specified a class string.

[pkotwicz, 2016-06-29 23:36:32]

sievers@ can you please take another look?

I brought back the validation from patch set #2

[no sievers, 2016-06-29 23:47:11]

On 2016/06/29 23:36:32, pkotwicz wrote:
> sievers@ can you please take another look?
> 
> I brought back the validation from patch set #2

lgtm thanks

[pkotwicz, 2016-06-30 00:11:31]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-06-30 00:11:31]

The patchset sent to the CQ was uploaded after l-g-t-m from hanxi@chromium.org,
mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2107573002/#ps100001
(title: "Merge branch 'master' into sandbox")

[commit-bot: I haz the power, 2016-06-30 00:13:07]

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, 2016-06-30 00:57:51]

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

[commit-bot: I haz the power, 2016-06-30 00:57:51]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pkotwicz, 2016-06-30 20:37:51]

Patchset #6 (id:120001) has been deleted

[pkotwicz, 2016-06-30 20:40:07]

sievers@ can you please take another look?

I skip the check whether a service class name corresponds to a real service when
SWITCH_SANDBOXED_SERVICES_NAME_FOR_TESTING is passed in for the sake of making
tests pass

[Xi Han, 2016-06-30 20:55:12]

lgtm again.

[no sievers, 2016-06-30 21:51:45]

On 2016/06/30 20:40:07, pkotwicz wrote:
> sievers@ can you please take another look?
> 
> I skip the check whether a service class name corresponds to a real service
when
> SWITCH_SANDBOXED_SERVICES_NAME_FOR_TESTING is passed in for the sake of making
> tests pass

lgtm

[pkotwicz, 2016-06-30 22:12:28]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-06-30 22:12:28]

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/2107573002/#ps140001
(title: "Merge branch 'master' into sandbox")

[commit-bot: I haz the power, 2016-06-30 22:13:13]

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, 2016-06-30 23:18:28]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-06-30 23:23:19]

Description was changed from

==========
Move WebApkSandboxedProcessService to webapk/shell_apk

BUG=None
==========

to

==========
Move WebApkSandboxedProcessService to webapk/shell_apk

BUG=None

Committed: https://crrev.com/9348eeca076a7c4b57833720efad8ca7f6897971
Cr-Commit-Position: refs/heads/master@{#403330}
==========

[commit-bot: I haz the power, 2016-06-30 23:23:20]

Patchset 6 (id:??) landed as
https://crrev.com/9348eeca076a7c4b57833720efad8ca7f6897971
Cr-Commit-Position: refs/heads/master@{#403330}