Add test jar generation logic for ant builds.

Add test jar generation logic for ant builds.

Add a new property generate.test.jar to ant scripts.
When this property is set, a jar containing all compiled
classes and referred classes is generated and placed in
out/{Debug|Release}/test.lib.java directory.
This makes it easier and cleaner to write ant build files for test apks.
New directory structure for the build:
Apks are placed in : out/{Debug|Release}/apks
All java libraries in : out/{Debug|Release}/lib.java
All test java libraries in : out/{Debug|Release}/test.lib.java

This CL also includes cleanup to work with the modified output
directory structure.

BUG=146583
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=157570

[cjhopman, 2012-09-06 22:33:41]

http://codereview.chromium.org/10905138/diff/1019/base/base.gyp
File base/base.gyp (right):

http://codereview.chromium.org/10905138/diff/1019/base/base.gyp#newcode929
base/base.gyp:929: 'target_name': 'build_output_directories',
Could you add a comment explaining why this is needed?

http://codereview.chromium.org/10905138/diff/1019/build/android/ant/chromium-...
File build/android/ant/chromium-jars.xml (right):

http://codereview.chromium.org/10905138/diff/1019/build/android/ant/chromium-...
build/android/ant/chromium-jars.xml:38: />
This property is no longer used.

http://codereview.chromium.org/10905138/diff/1019/build/android/ant/chromium-...
build/android/ant/chromium-jars.xml:50: <mkdir dir="${lib.java.dir}"/>
Remove the mkdir here. Should now be made by the new gyp target.

http://codereview.chromium.org/10905138/diff/1019/build/android/ant/chromium-...
build/android/ant/chromium-jars.xml:88:
jarfile="${lib.dir}/chromium_${PACKAGE_NAME}.jar"
This should be lib.java.dir now.

http://codereview.chromium.org/10905138/diff/1019/build/android/ant/common.xml
File build/android/ant/common.xml (right):

http://codereview.chromium.org/10905138/diff/1019/build/android/ant/common.xm...
build/android/ant/common.xml:84: check-exists="false"/>
Remove the check-exists="false". This directory should be created by the new gyp
target and must exist at this point. Same for the two below.

http://codereview.chromium.org/10905138/diff/1019/build/android/ant/create-te...
File build/android/ant/create-test-jar.js (right):

http://codereview.chromium.org/10905138/diff/1019/build/android/ant/create-te...
build/android/ant/create-test-jar.js:20: var javacCustomClasspath =
Maybe move this assignment down to the loop where it is used.

http://codereview.chromium.org/10905138/diff/1019/build/android/ant/create-te...
build/android/ant/create-test-jar.js:21:
project.getReference("javac.custom.classpath").list();
Nit: indent 4

http://codereview.chromium.org/10905138/diff/1019/build/android/ant/create-te...
build/android/ant/create-test-jar.js:27: // duplicate to "preserve" allows only
the first definition is included.
Nit: Fix this sentence's grammar. Maybe "[...] ensure only the first definition
is included." or "[...] first definition to be included."

http://codereview.chromium.org/10905138/diff/1019/build/android/ant/create-te...
build/android/ant/create-test-jar.js:33: var destFile =
project.getProperty("ant.project.name")+"-debug.jar";
Fix spacing around +

http://codereview.chromium.org/10905138/diff/1019/build/android/ant/create-te...
build/android/ant/create-test-jar.js:52:
(project.getProperty("project.app.package")).replace('.','/');
Indent 4.

http://codereview.chromium.org/10905138/diff/1019/build/android/ant/create-te...
build/android/ant/create-test-jar.js:58: exclusionString += appPackagePath+"/"+
excludedClasses[i] +" ";
Fix spacing around operators.

http://codereview.chromium.org/10905138/diff/1019/build/android/ant/sdk-targe...
File build/android/ant/sdk-targets.xml (right):

http://codereview.chromium.org/10905138/diff/1019/build/android/ant/sdk-targe...
build/android/ant/sdk-targets.xml:98: <then>
Nit: then block is indented too far.

