Add initial Chromium TestShell support for Android

Add initial Chromium TestShell support for Android

Add a build target and tie it into android_all.  This will build and link
Chrome code for a test apk for Android.  When it runs it properly
loads the library but crashes.  Will fix the crashes in subsequent
patches, but can fork that work after this is in.

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

[David Trainor- moved to gerrit, 2012-09-07 20:04:47]

PTAL :)

[Yaron, 2012-09-07 20:53:43]

Didn't get through it all yet. Just quick comments

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/android/test...
File chrome/android/testshell/chrome_main_delegate_testshell_android.cc (right):

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/android/test...
chrome/android/testshell/chrome_main_delegate_testshell_android.cc:22: // Add
extra JNI methods if necessary here.
Are there really going to be others? I think you can just do:
return ChromeMainDelegateAndroid::RegisterApplicationNativeMethods(env);

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/android/test...
File chrome/android/testshell/chrome_main_delegate_testshell_android.h (right):

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/android/test...
chrome/android/testshell/chrome_main_delegate_testshell_android.h:16: };
Add DISALLOW_COPY_AND_ASSIGN(ChromeMainDelegateTestShellAndroid);

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/android/test...
File chrome/android/testshell/java/AndroidManifest.xml (right):

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/android/test...
chrome/android/testshell/java/AndroidManifest.xml:57: <uses-sdk
android:minSdkVersion="16" android:targetSdkVersion="16" />
minSdkVersion is 14, right?

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/android/test...
File chrome/android/testshell/java/chromium_testshell_apk.xml (right):

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/android/test...
chrome/android/testshell/java/chromium_testshell_apk.xml:3: Copyright (c) 2012
The Chromium Authors. All rights reserved.
You should get Shashi or Chris to review this.

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/android/test...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java
(right):

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/android/test...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:11:
import org.chromium.content.app.AppResource;
unused

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/app/android/...
File chrome/app/android/chrome_main_delegate_android.cc (right):

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/app/android/...
chrome/app/android/chrome_main_delegate_android.cc:58: return true;
I think you should move chrome::android::RegisterJni(env); here (instead of it
being in ChromeBrowserMainPartsAndroid::PreEarlyInitialization). Does that work?

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/app/android/...
File chrome/app/android/chrome_main_delegate_android.h (right):

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/app/android/...
chrome/app/android/chrome_main_delegate_android.h:28:
scoped_ptr<content::BrowserMainRunner> browser_runner_;
DISALLOW_COPY..

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/chrome.gyp#n...
chrome/chrome.gyp:140: 'chrome_android.gypi',
This should probably be conditionally included for android.

[Torne, 2012-09-07 22:12:55]

Second yaron's comments, but generally looks reasonable and comparable to the
stuff I've done for webview. Some of it could potentially be reused but it's
only small bits, and I will look at that later rather than us doing it now.

[jam, 2012-09-07 22:40:26]

which parts do you want me to look at?

I'm heavily overloaded with content reviews, so if you can find another reviewer
for chrome, that's better.

https://codereview.chromium.org/10918113/diff/2001/chrome/app/chrome_main_del...
File chrome/app/chrome_main_delegate.h (right):

https://codereview.chromium.org/10918113/diff/2001/chrome/app/chrome_main_del...
chrome/app/chrome_main_delegate.h:28: virtual void PreSandboxStartup() OVERRIDE;
splitting up these overridden functions makes readability harder. keep them
together, even if not all of them need to be protected

[David Trainor- moved to gerrit, 2012-09-10 18:09:13]

Yaron: Addressed your changes.  That's a good point about the minSdkVersion. 
I'll look into it and make the change.  I think we do want 14.

Shashi: Can you review the ant file chromium_testshell_apk.xml?

Scott: Do you mind looking at chrome and chrome/app changes?

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/android/test...
File chrome/android/testshell/chrome_main_delegate_testshell_android.cc (right):

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/android/test...
chrome/android/testshell/chrome_main_delegate_testshell_android.cc:22: // Add
extra JNI methods if necessary here.
For the rendering and tab components I already have two more in a CL downstream.
 The tab component will eventually be rolled into the common lib but the
rendering might not.  I can remove this comment and just add them when I get to
them though.

