Provide android specific implementation for MultiProcessTest::SpawnChildImpl

Provide android specific implementation for MultiProcessTest::SpawnChildImpl

When tests are run in an APK, we do not have an executable to exec.
We resort to forking and executing the test method directly.

BUG=125059
TEST=


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

[Yaron, 2012-05-23 16:42:01]

http://codereview.chromium.org/10408081/diff/4001/base/test/multiprocess_test.cc
File base/test/multiprocess_test.cc (right):

http://codereview.chromium.org/10408081/diff/4001/base/test/multiprocess_test...
base/test/multiprocess_test.cc:39: #if !defined(OS_ANDROID)
I forget... isn't this style frowned upon (removing a single function and adding
android-specific file implementation).

[nilesh, 2012-05-24 00:48:54]

On 2012/05/23 16:42:01, Yaron wrote:
>
http://codereview.chromium.org/10408081/diff/4001/base/test/multiprocess_test.cc
> File base/test/multiprocess_test.cc (right):
> 
>
http://codereview.chromium.org/10408081/diff/4001/base/test/multiprocess_test...
> base/test/multiprocess_test.cc:39: #if !defined(OS_ANDROID)
> I forget... isn't this style frowned upon (removing a single function and
adding
> android-specific file implementation).

I don't think so. I was pointed to
http://dev.chromium.org/developers/design-documents/conventions-and-patterns-...
in one of my CLs. 

I believe the android implementation is quite different (not using
base::LaunchProcess)

[Yaron, 2012-05-24 01:09:11]

ok, lg but you need a proper OWNER

[nilesh, 2012-05-25 16:48:05]

Hi Mark, 
Can you please take a look.
Thanks.

[Mark Mentovai, 2012-05-25 16:48:58]

LGTM for base OWNERS approval.

[Paweł Hajdan Jr., 2012-05-25 19:00:35]

I'm not sure if that's the cleanest thing we can do.

Do we ever use multi-process tests that exec a different binary than the current
one? If not, why don't we always fork(), and do the debug_on_start logic in the
child (feel free to skip that on Android if it causes some trouble).

[nilesh, 2012-05-25 19:33:14]

On 2012/05/25 19:00:35, Paweł Hajdan Jr. wrote:
> I'm not sure if that's the cleanest thing we can do.
> 
> Do we ever use multi-process tests that exec a different binary than the
current
> one?

No, MakeCmdLine uses the current binary.

 If not, why don't we always fork(), and do the debug_on_start logic in the
> child (feel free to skip that on Android if it causes some trouble).

I am not sure I understand this suggestion. For non-android platforms the
behavior has not changed (base::LaunchProcess forks the process and execs the
current binary).

[Paweł Hajdan Jr., 2012-05-25 19:51:08]

On 2012/05/25 19:33:14, nileshagrawal1 wrote:
> On 2012/05/25 19:00:35, Paweł Hajdan Jr. wrote:
> > I'm not sure if that's the cleanest thing we can do.
> > 
> > Do we ever use multi-process tests that exec a different binary than the
> current
> > one?
> 
> No, MakeCmdLine uses the current binary.

Good.

>  If not, why don't we always fork(), and do the debug_on_start logic in the
> > child (feel free to skip that on Android if it causes some trouble).
> 
> I am not sure I understand this suggestion. For non-android platforms the
> behavior has not changed (base::LaunchProcess forks the process and execs the
> current binary).

I'd like you to change the behavior on non-android platforms, so that we avoid
too many differences between android and non-android.

[nilesh, 2012-05-25 20:51:05]

On 2012/05/25 19:51:08, Paweł Hajdan Jr. wrote:
> On 2012/05/25 19:33:14, nileshagrawal1 wrote:
> > On 2012/05/25 19:00:35, Paweł Hajdan Jr. wrote:
> > > I'm not sure if that's the cleanest thing we can do.
> > > 
> > > Do we ever use multi-process tests that exec a different binary than the
> > current
> > > one?
> > 
> > No, MakeCmdLine uses the current binary.
> 
> Good.
> 
> >  If not, why don't we always fork(), and do the debug_on_start logic in the
> > > child (feel free to skip that on Android if it causes some trouble).
> > 
> > I am not sure I understand this suggestion. For non-android platforms the
> > behavior has not changed (base::LaunchProcess forks the process and execs
the
> > current binary).
> 
> I'd like you to change the behavior on non-android platforms, so that we avoid
> too many differences between android and non-android.