http://codereview.chromium.org/10905138/diff/1019/build/android/ant/sdk-targe...
build/android/ant/sdk-targets.xml:101:
src="${PRODUCT_DIR}/../../build/android/ant/create-test-jar.js" />
This should not be referred to relative to PRODUCT_DIR. If you have to you could
have gyp pass in the path to build/android/ant.

http://codereview.chromium.org/10905138/diff/1019/build/android/pylib/test_op...
File build/android/pylib/test_options_parser.py (right):

http://codereview.chromium.org/10905138/diff/1019/build/android/pylib/test_op...
build/android/pylib/test_options_parser.py:89: ' suffix. for example,
ContentShellTest).'))
Nit: Capitalize "for".

http://codereview.chromium.org/10905138/diff/1019/build/android/pylib/test_op...
build/android/pylib/test_options_parser.py:118: # In case of SDK Build, the jars
and apks have a debug suffix.
have a -debug suffix.

http://codereview.chromium.org/10905138/diff/1019/content/shell/android/javat...
File content/shell/android/javatests/content_shell_test_apk.xml (right):

http://codereview.chromium.org/10905138/diff/1019/content/shell/android/javat...
content/shell/android/javatests/content_shell_test_apk.xml:21: <!-- Set the
output directory for the final apk to the ${apks.dir} -->
Nit: full stop.

[shashi, 2012-09-07 00:41:32]

https://chromiumcodereview.appspot.com/10905138/diff/1019/base/base.gyp
File base/base.gyp (right):

https://chromiumcodereview.appspot.com/10905138/diff/1019/base/base.gyp#newco...
base/base.gyp:929: 'target_name': 'build_output_directories',
On 2012/09/06 22:33:41, cjhopman wrote:
> Could you add a comment explaining why this is needed?

Done.

https://chromiumcodereview.appspot.com/10905138/diff/1019/build/android/ant/c...
File build/android/ant/chromium-jars.xml (right):

https://chromiumcodereview.appspot.com/10905138/diff/1019/build/android/ant/c...
build/android/ant/chromium-jars.xml:38: />
On 2012/09/06 22:33:41, cjhopman wrote:
> This property is no longer used.

Done.

https://chromiumcodereview.appspot.com/10905138/diff/1019/build/android/ant/c...
build/android/ant/chromium-jars.xml:50: <mkdir dir="${lib.java.dir}"/>
On 2012/09/06 22:33:41, cjhopman wrote:
> Remove the mkdir here. Should now be made by the new gyp target.

Done.

https://chromiumcodereview.appspot.com/10905138/diff/1019/build/android/ant/c...
build/android/ant/chromium-jars.xml:88:
jarfile="${lib.dir}/chromium_${PACKAGE_NAME}.jar"
On 2012/09/06 22:33:41, cjhopman wrote:
> This should be lib.java.dir now.

Done.

https://chromiumcodereview.appspot.com/10905138/diff/1019/build/android/ant/c...
File build/android/ant/common.xml (right):

https://chromiumcodereview.appspot.com/10905138/diff/1019/build/android/ant/c...
build/android/ant/common.xml:84: check-exists="false"/>
On 2012/09/06 22:33:41, cjhopman wrote:
> Remove the check-exists="false". This directory should be created by the new
gyp
> target and must exist at this point. Same for the two below.

Done.

https://chromiumcodereview.appspot.com/10905138/diff/1019/build/android/ant/c...
File build/android/ant/create-test-jar.js (right):

https://chromiumcodereview.appspot.com/10905138/diff/1019/build/android/ant/c...
build/android/ant/create-test-jar.js:20: var javacCustomClasspath =
On 2012/09/06 22:33:41, cjhopman wrote:
> Maybe move this assignment down to the loop where it is used.

Done.

https://chromiumcodereview.appspot.com/10905138/diff/1019/build/android/ant/c...
build/android/ant/create-test-jar.js:21:
project.getReference("javac.custom.classpath").list();
On 2012/09/06 22:33:41, cjhopman wrote:
> Nit: indent 4

Done.

