Chromium support of running DumpRenderTree as an apk on Android

Chromium support of running DumpRenderTree as an apk on Android

This is an upstream of chromium-android.
The WebKit part is https://bugs.webkit.org/show_bug.cgi?id=86862.

TBR=darin (for base/base.gyp)
BUG=none
TEST=build and run current native tests without error


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

[Xianzhu, 2012-05-24 21:03:42]

Hi, reviewers,

Below are more descriptions of the change. Could you review the part that you
are the owner? Thanks.

- base/test (owner phajdan.jr):
  * test_stub_android.h renamed to test_support_android.h
  * test_stub_android.cc renamed to test_support_android.cc with a few changes,
most of which are about styles.
  * test_suite.cc: Changed at_exit_manager_ to use ShadowingAtExitManager
because we may wrap a whole test executable into an apk and the launcher
(testing/android/native_test_wrapper.cc) in the wrapper has its own
AtExitManager.

- testing/android/* (jrg): pure upstream from Android

- webkit/support (tkent)
  * platform_support_android.cc:
     * Style changes;
     * Hard-coded DRT directory.
  * webkit_support.cc:
     * Android version of InitLogging;
     * AtExitManager -> ShadowingExitManager because we have  an AtExitManager
in our launcher in apk wrapper (testing/android/native_test_launcher.cc).

Thanks,
Xianzhu

[John Grabowski, 2012-05-24 21:07:41]

Most testing/android LGTM

Prefer nilesh to confirm testing/android/native_test_launcher.cc changes can be
upstreamed at this time

[nilesh, 2012-05-24 21:28:32]

LGTM with minor nits.
Please try this on the android_test try bot.

http://codereview.chromium.org/10408091/diff/5001/testing/android/native_test...
File testing/android/native_test.gyp (right):

http://codereview.chromium.org/10408091/diff/5001/testing/android/native_test...
testing/android/native_test.gyp:56: 'native_test_jni_headers',
Nit: sort order.

http://codereview.chromium.org/10408091/diff/5001/webkit/support/platform_sup...
File webkit/support/platform_support_android.cc (right):

http://codereview.chromium.org/10408091/diff/5001/webkit/support/platform_sup...
webkit/support/platform_support_android.cc:29: const char kDumpRenderTreeDir[] =
"/data/drt";
We may also want to explore using "/data/local/tmp/drt/" as it will enable us to
use non-rooted devices. Worth adding to the TODO.

[Xianzhu, 2012-05-24 21:59:35]

http://codereview.chromium.org/10408091/diff/5001/testing/android/native_test...
File testing/android/native_test.gyp (right):

http://codereview.chromium.org/10408091/diff/5001/testing/android/native_test...
testing/android/native_test.gyp:56: 'native_test_jni_headers',
On 2012/05/24 21:28:33, nileshagrawal1 wrote:
> Nit: sort order.

Done.

http://codereview.chromium.org/10408091/diff/5001/webkit/support/platform_sup...
File webkit/support/platform_support_android.cc (right):

http://codereview.chromium.org/10408091/diff/5001/webkit/support/platform_sup...
webkit/support/platform_support_android.cc:29: const char kDumpRenderTreeDir[] =
"/data/drt";
On 2012/05/24 21:28:33, nileshagrawal1 wrote:
> We may also want to explore using "/data/local/tmp/drt/" as it will enable us
to
> use non-rooted devices. Worth adding to the TODO.

Done.

[tkent, 2012-05-25 01:28:51]

LGTM for webkit/support/

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

You shouldn't be using ShadowingAtExitManager, this is a serious issue.

http://codereview.chromium.org/10408091/diff/12002/base/test/test_suite.cc
File base/test/test_suite.cc (right):

http://codereview.chromium.org/10408091/diff/12002/base/test/test_suite.cc#ne...
base/test/test_suite.cc:105: at_exit_manager_.reset(new
base::ShadowingAtExitManager);
This looks Very Bad to me. You should not need that. Why are you using it here?

http://codereview.chromium.org/10408091/diff/12002/base/test/test_support_and...
File base/test/test_support_android.h (right):

http://codereview.chromium.org/10408091/diff/12002/base/test/test_support_and...
base/test/test_support_android.h:8: void InitAndroidTestLogging();
Those should have short comments above each.

http://codereview.chromium.org/10408091/diff/12002/base/test/test_support_and...
base/test/test_support_android.h:11: // Do all of the above.
Do you need to expose the sub-parts then?

http://codereview.chromium.org/10408091/diff/12002/webkit/support/platform_su...
File webkit/support/platform_support_android.cc (right):

http://codereview.chromium.org/10408091/diff/12002/webkit/support/platform_su...
webkit/support/platform_support_android.cc:38: setlocale(LC_CTYPE, "en_US");
Is this ever used in non-test code? If so, this logic should only be executed in
tests. We have a scoped object for that, please use it.

http://codereview.chromium.org/10408091/diff/12002/webkit/support/webkit_supp...
File webkit/support/webkit_support.cc (right):

http://codereview.chromium.org/10408091/diff/12002/webkit/support/webkit_supp...
webkit/support/webkit_support.cc:274: at_exit_manager = new
base::ShadowingAtExitManager;
Don't do that, it's most likely papering over some other issues.

[wangxianzhu, 2012-05-26 04:04:24]

On 2012/05/25 19:06:06, Paweł Hajdan Jr. wrote:
> You shouldn't be using ShadowingAtExitManager, this is a serious issue.

Could you explain this more? I thought ShadowingAtExitManager is almost
equivalent with AtExitManager in tests except it allows another AtExitManager
created before it, but I might have missed something.

We need that because we create an AtExitManager in JNI_OnLoad() (in
testing/android/native_test_wrapper.cc) before calling the original main().

[Paweł Hajdan Jr., 2012-05-28 07:37:13]

On 2012/05/26 04:04:24, Xianzhu Wang wrote:
> On 2012/05/25 19:06:06, Paweł Hajdan Jr. wrote:
> > You shouldn't be using ShadowingAtExitManager, this is a serious issue.
> 
> Could you explain this more? I thought ShadowingAtExitManager is almost
> equivalent with AtExitManager in tests except it allows another AtExitManager
> created before it, but I might have missed something.

That's right. And it's extremely dangerous. AtExitManager destroys Singletons
and other things. If you have multiple managers, different objects may register
their destructions on different managers, and you get a mess in the end with
wrong destruction order and objects being destroyed that other objects refer to.

It's *guaranteed* to give you crashes. Don't do that. We only need
ShadowingAtExitManager to test AtExitManager.

> We need that because we create an AtExitManager in JNI_OnLoad() (in
> testing/android/native_test_wrapper.cc) before calling the original main().

Design it so you have only one AtExitManager.

[Xianzhu, 2012-05-29 17:15:55]

http://codereview.chromium.org/10408091/diff/12002/base/test/test_suite.cc
File base/test/test_suite.cc (right):

http://codereview.chromium.org/10408091/diff/12002/base/test/test_suite.cc#ne...
base/test/test_suite.cc:105: at_exit_manager_.reset(new
base::ShadowingAtExitManager);
On 2012/05/25 19:06:08, Paweł Hajdan Jr. wrote:
> This looks Very Bad to me. You should not need that. Why are you using it
here?

Now use #if !defined(ANDROID_APK_TEST_TARGET). Will change to #if
!defined(OS_ANDROID) after we convert all unit tests to apks.

http://codereview.chromium.org/10408091/diff/12002/base/test/test_support_and...
File base/test/test_support_android.h (right):

http://codereview.chromium.org/10408091/diff/12002/base/test/test_support_and...
base/test/test_support_android.h:8: void InitAndroidTestLogging();
On 2012/05/25 19:06:08, Paweł Hajdan Jr. wrote:
> Those should have short comments above each.

Done.

http://codereview.chromium.org/10408091/diff/12002/base/test/test_support_and...
base/test/test_support_android.h:11: // Do all of the above.
On 2012/05/25 19:06:08, Paweł Hajdan Jr. wrote:
> Do you need to expose the sub-parts then?

Yes. For chromium unit tests, InitAndroidTest() is enough. For webkit unit tests
and layout tests, because in webkit_support.cc the sub-parts are initialized
separately, requiring us to provide the init functions of the sub-parts.

http://codereview.chromium.org/10408091/diff/12002/webkit/support/platform_su...
File webkit/support/platform_support_android.cc (right):

http://codereview.chromium.org/10408091/diff/12002/webkit/support/platform_su...
webkit/support/platform_support_android.cc:38: setlocale(LC_CTYPE, "en_US");
On 2012/05/25 19:06:08, Paweł Hajdan Jr. wrote:
> Is this ever used in non-test code? If so, this logic should only be executed
in
> tests. We have a scoped object for that, please use it.

This is only for layout tests. Just verified that after we switched to apk-based
tests, this is not needed.

Removed.

http://codereview.chromium.org/10408091/diff/12002/webkit/support/webkit_supp...
File webkit/support/webkit_support.cc (right):

http://codereview.chromium.org/10408091/diff/12002/webkit/support/webkit_supp...
webkit/support/webkit_support.cc:274: at_exit_manager = new
base::ShadowingAtExitManager;
On 2012/05/25 19:06:08, Paweł Hajdan Jr. wrote:
> Don't do that, it's most likely papering over some other issues.

Now use #if !defined(OS_ANDROID).

[Xianzhu, 2012-05-30 15:59:09]

Hi Pawel,

Does the latest patch set look good to you?

Thanks,
Xianzhu

[Paweł Hajdan Jr., 2012-05-30 17:15:38]

http://codereview.chromium.org/10408091/diff/21002/webkit/support/webkit_supp...
File webkit/support/webkit_support.cc (right):

http://codereview.chromium.org/10408091/diff/21002/webkit/support/webkit_supp...
webkit/support/webkit_support.cc:286: #if !defined(OS_ANDROID)
I don't see terminating #endif for this - is that intended?

[Xianzhu, 2012-05-30 18:17:56]

http://codereview.chromium.org/10408091/diff/21002/webkit/support/webkit_supp...
File webkit/support/webkit_support.cc (right):

http://codereview.chromium.org/10408091/diff/21002/webkit/support/webkit_supp...
webkit/support/webkit_support.cc:286: #if !defined(OS_ANDROID)
On 2012/05/30 17:15:38, Paweł Hajdan Jr. wrote:
> I don't see terminating #endif for this - is that intended?

Sorry I messed up the code when managing downstream/upstream versions. Fixed.

[Xianzhu, 2012-05-31 22:25:19]

Paweł, could you have a look at the latest patch set. Thanks!

[Paweł Hajdan Jr., 2012-06-01 10:18:29]

LGTM

[commit-bot: I haz the power, 2012-06-01 15:57:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/10408091/13005

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

Change committed as 140046