On 2012/09/07 20:53:43, Yaron wrote:
> Are there really going to be others? I think you can just do:
> return ChromeMainDelegateAndroid::RegisterApplicationNativeMethods(env);

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/android/test...
File chrome/android/testshell/chrome_main_delegate_testshell_android.h (right):

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/android/test...
chrome/android/testshell/chrome_main_delegate_testshell_android.h:16: };
On 2012/09/07 20:53:43, Yaron wrote:
> Add DISALLOW_COPY_AND_ASSIGN(ChromeMainDelegateTestShellAndroid);

Done.

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/android/test...
File chrome/android/testshell/java/AndroidManifest.xml (right):

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/android/test...
chrome/android/testshell/java/AndroidManifest.xml:57: <uses-sdk
android:minSdkVersion="16" android:targetSdkVersion="16" />
I'll talk to Grace.  She mentioned I should be using 16 in an earlier CL.

On 2012/09/07 20:53:43, Yaron wrote:
> minSdkVersion is 14, right?

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/android/test...
File chrome/android/testshell/java/chromium_testshell_apk.xml (right):

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/android/test...
chrome/android/testshell/java/chromium_testshell_apk.xml:3: Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2012/09/07 20:53:43, Yaron wrote:
> You should get Shashi or Chris to review this.

Done.

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/android/test...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java
(right):

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/android/test...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:11:
import org.chromium.content.app.AppResource;
On 2012/09/07 20:53:43, Yaron wrote:
> unused

Done.

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/app/android/...
File chrome/app/android/chrome_main_delegate_android.cc (right):

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/app/android/...
chrome/app/android/chrome_main_delegate_android.cc:58: return true;
On 2012/09/07 20:53:43, Yaron wrote:
> I think you should move chrome::android::RegisterJni(env); here (instead of it
> being in ChromeBrowserMainPartsAndroid::PreEarlyInitialization). Does that
work?

Yeah that should work.  It looks like the registration would still be called
before the rest of PreEarlyInitialization which is fine.

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/app/android/...
File chrome/app/android/chrome_main_delegate_android.h (right):

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/app/android/...
chrome/app/android/chrome_main_delegate_android.h:28:
scoped_ptr<content::BrowserMainRunner> browser_runner_;
On 2012/09/07 20:53:43, Yaron wrote:
> DISALLOW_COPY..

Done.

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/app/chrome_m...
File chrome/app/chrome_main_delegate.h (right):

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/app/chrome_m...
chrome/app/chrome_main_delegate.h:28: virtual void PreSandboxStartup() OVERRIDE;
On 2012/09/07 22:40:26, John Abd-El-Malek wrote:
> splitting up these overridden functions makes readability harder. keep them
> together, even if not all of them need to be protected

Done.

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://chromiumcodereview.appspot.com/10918113/diff/2001/chrome/chrome.gyp#n...
chrome/chrome.gyp:140: 'chrome_android.gypi',
On 2012/09/07 20:53:43, Yaron wrote:
> This should probably be conditionally included for android.

Done.

[shashi, 2012-09-10 18:37:28]

only looked at ant changes: lgtm

https://chromiumcodereview.appspot.com/10918113/diff/23/chrome/android/testsh...
File chrome/android/testshell/java/chromium_testshell_apk.xml (right):

https://chromiumcodereview.appspot.com/10918113/diff/23/chrome/android/testsh...
chrome/android/testshell/java/chromium_testshell_apk.xml:15: <script
language="javascript">
configuration.name, is this needed?

https://chromiumcodereview.appspot.com/10918113/diff/23/chrome/android/testsh...
chrome/android/testshell/java/chromium_testshell_apk.xml:30: <path
id="native.libs.gdbserver">
Nit: Not needed, it is not used anywhere.

https://chromiumcodereview.appspot.com/10918113/diff/23/chrome/chrome_android...
File chrome/chrome_android.gypi (right):

https://chromiumcodereview.appspot.com/10918113/diff/23/chrome/chrome_android...
chrome/chrome_android.gypi:64: '<!@(find android/testshell/java/src -name
"*.xml")',
android/testshell/java/res -name '*.png'
android/testshell/java/res -name '*.xml'

[jam, 2012-09-10 20:05:13]