https://chromiumcodereview.appspot.com/10905138/diff/1019/build/android/ant/c...
build/android/ant/create-test-jar.js:27: // duplicate to "preserve" allows only
the first definition is included.
On 2012/09/06 22:33:41, cjhopman wrote:
> Nit: Fix this sentence's grammar. Maybe "[...] ensure only the first
definition
> is included." or "[...] first definition to be included."

Done.

https://chromiumcodereview.appspot.com/10905138/diff/1019/build/android/ant/c...
build/android/ant/create-test-jar.js:33: var destFile =
project.getProperty("ant.project.name")+"-debug.jar";
On 2012/09/06 22:33:41, cjhopman wrote:
> Fix spacing around +

Done.

https://chromiumcodereview.appspot.com/10905138/diff/1019/build/android/ant/c...
build/android/ant/create-test-jar.js:52:
(project.getProperty("project.app.package")).replace('.','/');
On 2012/09/06 22:33:41, cjhopman wrote:
> Indent 4.

Done.

https://chromiumcodereview.appspot.com/10905138/diff/1019/build/android/ant/c...
build/android/ant/create-test-jar.js:58: exclusionString += appPackagePath+"/"+
excludedClasses[i] +" ";
On 2012/09/06 22:33:41, cjhopman wrote:
> Fix spacing around operators.

Done.

https://chromiumcodereview.appspot.com/10905138/diff/1019/build/android/ant/s...
File build/android/ant/sdk-targets.xml (right):

https://chromiumcodereview.appspot.com/10905138/diff/1019/build/android/ant/s...
build/android/ant/sdk-targets.xml:98: <then>
On 2012/09/06 22:33:41, cjhopman wrote:
> Nit: then block is indented too far.

Done.

https://chromiumcodereview.appspot.com/10905138/diff/1019/build/android/ant/s...
build/android/ant/sdk-targets.xml:101:
src="${PRODUCT_DIR}/../../build/android/ant/create-test-jar.js" />
On 2012/09/06 22:33:41, cjhopman wrote:
> This should not be referred to relative to PRODUCT_DIR. If you have to you
could
> have gyp pass in the path to build/android/ant.

Done.

https://chromiumcodereview.appspot.com/10905138/diff/1019/build/android/pylib...
File build/android/pylib/test_options_parser.py (right):

https://chromiumcodereview.appspot.com/10905138/diff/1019/build/android/pylib...
build/android/pylib/test_options_parser.py:89: ' suffix. for example,
ContentShellTest).'))
On 2012/09/06 22:33:41, cjhopman wrote:
> Nit: Capitalize "for".

Done.

https://chromiumcodereview.appspot.com/10905138/diff/1019/build/android/pylib...
build/android/pylib/test_options_parser.py:118: # In case of SDK Build, the jars
and apks have a debug suffix.
On 2012/09/06 22:33:41, cjhopman wrote:
> have a -debug suffix.

Done.

https://chromiumcodereview.appspot.com/10905138/diff/1019/content/shell/andro...
File content/shell/android/javatests/content_shell_test_apk.xml (right):

https://chromiumcodereview.appspot.com/10905138/diff/1019/content/shell/andro...
content/shell/android/javatests/content_shell_test_apk.xml:21: <!-- Set the
output directory for the final apk to the ${apks.dir} -->
On 2012/09/06 22:33:41, cjhopman wrote:
> Nit: full stop.

Done.

[shashi, 2012-09-07 01:50:42]

base/, content/: bretw
content/shell/android : yfriedman
build: peter, yfriedman, cjhopman, nyquist

[Yaron, 2012-09-07 19:19:31]

https://chromiumcodereview.appspot.com/10905138/diff/12004/base/base.gyp
File base/base.gyp (right):

https://chromiumcodereview.appspot.com/10905138/diff/12004/base/base.gyp#newc...
base/base.gyp:929: # Target for creating common output build directories.
Seems like this may as well be in a build/ .gyp file since it's about the build
and not the base library.

https://chromiumcodereview.appspot.com/10905138/diff/12004/build/android/adb_...
File build/android/adb_install_content_shell (right):