Well, to get debug_on_start to work on other platforms, we need to call exec
(and we are using the code in base::LaunchProcess to do that). I would not want
to restrict the functionality on other platforms because of restrictions we have
on android.

[Paweł Hajdan Jr., 2012-05-28 07:34:38]

On 2012/05/25 20:51:05, nileshagrawal1 wrote:
> Well, to get debug_on_start to work on other platforms, we need to call exec
> (and we are using the code in base::LaunchProcess to do that).

Not really. How about executing logic triggered by switches::kDebugOnStart in
the forked child? So far I see nothing that would prevent doing that.

> I would not want
> to restrict the functionality on other platforms because of restrictions we
have
> on android.

True, but we don't really use some "functionality" anyway, and having the code
diverge too much is not good either. If everyone runs the same version the risk
of accidentally breaking Android is much lower.

[nilesh, 2012-05-29 05:52:39]

On 2012/05/28 07:34:38, Paweł Hajdan Jr. wrote:
> On 2012/05/25 20:51:05, nileshagrawal1 wrote:
> > Well, to get debug_on_start to work on other platforms, we need to call exec
> > (and we are using the code in base::LaunchProcess to do that).
> 
> Not really. How about executing logic triggered by switches::kDebugOnStart in
> the forked child? So far I see nothing that would prevent doing that.

I would need to duplicate some code (from base::LaunchProcess) to successfully
create a child process here on all platforms. The current version works on
Android but I dont think it will work on windows.

> 
> > I would not want
> > to restrict the functionality on other platforms because of restrictions we
> have
> > on android.
> 
> True, but we don't really use some "functionality" anyway, and having the code
> diverge too much is not good either. If everyone runs the same version the
risk
> of accidentally breaking Android is much lower.

This would mean diverging test and non-test code for other non-Android
platforms. Android does not use base::LaunchProcess in non-test code too while
other platforms do. I thinks its better to use the same code path in test code
for such platforms.

[Paweł Hajdan Jr., 2012-05-29 08:32:12]

LGTM with nits.

Sorry I forgot we don't have fork() on Windows. The duplication would be
solvable by extracting a helper function I think, but lack of fork() breaks it
all.

http://codereview.chromium.org/10408081/diff/11002/base/test/multiprocess_tes...
File base/test/multiprocess_test_android.cc (right):

http://codereview.chromium.org/10408081/diff/11002/base/test/multiprocess_tes...
base/test/multiprocess_test_android.cc:30: // Child process
nit: Dot at the end:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Punctu...

http://codereview.chromium.org/10408081/diff/11002/base/test/multiprocess_tes...
base/test/multiprocess_test_android.cc:31: int ret =
multi_process_function_list::InvokeChildProcessTest(procname);
nit: No need for temporary int ret, why not just _exit(multi_process_...) ?

http://codereview.chromium.org/10408081/diff/11002/base/test/multiprocess_tes...
base/test/multiprocess_test_android.cc:35: // Parent process
nit: Dot at the end.

[nilesh, 2012-05-29 16:55:03]

Thanks for the review.

http://codereview.chromium.org/10408081/diff/11002/base/test/multiprocess_tes...
File base/test/multiprocess_test_android.cc (right):

http://codereview.chromium.org/10408081/diff/11002/base/test/multiprocess_tes...
base/test/multiprocess_test_android.cc:30: // Child process
On 2012/05/29 08:32:12, Paweł Hajdan Jr. wrote:
> nit: Dot at the end:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Punctu...

Done.

http://codereview.chromium.org/10408081/diff/11002/base/test/multiprocess_tes...
base/test/multiprocess_test_android.cc:31: int ret =
multi_process_function_list::InvokeChildProcessTest(procname);
On 2012/05/29 08:32:12, Paweł Hajdan Jr. wrote:
> nit: No need for temporary int ret, why not just _exit(multi_process_...) ?

Done.

http://codereview.chromium.org/10408081/diff/11002/base/test/multiprocess_tes...
base/test/multiprocess_test_android.cc:35: // Parent process
On 2012/05/29 08:32:12, Paweł Hajdan Jr. wrote:
> nit: Dot at the end.

Done.

[commit-bot: I haz the power, 2012-05-29 16:59:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nileshagrawal@chromium.org/10408081/20001

[commit-bot: I haz the power, 2012-05-29 19:36:35]

Change committed as 139342