https://codereview.chromium.org/10918113/diff/23/chrome/app/chrome_main_deleg...
File chrome/app/chrome_main_delegate.h (right):

https://codereview.chromium.org/10918113/diff/23/chrome/app/chrome_main_deleg...
chrome/app/chrome_main_delegate.h:20: protected:
nit: put this before the comment

[David Trainor- moved to gerrit, 2012-09-10 21:26:16]

https://codereview.chromium.org/10918113/diff/23/chrome/android/testshell/jav...
File chrome/android/testshell/java/chromium_testshell_apk.xml (right):

https://codereview.chromium.org/10918113/diff/23/chrome/android/testshell/jav...
chrome/android/testshell/java/chromium_testshell_apk.xml:15: <script
language="javascript">
On 2012/09/10 18:37:29, shashi wrote:
> configuration.name, is this needed?

Done.

https://codereview.chromium.org/10918113/diff/23/chrome/android/testshell/jav...
chrome/android/testshell/java/chromium_testshell_apk.xml:30: <path
id="native.libs.gdbserver">
Ah I forgot to add the component that actually adds the gdbserver.  Added that
from content_shell_apk.xml.

On 2012/09/10 18:37:29, shashi wrote:
> Nit: Not needed, it is not used anywhere.

https://codereview.chromium.org/10918113/diff/23/chrome/app/chrome_main_deleg...
File chrome/app/chrome_main_delegate.h (right):

https://codereview.chromium.org/10918113/diff/23/chrome/app/chrome_main_deleg...
chrome/app/chrome_main_delegate.h:20: protected:
On 2012/09/10 20:05:13, John Abd-El-Malek wrote:
> nit: put this before the comment

Done.

https://codereview.chromium.org/10918113/diff/23/chrome/chrome_android.gypi
File chrome/chrome_android.gypi (right):

https://codereview.chromium.org/10918113/diff/23/chrome/chrome_android.gypi#n...
chrome/chrome_android.gypi:64: '<!@(find android/testshell/java/src -name
"*.xml")',
Good catch thanks!

On 2012/09/10 18:37:29, shashi wrote:
> android/testshell/java/res -name '*.png'
> android/testshell/java/res -name '*.xml'

[sky, 2012-09-10 21:26:16]

LGTM

https://codereview.chromium.org/10918113/diff/13001/chrome/app/android/chrome...
File chrome/app/android/chrome_main_delegate_android.h (right):

https://codereview.chromium.org/10918113/diff/13001/chrome/app/android/chrome...
chrome/app/android/chrome_main_delegate_android.h:17: virtual bool
RegisterApplicationNativeMethods(JNIEnv* env);
Add a description.

[Yaron, 2012-09-11 00:01:28]

lgtm

Feel free to land if changes are straightforward.

https://codereview.chromium.org/10918113/diff/24/chrome/android/testshell/chr...
File chrome/android/testshell/chrome_main_delegate_testshell_android.cc (right):

https://codereview.chromium.org/10918113/diff/24/chrome/android/testshell/chr...
chrome/android/testshell/chrome_main_delegate_testshell_android.cc:17: //
Register the generic Chrome JNI methods first.  If that succeeds,
Remove comment

https://codereview.chromium.org/10918113/diff/24/chrome/android/testshell/jav...
File chrome/android/testshell/java/AndroidManifest.xml (right):

https://codereview.chromium.org/10918113/diff/24/chrome/android/testshell/jav...
chrome/android/testshell/java/AndroidManifest.xml:57: <uses-sdk
android:minSdkVersion="16" android:targetSdkVersion="16" />
So why still 16?

https://codereview.chromium.org/10918113/diff/24/chrome/app/android/chrome_ma...
File chrome/app/android/chrome_main_delegate_android.cc (right):