https://chromiumcodereview.appspot.com/10905138/diff/12004/build/android/adb_...
build/android/adb_install_content_shell:20: constants.SDK_BUILD_APKS_DIR,
'ContentShell-debug.apk')
So what happens for a non-sdk build?

https://chromiumcodereview.appspot.com/10905138/diff/12004/build/android/ant/...
File build/android/ant/sdk-targets.xml (right):

https://chromiumcodereview.appspot.com/10905138/diff/12004/build/android/ant/...
build/android/ant/sdk-targets.xml:94: <!-- if the project needs a test jar then
generate a jar containing
Nit: Start comment on next line and capitalize.

https://chromiumcodereview.appspot.com/10905138/diff/12004/build/android/ant/...
build/android/ant/sdk-targets.xml:99: <echo level="info">Creating test jar
file...</echo>
Include the property in the description so we can see which one is being
generated

https://chromiumcodereview.appspot.com/10905138/diff/12004/build/android/ant/...
build/android/ant/sdk-targets.xml:101:
location="${CHROMIUM_SRC}/build/android/ant/create-test-jar.js" />
indent 4. Please also remove trailing spaces before the "/>" throughout your
change

https://chromiumcodereview.appspot.com/10905138/diff/12004/content/shell/andr...
File content/shell/android/javatests/content_shell_test_apk.xml (left):

https://chromiumcodereview.appspot.com/10905138/diff/12004/content/shell/andr...
content/shell/android/javatests/content_shell_test_apk.xml:48: <!-- copy
gdbserver to main libs directory if building debug.
Where did this logic go?

https://chromiumcodereview.appspot.com/10905138/diff/12004/content/shell/andr...
File content/shell/android/javatests/content_shell_test_apk.xml (right):

https://chromiumcodereview.appspot.com/10905138/diff/12004/content/shell/andr...
content/shell/android/javatests/content_shell_test_apk.xml:31: <fileset
file="${lib.java.dir}/chromium_media.jar"/>
Why media?

[cjhopman, 2012-09-07 19:50:35]

http://codereview.chromium.org/10905138/diff/12004/base/base.gyp
File base/base.gyp (right):

http://codereview.chromium.org/10905138/diff/12004/base/base.gyp#newcode939
base/base.gyp:939: 'action_name': 'create_output_dirs',
Maybe rename this to create_java_output_dirs.

http://codereview.chromium.org/10905138/diff/12004/build/android/pylib/test_o...
File build/android/pylib/test_options_parser.py (right):

http://codereview.chromium.org/10905138/diff/12004/build/android/pylib/test_o...
build/android/pylib/test_options_parser.py:87: '(without the .apk extension) or
For SDK '
Nit: lowercase for (or delete 'or' and start a new sentence).

http://codereview.chromium.org/10905138/diff/12004/content/shell/android/java...
File content/shell/android/java/content_shell_apk.xml (left):

http://codereview.chromium.org/10905138/diff/12004/content/shell/android/java...
content/shell/android/java/content_shell_apk.xml:41: <!-- We expect PRODUCT_DIR
to be set like the gyp var (e.g. $ROOT/out/Debug) -->
We are no longer checking that PRODUCT_DIR is set? Maybe you could add this
check in common.xml

http://codereview.chromium.org/10905138/diff/12004/content/shell/android/java...
File content/shell/android/java/content_shell_apk.xml (right):

http://codereview.chromium.org/10905138/diff/12004/content/shell/android/java...
content/shell/android/java/content_shell_apk.xml:35: <fileset
file="${lib.java.dir}/chromium_content.jar"/>
<pathelement location=

http://codereview.chromium.org/10905138/diff/12004/content/shell/android/java...
File content/shell/android/javatests/content_shell_test_apk.xml (left):

http://codereview.chromium.org/10905138/diff/12004/content/shell/android/java...
content/shell/android/javatests/content_shell_test_apk.xml:48: <!-- copy
gdbserver to main libs directory if building debug.
This step claims to enable native debugging. You've deleted this step. Are the
comments/echo here wrong?

http://codereview.chromium.org/10905138/diff/12004/content/shell/android/java...
File content/shell/android/javatests/content_shell_test_apk.xml (right):

http://codereview.chromium.org/10905138/diff/12004/content/shell/android/java...
content/shell/android/javatests/content_shell_test_apk.xml:15: <property-value
name="target.abi" value="${APP_ABI}"/>
Should the other below this also be switched to the safe setters?

http://codereview.chromium.org/10905138/diff/12004/content/shell/android/java...
content/shell/android/javatests/content_shell_test_apk.xml:22:
<property-location name="out.final.file"
location="${apks.dir}/${ant.project.name}-debug.apk"
Shouldn't this directory always exist at this point?

http://codereview.chromium.org/10905138/diff/12004/content/shell/android/java...
content/shell/android/javatests/content_shell_test_apk.xml:28: <fileset
file="${lib.java.dir}/chromium_base_javatests.jar"/>
use <pathelement location=[...]/> for a single file rather than a fileset.

http://codereview.chromium.org/10905138/diff/12004/content/shell/android/java...
content/shell/android/javatests/content_shell_test_apk.xml:32: <fileset
file="${test.lib.java.dir}/ContentShell-debug.jar"/>
I don't really like depending on one of the debug jars here... For example, this
will fail if ContentShell starts depending on media_java since the debug jar
just packages that all together, and then you'll have it duplicated...

If you need the ContentShell classes packaged in a jar, then Content shell
should create a normal jar for you and use that here.

http://codereview.chromium.org/10905138/diff/12004/content/shell/android/java...
content/shell/android/javatests/content_shell_test_apk.xml:39: <fileset
file="${test.lib.java.dir}/ContentShellTest-debug.jar"/>
<pathelement location=

[shashi, 2012-09-07 20:53:05]

https://chromiumcodereview.appspot.com/10905138/diff/12004/content/shell/andr...
File content/shell/android/javatests/content_shell_test_apk.xml (left):

https://chromiumcodereview.appspot.com/10905138/diff/12004/content/shell/andr...
content/shell/android/javatests/content_shell_test_apk.xml:48: <!-- copy
gdbserver to main libs directory if building debug.
I do not think this is needed for ContentShell tests.It is needed for
ContentShell. I think most of the steps in the file were result of copying the
file from ContentShell xml file.

On 2012/09/07 19:50:35, cjhopman wrote:
> This step claims to enable native debugging. You've deleted this step. Are the
> comments/echo here wrong?

[shashi, 2012-09-07 21:02:29]

https://chromiumcodereview.appspot.com/10905138/diff/12004/content/shell/andr...
File content/shell/android/java/content_shell_apk.xml (left):

https://chromiumcodereview.appspot.com/10905138/diff/12004/content/shell/andr...
content/shell/android/java/content_shell_apk.xml:41: <!-- We expect PRODUCT_DIR
to be set like the gyp var (e.g. $ROOT/out/Debug) -->
It is already done in common.xml, due to safe setters. In fact all common
directories use safe setters and are defined relative to PRODUCT_DIR, so if
PRODUCT_DIR is not set, step will fail and we no longer depend on env variables
so the logic here is redundant.

On 2012/09/07 19:50:35, cjhopman wrote:
> We are no longer checking that PRODUCT_DIR is set? Maybe you could add this
> check in common.xml

[shashi, 2012-09-07 23:31:31]

I verified that ContentShellTest.apk does not have any c++ code or .so library,
so makes sense to not copy gdbserver with it and remove it.

https://chromiumcodereview.appspot.com/10905138/diff/12004/base/base.gyp
File base/base.gyp (right):

https://chromiumcodereview.appspot.com/10905138/diff/12004/base/base.gyp#newc...
base/base.gyp:929: # Target for creating common output build directories.
On 2012/09/07 19:19:31, Yaron wrote:
> Seems like this may as well be in a build/ .gyp file since it's about the
build
> and not the base library.

Done.