https://codereview.chromium.org/10918113/diff/24/chrome/app/android/chrome_ma...
chrome/app/android/chrome_main_delegate_android.cc:26: if (process_type.empty())
{
I think you can remove lines 26-29. It's done below already in RunProcess which
should be called earlier, right?

https://codereview.chromium.org/10918113/diff/24/chrome/app/android/chrome_ma...
File chrome/app/android/chrome_main_delegate_android.h (right):

https://codereview.chromium.org/10918113/diff/24/chrome/app/android/chrome_ma...
chrome/app/android/chrome_main_delegate_android.h:22:
DISALLOW_COPY_AND_ASSIGN(ChromeMainDelegateAndroid);
Nit: move to bottom of class definition.

https://codereview.chromium.org/10918113/diff/24/chrome/chrome_android.gypi
File chrome/chrome_android.gypi (right):

https://codereview.chromium.org/10918113/diff/24/chrome/chrome_android.gypi#n...
chrome/chrome_android.gypi:71:
'<(PRODUCT_DIR)/chromium_testshell/libs/<(android_app_abi)/libchromiumtestshell.so'
This will need to be synced up with Shashi's cl
(http://codereview.chromium.org/10905138/). Whomever lands last will need to
update their change.

https://codereview.chromium.org/10918113/diff/24/chrome/chrome_android.gypi#n...
chrome/chrome_android.gypi:100: '../content/content.gyp:content',
The non-chrome deps seem bogus (they should be pulled in transitively)

https://codereview.chromium.org/10918113/diff/24/chrome/chrome_android.gypi#n...
chrome/chrome_android.gypi:118: 'browser/android/tab_android.h',
Remove this.

https://codereview.chromium.org/10918113/diff/24/chrome/chrome_android.gypi#n...
chrome/chrome_android.gypi:126: 'conditions': [
This doesn't seem necessary. The define is only in content and is defined there.

https://codereview.chromium.org/10918113/diff/24/chrome/common/chrome_paths_a...
File chrome/common/chrome_paths_android.cc (right):

https://codereview.chromium.org/10918113/diff/24/chrome/common/chrome_paths_a...
chrome/common/chrome_paths_android.cc:47: // Any reason to forbid this on
Android?
Remove comment

[Ted C, 2012-09-11 00:10:05]

a few extra nits, but lgtm w/ yaron's changes

https://chromiumcodereview.appspot.com/10918113/diff/24/chrome/android/testsh...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java
(right):

https://chromiumcodereview.appspot.com/10918113/diff/24/chrome/android/testsh...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:20:
private static final String COMMAND_LINE_FILE =
"/data/local/tmp/chrome-command-line";
should this be chrome-test-shell-command-line or something less horrendously
long?

https://chromiumcodereview.appspot.com/10918113/diff/24/chrome/app/android/ch...
File chrome/app/android/chrome_android_initializer.cc (right):

https://chromiumcodereview.appspot.com/10918113/diff/24/chrome/app/android/ch...
chrome/app/android/chrome_android_initializer.cc:16: if
(!content::RegisterLibraryLoaderEntryHook(env)) {
no need for the braces

https://chromiumcodereview.appspot.com/10918113/diff/24/chrome/app/android/ch...
File chrome/app/android/chrome_android_initializer.h (right):

https://chromiumcodereview.appspot.com/10918113/diff/24/chrome/app/android/ch...
chrome/app/android/chrome_android_initializer.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
2012

[David Trainor- moved to gerrit, 2012-09-11 00:25:04]

https://codereview.chromium.org/10918113/diff/24/chrome/android/testshell/chr...
File chrome/android/testshell/chrome_main_delegate_testshell_android.cc (right):

https://codereview.chromium.org/10918113/diff/24/chrome/android/testshell/chr...
chrome/android/testshell/chrome_main_delegate_testshell_android.cc:17: //
Register the generic Chrome JNI methods first.  If that succeeds,
On 2012/09/11 00:01:28, Yaron wrote:
> Remove comment

Done.

https://codereview.chromium.org/10918113/diff/24/chrome/android/testshell/jav...
File chrome/android/testshell/java/AndroidManifest.xml (right):

https://codereview.chromium.org/10918113/diff/24/chrome/android/testshell/jav...
chrome/android/testshell/java/AndroidManifest.xml:57: <uses-sdk
android:minSdkVersion="16" android:targetSdkVersion="16" />
On 2012/09/11 00:01:28, Yaron wrote:
> So why still 16?

Done.

https://codereview.chromium.org/10918113/diff/24/chrome/android/testshell/jav...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java
(right):

https://codereview.chromium.org/10918113/diff/24/chrome/android/testshell/jav...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:20:
private static final String COMMAND_LINE_FILE =
"/data/local/tmp/chrome-command-line";
On 2012/09/11 00:10:05, Ted C wrote:
> should this be chrome-test-shell-command-line or something less horrendously
> long?

Done.

https://codereview.chromium.org/10918113/diff/24/chrome/app/android/chrome_an...
File chrome/app/android/chrome_android_initializer.cc (right):

https://codereview.chromium.org/10918113/diff/24/chrome/app/android/chrome_an...
chrome/app/android/chrome_android_initializer.cc:16: if
(!content::RegisterLibraryLoaderEntryHook(env)) {
On 2012/09/11 00:10:05, Ted C wrote:
> no need for the braces

Done.

https://codereview.chromium.org/10918113/diff/24/chrome/app/android/chrome_an...
File chrome/app/android/chrome_android_initializer.h (right):

https://codereview.chromium.org/10918113/diff/24/chrome/app/android/chrome_an...
chrome/app/android/chrome_android_initializer.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/09/11 00:10:05, Ted C wrote:
> 2012

Done.

https://codereview.chromium.org/10918113/diff/24/chrome/app/android/chrome_ma...
File chrome/app/android/chrome_main_delegate_android.cc (right):

https://codereview.chromium.org/10918113/diff/24/chrome/app/android/chrome_ma...
chrome/app/android/chrome_main_delegate_android.cc:26: if (process_type.empty())
{
On 2012/09/11 00:01:28, Yaron wrote:
> I think you can remove lines 26-29. It's done below already in RunProcess
which
> should be called earlier, right?

Done.

https://codereview.chromium.org/10918113/diff/24/chrome/app/android/chrome_ma...
File chrome/app/android/chrome_main_delegate_android.h (right):

https://codereview.chromium.org/10918113/diff/24/chrome/app/android/chrome_ma...
chrome/app/android/chrome_main_delegate_android.h:22:
DISALLOW_COPY_AND_ASSIGN(ChromeMainDelegateAndroid);
On 2012/09/11 00:01:28, Yaron wrote:
> Nit: move to bottom of class definition.

Done.

https://codereview.chromium.org/10918113/diff/24/chrome/chrome_android.gypi
File chrome/chrome_android.gypi (right):

https://codereview.chromium.org/10918113/diff/24/chrome/chrome_android.gypi#n...
chrome/chrome_android.gypi:71:
'<(PRODUCT_DIR)/chromium_testshell/libs/<(android_app_abi)/libchromiumtestshell.so'
I shall be first!

On 2012/09/11 00:01:28, Yaron wrote:
> This will need to be synced up with Shashi's cl
> (http://codereview.chromium.org/10905138/). Whomever lands last will need to
> update their change.

https://codereview.chromium.org/10918113/diff/24/chrome/chrome_android.gypi#n...
chrome/chrome_android.gypi:100: '../content/content.gyp:content',
On 2012/09/11 00:01:28, Yaron wrote:
> The non-chrome deps seem bogus (they should be pulled in transitively)

Done.

https://codereview.chromium.org/10918113/diff/24/chrome/chrome_android.gypi#n...
chrome/chrome_android.gypi:118: 'browser/android/tab_android.h',
On 2012/09/11 00:01:28, Yaron wrote:
> Remove this.

Done.

https://codereview.chromium.org/10918113/diff/24/chrome/chrome_android.gypi#n...
chrome/chrome_android.gypi:126: 'conditions': [
On 2012/09/11 00:01:28, Yaron wrote:
> This doesn't seem necessary. The define is only in content and is defined
there.

Done.

https://codereview.chromium.org/10918113/diff/24/chrome/common/chrome_paths_a...
File chrome/common/chrome_paths_android.cc (right):

https://codereview.chromium.org/10918113/diff/24/chrome/common/chrome_paths_a...
chrome/common/chrome_paths_android.cc:47: // Any reason to forbid this on
Android?
On 2012/09/11 00:01:28, Yaron wrote:
> Remove comment

Done.

[commit-bot: I haz the power, 2012-09-11 00:25:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtrainor@chromium.org/10918113/27

[commit-bot: I haz the power, 2012-09-11 00:34:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtrainor@chromium.org/10918113/2034

[commit-bot: I haz the power, 2012-09-11 05:51:38]

Change committed as 155950