https://chromiumcodereview.appspot.com/10905138/diff/12004/base/base.gyp#newc...
base/base.gyp:939: 'action_name': 'create_output_dirs',
On 2012/09/07 19:50:35, cjhopman wrote:
> Maybe rename this to create_java_output_dirs.

Done.

https://chromiumcodereview.appspot.com/10905138/diff/12004/build/android/adb_...
File build/android/adb_install_content_shell (right):

https://chromiumcodereview.appspot.com/10905138/diff/12004/build/android/adb_...
build/android/adb_install_content_shell:20: constants.SDK_BUILD_APKS_DIR,
'ContentShell-debug.apk')
This script is only upstream.
On 2012/09/07 19:19:31, Yaron wrote:
> So what happens for a non-sdk build?

https://chromiumcodereview.appspot.com/10905138/diff/12004/build/android/pyli...
File build/android/pylib/test_options_parser.py (right):

https://chromiumcodereview.appspot.com/10905138/diff/12004/build/android/pyli...
build/android/pylib/test_options_parser.py:87: '(without the .apk extension) or
For SDK '
On 2012/09/07 19:50:35, cjhopman wrote:
> Nit: lowercase for (or delete 'or' and start a new sentence).

Done.

https://chromiumcodereview.appspot.com/10905138/diff/12004/content/shell/andr...
File content/shell/android/java/content_shell_apk.xml (right):

https://chromiumcodereview.appspot.com/10905138/diff/12004/content/shell/andr...
content/shell/android/java/content_shell_apk.xml:35: <fileset
file="${lib.java.dir}/chromium_content.jar"/>
On 2012/09/07 19:50:35, cjhopman wrote:
> <pathelement location=

Done.

https://chromiumcodereview.appspot.com/10905138/diff/12004/content/shell/andr...
File content/shell/android/javatests/content_shell_test_apk.xml (right):

https://chromiumcodereview.appspot.com/10905138/diff/12004/content/shell/andr...
content/shell/android/javatests/content_shell_test_apk.xml:22:
<property-location name="out.final.file"
location="${apks.dir}/${ant.project.name}-debug.apk"
Yes, it is just a safe setter.
On 2012/09/07 19:50:35, cjhopman wrote:
> Shouldn't this directory always exist at this point?

https://chromiumcodereview.appspot.com/10905138/diff/12004/content/shell/andr...
content/shell/android/javatests/content_shell_test_apk.xml:28: <fileset
file="${lib.java.dir}/chromium_base_javatests.jar"/>
On 2012/09/07 19:50:35, cjhopman wrote:
> use <pathelement location=[...]/> for a single file rather than a fileset.

Done.

https://chromiumcodereview.appspot.com/10905138/diff/12004/content/shell/andr...
content/shell/android/javatests/content_shell_test_apk.xml:31: <fileset
file="${lib.java.dir}/chromium_media.jar"/>
Nice catch, it is not needed, I cleaned up a few jars but still left it there
somehow.
On 2012/09/07 19:19:31, Yaron wrote:
> Why media?

https://chromiumcodereview.appspot.com/10905138/diff/12004/content/shell/andr...
content/shell/android/javatests/content_shell_test_apk.xml:32: <fileset
file="${test.lib.java.dir}/ContentShell-debug.jar"/>
This is for javac classpath. If multiple definition are present first one is
used. So this will not fail the build.

This can fail if we use all the jars in classpath also for dex step. This is
avoided by using the compiled test jar file for dex step. (below). So even when
content_shell will depend on media or any other common jar, the compilation or
dex step will not fail.

On 2012/09/07 19:50:35, cjhopman wrote:
> I don't really like depending on one of the debug jars here... For example,
this
> will fail if ContentShell starts depending on media_java since the debug jar
> just packages that all together, and then you'll have it duplicated...
> 
> If you need the ContentShell classes packaged in a jar, then Content shell
> should create a normal jar for you and use that here.

https://chromiumcodereview.appspot.com/10905138/diff/12004/content/shell/andr...
content/shell/android/javatests/content_shell_test_apk.xml:39: <fileset
file="${test.lib.java.dir}/ContentShellTest-debug.jar"/>
On 2012/09/07 19:50:35, cjhopman wrote:
> <pathelement location=

Done.

[Yaron, 2012-09-10 19:58:10]

lgtm but please wait for Chris' approval

https://chromiumcodereview.appspot.com/10905138/diff/12004/build/android/adb_...
File build/android/adb_install_content_shell (right):

https://chromiumcodereview.appspot.com/10905138/diff/12004/build/android/adb_...
build/android/adb_install_content_shell:20: constants.SDK_BUILD_APKS_DIR,
'ContentShell-debug.apk')
On 2012/09/07 23:31:31, shashi wrote:
> This script is only upstream.
> On 2012/09/07 19:19:31, Yaron wrote:
> > So what happens for a non-sdk build?
> 

Not really. The script comes down with the merge and you can build/run content
shell downstream too. I guess it's no more broken then it previously was because
of the "-debug.apk" suffix.

https://chromiumcodereview.appspot.com/10905138/diff/7006/build/all_android.gyp
File build/all_android.gyp (right):

https://chromiumcodereview.appspot.com/10905138/diff/7006/build/all_android.g...
build/all_android.gyp:126: '<(PRODUCT_DIR)/lib.java',
Nit: indentation is off throughout this target.

https://chromiumcodereview.appspot.com/10905138/diff/7006/build/android/ant/s...
File build/android/ant/sdk-targets.xml (right):

https://chromiumcodereview.appspot.com/10905138/diff/7006/build/android/ant/s...
build/android/ant/sdk-targets.xml:101: ${ant.project.name}-debug.jar</echo>
indent 4 for line-continuations.

[shashi, 2012-09-11 17:43:36]

content/: brettw , very short code review, only changes are in gyp output
directory for ContentShell and ContentShellTest.
build/: cjhopman, PTAL.

[brettw, 2012-09-11 18:19:54]

lgtm

[cjhopman, 2012-09-13 00:07:02]

http://codereview.chromium.org/10905138/diff/9008/build/android/ant/create-te...
File build/android/ant/create-test-jar.js (right):

http://codereview.chromium.org/10905138/diff/9008/build/android/ant/create-te...
build/android/ant/create-test-jar.js:62: echo.setMessage("Creating test jar: " +
You have almost the exact same echo in the ant build file just before calling
this script. Not sure if that was intentional...

http://codereview.chromium.org/10905138/diff/9008/build/android/pylib/test_op...
File build/android/pylib/test_options_parser.py (right):

http://codereview.chromium.org/10905138/diff/9008/build/android/pylib/test_op...
build/android/pylib/test_options_parser.py:89: ' suffix(for example,
ContentShellTest).'))
Nit: The space should go at the end of the line above.

http://codereview.chromium.org/10905138/diff/9008/content/shell/android/java/...
File content/shell/android/java/content_shell_apk.xml (right):

http://codereview.chromium.org/10905138/diff/9008/content/shell/android/java/...
content/shell/android/java/content_shell_apk.xml:31: <property
name="generate.test.jar" value="true"/>
I don't see why ContentShell is creating a test jar. The test jars should only
be used for listing the tests by annotation, and there won't be any tests
included in ContentShell-debug.jar.

http://codereview.chromium.org/10905138/diff/9008/content/shell/android/javat...
File content/shell/android/javatests/content_shell_test_apk.xml (right):

http://codereview.chromium.org/10905138/diff/9008/content/shell/android/javat...
content/shell/android/javatests/content_shell_test_apk.xml:32: <pathelement
location="${test.lib.java.dir}/ContentShell-debug.jar"/>
There should not be any test jars included in the classpath. They should only be
used for listing tests.

http://codereview.chromium.org/10905138/diff/9008/content/shell/android/javat...
content/shell/android/javatests/content_shell_test_apk.xml:35: <!-- Use the
output jar for dex, this jar contains all the compiled
Is there a reason that the dex inputs had to change? Isn't this just including
the same set of stuff as it used to?

[shashi, 2012-09-13 01:12:51]

http://codereview.chromium.org/10905138/diff/9008/content/shell/android/java/...
File content/shell/android/java/content_shell_apk.xml (right):

http://codereview.chromium.org/10905138/diff/9008/content/shell/android/java/...
content/shell/android/java/content_shell_apk.xml:31: <property
name="generate.test.jar" value="true"/>
The jar created here is referred in classpath for instrumentation tests.

In general any apk which also has instrumentation tests, will need some way in
which tests can refer to classes in apk.
This jar is generated so that test apk can include it in classpath if needed.

On 2012/09/13 00:07:02, cjhopman wrote:
> I don't see why ContentShell is creating a test jar. The test jars should only
> be used for listing the tests by annotation, and there won't be any tests
> included in ContentShell-debug.jar.

http://codereview.chromium.org/10905138/diff/9008/content/shell/android/javat...
File content/shell/android/javatests/content_shell_test_apk.xml (right):

http://codereview.chromium.org/10905138/diff/9008/content/shell/android/javat...
content/shell/android/javatests/content_shell_test_apk.xml:32: <pathelement
location="${test.lib.java.dir}/ContentShell-debug.jar"/>
Same reason as before, jar is included because classes in instrumentation tests
apk refer to classes in the apk being instrumented.
On 2012/09/13 00:07:02, cjhopman wrote:
> There should not be any test jars included in the classpath. They should only
be
> used for listing tests.

http://codereview.chromium.org/10905138/diff/9008/content/shell/android/javat...
content/shell/android/javatests/content_shell_test_apk.xml:35: <!-- Use the
output jar for dex, this jar contains all the compiled
Yes, because there can be duplication of classes in classpath, but in the jar
the classes are all compiled and referred classes.
On 2012/09/13 00:07:02, cjhopman wrote:
> Is there a reason that the dex inputs had to change? Isn't this just including
> the same set of stuff as it used to?

[cjhopman, 2012-09-18 21:40:20]

lgtm

https://chromiumcodereview.appspot.com/10905138/diff/17001/build/all_android.gyp
File build/all_android.gyp (right):

https://chromiumcodereview.appspot.com/10905138/diff/17001/build/all_android....
build/all_android.gyp:119: # Target for creating common output build
directories.
Nit: Line breaks in this comment appear to be at ~60 columns instead of ~80

https://chromiumcodereview.appspot.com/10905138/diff/17001/build/android/pyli...
File build/android/pylib/test_options_parser.py (right):

https://chromiumcodereview.appspot.com/10905138/diff/17001/build/android/pyli...
build/android/pylib/test_options_parser.py:90: ' suffix(for example,
ContentShellTest).'))
The space should go at the end of the previous line.

https://chromiumcodereview.appspot.com/10905138/diff/17001/build/android/pyli...
build/android/pylib/test_options_parser.py:130: % options.test_apk)
Nit: this should fit on the line above and that would be formatted consistently
with line 125

https://chromiumcodereview.appspot.com/10905138/diff/17001/content/shell/andr...
File content/shell/android/java/content_shell_apk.xml (right):

https://chromiumcodereview.appspot.com/10905138/diff/17001/content/shell/andr...
content/shell/android/java/content_shell_apk.xml:16: -->
No need to change this to multiline (100 columns for ant).

[commit-bot: I haz the power, 2012-09-18 21:57:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shashishekhar@chromium.org/10905138/26004

[commit-bot: I haz the power, 2012-09-18 21:57:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shashishekhar@chromium.org/10905138/26004

[commit-bot: I haz the power, 2012-09-18 22:14:09]

Retried try job too often for step(s) interactive_ui_tests, jingle_unittests,
gpu_unittests, base_unittests, sync_integration_tests, sql_unittests,
chrome_frame_unittests, content_unittests, safe_browsing_tests, ipc_tests,
mini_installer_test, cacheinvalidation_unittests, remoting_unittests,
browser_tests, net_unittests, installer_util_unittests, chrome_frame_net_tests,
check_deps, crypto_unittests, unit_tests, nacl_integration, media_unittests,
content_browsertests, printing_unittests, sync_unit_tests