Support Java resources within content and chrome

Support Java resources within content.

This provides support for android-style resource folders in content and other
non-apk Java targets. The AppResource hack can then be removed shortly.

Details: while building a non-apk target (e.g. chromium_content.jar), we
generate an R.java file with non-final constants and in the appropriate Java
package (e.g. org.chromium.content.R) using the resources in the target (e.g.
content/public/android/java/res). This R.java is used to produce the jar, but is
not included in the jar itself.

When we later build an apk, we merge the resources from the apk (e.g.
org/chromium/content_shell/res) with the resources from the non-apk targets it
depends on (e.g. content/public/android/java/res). A new R.java is generated
using the merged resources with the correct mapping from resources to integer
IDs. This R.java file is copied into each needed package (e.g.
org.chromium.content.R and org.chromium.content_shell.R) and included in the
apk.

This is the first of three CLs to replace AppResource with R:
1. http://codereview.chromium.org/11363150 - Support Java resources within content
2. http://codereview.chromium.org/11360207 - Add Java resources to content and chrome
3. http://codereview.chromium.org/11377117 - Remove AppResource and unneeded resources

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

[benm (inactive), 2012-11-09 11:05:10]

http://codereview.chromium.org/11363150/diff/1/content/public/android/java/sr...
File
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java
(right):

http://codereview.chromium.org/11363150/diff/1/content/public/android/java/sr...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:124:
sOverlayDrawable =
context.getResources().getDrawable(R.drawable.ondemand_overlay);
I don't think that this will work for WebView. We do not have a concept of a
final built APK (only developers using WebView do), and need to map the
resources that content/ needs to resource IDs in the Android framework. The
extra level of indirection that AppResource gave us suited perfectly - I did not
realise this was just a hack?

[benm (inactive), 2012-11-09 16:20:00]

https://codereview.chromium.org/11363150/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java
(left):

https://codereview.chromium.org/11363150/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:124:
AppResource.DRAWABLE_LINK_PREVIEW_POPUP_OVERLAY);
Should there be a diff in AppResource.java to remove this?

[newt (away), 2012-11-09 21:37:44]

https://codereview.chromium.org/11363150/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java
(left):

https://codereview.chromium.org/11363150/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:124:
AppResource.DRAWABLE_LINK_PREVIEW_POPUP_OVERLAY);
On 2012/11/09 16:20:00, benm wrote:
> Should there be a diff in AppResource.java to remove this?

My plan is to change AppResource.java in another CL.  For now, I just want to
reference org.chromium.content.R.drawable.ondemand_overlay to prove that it
works.

https://codereview.chromium.org/11363150/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java
(right):

https://codereview.chromium.org/11363150/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:124:
sOverlayDrawable =
context.getResources().getDrawable(R.drawable.ondemand_overlay);
On 2012/11/09 11:05:11, benm wrote:
> I don't think that this will work for WebView. We do not have a concept of a
> final built APK (only developers using WebView do), and need to map the
> resources that content/ needs to resource IDs in the Android framework. The
> extra level of indirection that AppResource gave us suited perfectly - I did
not
> realise this was just a hack?

As discussed offline, WebView will used a hand crafted version of
org.chromium.content.R, which will provide the sort of indirection that
AppResource currently does.

[cjhopman, 2012-11-09 22:21:30]

https://codereview.chromium.org/11363150/diff/1/build/android/ant/chromium-ap...
File build/android/ant/chromium-apk.xml (right):

https://codereview.chromium.org/11363150/diff/1/build/android/ant/chromium-ap...
build/android/ant/chromium-apk.xml:37: semicolon-delimited while
ADDITION_RES_PACKAGES is space-delimited, hence
ADDITION_RES_PACKAGES is misspelled.

https://codereview.chromium.org/11363150/diff/1/build/android/ant/chromium-ap...
build/android/ant/chromium-apk.xml:38: the javascript task. -->
Nit: Use multi-line comment formatting like on lines 8-11

https://codereview.chromium.org/11363150/diff/1/build/android/ant/chromium-ap...
build/android/ant/chromium-apk.xml:43: var before =
project.getProperty("ADDITIONAL_RES_PACKAGES");
Nit: indent 2, not 4

https://codereview.chromium.org/11363150/diff/1/build/java.gypi
File build/java.gypi (right):

https://codereview.chromium.org/11363150/diff/1/build/java.gypi#newcode42
build/java.gypi:42: #  R_package_path_ - Same as R_package, but replace each '.'
with '/'.  (The
Instead of the trailing underscore, you could have R_package_path always be
specified with a leading '/'. Then gyp treats it as an absolute path and doesn't
relativize it. I don't know which way is cleaner.

https://codereview.chromium.org/11363150/diff/1/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/11363150/diff/1/build/java_apk.gypi#newcode41
build/java_apk.gypi:41: #    each directory in additional_res_dirs, as a
semicolon-separated list.
This is a list now, not semicolon-separated

https://codereview.chromium.org/11363150/diff/2004/content/public/android/jav...
File content/public/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/11363150/diff/2004/content/public/android/jav...
content/public/android/java/AndroidManifest.xml:10:
package="org.chromium.content">
Is it possible to do this without the AndroidManifest for each different
library? Maybe using just a single dummy manifest and some magic aapt arguments
(--custom-package maybe)?

[newt (away), 2012-11-09 23:05:35]

https://codereview.chromium.org/11363150/diff/1/build/android/ant/chromium-ap...
File build/android/ant/chromium-apk.xml (right):

https://codereview.chromium.org/11363150/diff/1/build/android/ant/chromium-ap...
build/android/ant/chromium-apk.xml:37: semicolon-delimited while
ADDITION_RES_PACKAGES is space-delimited, hence
On 2012/11/09 22:21:30, cjhopman wrote:
> ADDITION_RES_PACKAGES is misspelled.

Done.

https://codereview.chromium.org/11363150/diff/1/build/android/ant/chromium-ap...
build/android/ant/chromium-apk.xml:38: the javascript task. -->
On 2012/11/09 22:21:30, cjhopman wrote:
> Nit: Use multi-line comment formatting like on lines 8-11

Done.

https://codereview.chromium.org/11363150/diff/1/build/android/ant/chromium-ap...
build/android/ant/chromium-apk.xml:43: var before =
project.getProperty("ADDITIONAL_RES_PACKAGES");
On 2012/11/09 22:21:30, cjhopman wrote:
> Nit: indent 2, not 4

Done.

https://codereview.chromium.org/11363150/diff/1/build/java.gypi
File build/java.gypi (right):

https://codereview.chromium.org/11363150/diff/1/build/java.gypi#newcode42
build/java.gypi:42: #  R_package_path_ - Same as R_package, but replace each '.'
with '/'.  (The
On 2012/11/09 22:21:30, cjhopman wrote:
> Instead of the trailing underscore, you could have R_package_path always be
> specified with a leading '/'. Then gyp treats it as an absolute path and
doesn't
> relativize it. I don't know which way is cleaner.

I considered that... though it could break if someone forgets to put the leading
slash somewhere.  Or I could use R_package_relpath...

https://codereview.chromium.org/11363150/diff/1/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/11363150/diff/1/build/java_apk.gypi#newcode41
build/java_apk.gypi:41: #    each directory in additional_res_dirs, as a
semicolon-separated list.
On 2012/11/09 22:21:30, cjhopman wrote:
> This is a list now, not semicolon-separated

Done.

https://codereview.chromium.org/11363150/diff/2004/content/public/android/jav...
File content/public/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/11363150/diff/2004/content/public/android/jav...
content/public/android/java/AndroidManifest.xml:10:
package="org.chromium.content">
On 2012/11/09 22:21:30, cjhopman wrote:
> Is it possible to do this without the AndroidManifest for each different
> library? Maybe using just a single dummy manifest and some magic aapt
arguments
> (--custom-package maybe)? 

any ideas where this file could live?

[cjhopman, 2012-11-12 16:47:32]

https://codereview.chromium.org/11363150/diff/2004/content/public/android/jav...
File content/public/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/11363150/diff/2004/content/public/android/jav...
content/public/android/java/AndroidManifest.xml:10:
package="org.chromium.content">
On 2012/11/09 23:05:35, newt wrote:
> On 2012/11/09 22:21:30, cjhopman wrote:
> > Is it possible to do this without the AndroidManifest for each different
> > library? Maybe using just a single dummy manifest and some magic aapt
> arguments
> > (--custom-package maybe)? 
> 
> any ideas where this file could live?

Probably build/android.

[cjhopman, 2012-11-14 00:34:41]

http://codereview.chromium.org/11363150/diff/6004/build/java.gypi
File build/java.gypi (right):

http://codereview.chromium.org/11363150/diff/6004/build/java.gypi#newcode99
build/java.gypi:99: 'conditions': [
I think this block should go before the previous action, so that this file reads
in the same order that the actions are actually executed.

[newt (away), 2012-11-14 03:28:50]

http://codereview.chromium.org/11363150/diff/6004/build/java.gypi
File build/java.gypi (right):

http://codereview.chromium.org/11363150/diff/6004/build/java.gypi#newcode99
build/java.gypi:99: 'conditions': [
On 2012/11/14 00:34:41, cjhopman wrote:
> I think this block should go before the previous action, so that this file
reads
> in the same order that the actions are actually executed.

I just noticed that other gyp files tend to put conditions at the end.  If you
feel that this should still be moved up, I'll do it.

[cjhopman, 2012-11-14 21:16:19]

lgtm either with the conditions block moved or not

http://codereview.chromium.org/11363150/diff/6004/build/java.gypi
File build/java.gypi (right):

http://codereview.chromium.org/11363150/diff/6004/build/java.gypi#newcode99
build/java.gypi:99: 'conditions': [
On 2012/11/14 03:28:50, newt wrote:
> On 2012/11/14 00:34:41, cjhopman wrote:
> > I think this block should go before the previous action, so that this file
> reads
> > in the same order that the actions are actually executed.
> 
> I just noticed that other gyp files tend to put conditions at the end.  If you
> feel that this should still be moved up, I'll do it.
Yeah, I think it would be better to move it up.

[newt (away), 2012-11-15 00:25:40]

http://codereview.chromium.org/11363150/diff/13006/chrome/android/java/res/va...
File chrome/android/java/res/values/strings.xml (right):

http://codereview.chromium.org/11363150/diff/13006/chrome/android/java/res/va...
chrome/android/java/res/values/strings.xml:8: <resources>
this empty file forces git to create the res folder.  otherwise gyp will fail.

http://codereview.chromium.org/11363150/diff/13006/content/public/android/jav...
File content/public/android/java/res/values/strings.xml (right):

http://codereview.chromium.org/11363150/diff/13006/content/public/android/jav...
content/public/android/java/res/values/strings.xml:8: <resources>
this empty file forces git to create the res folder.  otherwise gyp will fail.

[newt (away), 2012-11-15 00:55:45]

Primary reviewers:
+benm
+cjhopman

Secondary reviewers:
+thakis for chrome changes
+piman for content changes
+nileshagrawal for chrome/android and content/public/android changes

[newt (away), 2012-11-15 00:56:34]

Primary reviewers:
+benm
+cjhopman

Secondary reviewers:
+thakis for chrome changes
+piman for content changes
+nileshagrawal for chrome/android and content/public/android changes

[benm (inactive), 2012-11-15 15:26:02]

The motivation for this change and the associated patches lg2m, but I don't feel
qualified enough in the build system to approve this change...

Thanks for doing this Newton!

[Nico, 2012-11-15 17:43:32]

I'm not qualified to review the ant bits, but it looks like cjhopman did that
already. (TIL that ant supports arbitrary bits of code in several procedural
languages.)

The gyp file changes below chrome lgtm.

Please document this scheme somewhere on http://dev.chromium.org/developers and
possibly link to there from the gypi file.

[nilesh, 2012-11-15 17:49:24]

> +nileshagrawal for chrome/android and content/public/android changes
LGTM for chrome/android and content/public/android

[benm (inactive), 2012-11-15 18:28:29]

android_webview lgtm

[newt (away), 2012-11-15 21:13:49]

On 2012/11/15 17:43:32, Nico wrote:
> I'm not qualified to review the ant bits, but it looks like cjhopman did that
> already. (TIL that ant supports arbitrary bits of code in several procedural
> languages.)
> 
> The gyp file changes below chrome lgtm.
> 
> Please document this scheme somewhere on http://dev.chromium.org/developers
and
> possibly link to there from the gypi file.

Sure.  What part of this change do you think is important to document? 
How/where resources should be added, or how gyp and ant build jars and APKs
using these resources?

[Nico, 2012-11-15 21:18:37]

On 2012/11/15 21:13:49, newt wrote:
> On 2012/11/15 17:43:32, Nico wrote:
> > I'm not qualified to review the ant bits, but it looks like cjhopman did
that
> > already. (TIL that ant supports arbitrary bits of code in several procedural
> > languages.)
> > 
> > The gyp file changes below chrome lgtm.
> > 
> > Please document this scheme somewhere on http://dev.chromium.org/developers
> and
> > possibly link to there from the gypi file.
> 
> Sure.  What part of this change do you think is important to document? 
> How/where resources should be added, or how gyp and ant build jars and APKs
> using these resources?

How the jar / apk stuff works.

[newt (away), 2012-11-15 22:33:50]

+jam for content changes

(looks like piman is OOO)

[jam, 2012-11-15 23:01:00]

lgtm

[commit-bot: I haz the power, 2012-11-15 23:04:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/newt@chromium.org/11363150/13006

[newt (away), 2012-11-16 00:33:20]

On 2012/11/15 21:18:37, Nico wrote:
> On 2012/11/15 21:13:49, newt wrote:
> > On 2012/11/15 17:43:32, Nico wrote:
> > > I'm not qualified to review the ant bits, but it looks like cjhopman did
> that
> > > already. (TIL that ant supports arbitrary bits of code in several
procedural
> > > languages.)
> > > 
> > > The gyp file changes below chrome lgtm.
> > > 
> > > Please document this scheme somewhere on
http://dev.chromium.org/developers
> > and
> > > possibly link to there from the gypi file.
> > 
> > Sure.  What part of this change do you think is important to document? 
> > How/where resources should be added, or how gyp and ant build jars and APKs
> > using these resources?
> 
> How the jar / apk stuff works.

Added documentation here:
http://www.chromium.org/developers/design-documents/java-resources-on-android

[Nico, 2012-11-16 00:36:47]

Looks great, thanks!
On Nov 15, 2012 4:33 PM, <newt@chromium.org> wrote:

> On 2012/11/15 21:18:37, Nico wrote:
>
>> On 2012/11/15 21:13:49, newt wrote:
>> > On 2012/11/15 17:43:32, Nico wrote:
>> > > I'm not qualified to review the ant bits, but it looks like cjhopman
>> did
>> that
>> > > already. (TIL that ant supports arbitrary bits of code in several
>>
> procedural
>
>> > > languages.)
>> > >
>> > > The gyp file changes below chrome lgtm.
>> > >
>> > > Please document this scheme somewhere on
>>
> http://dev.chromium.org/**developers <http://dev.chromium.org/developers>
>
>> > and
>> > > possibly link to there from the gypi file.
>> >
>> > Sure.  What part of this change do you think is important to document?
>> > How/where resources should be added, or how gyp and ant build jars and
>> APKs
>> > using these resources?
>>
>
>  How the jar / apk stuff works.
>>
>
> Added documentation here:
> http://www.chromium.org/**developers/design-documents/**
>
java-resources-on-android<http://www.chromium.org/developers/design-documents/java-resources-on-android>
>
>
https://codereview.chromium.**org/11363150/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2012-11-16 05:09:35]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-11-16 19:07:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/newt@chromium.org/11363150/13006

[commit-bot: I haz the power, 2012-11-16 19:08:00]

Failed to apply patch for build/java_apk.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file build/java_apk.gypi
  Hunk #4 FAILED at 71.
  Hunk #5 succeeded at 126 (offset 9 lines).
  Hunk #6 succeeded at 157 with fuzz 1 (offset 10 lines).
  1 out of 6 hunks FAILED -- saving rejects to file build/java_apk.gypi.rej

Patch:       build/java_apk.gypi
Index: build/java_apk.gypi
diff --git a/build/java_apk.gypi b/build/java_apk.gypi
index
096d8a92e85c8b75e4fa2e7795a58af800cdeb2f..0c341a99b8cb36c81411d4040e6db0612bfe6729
100644
--- a/build/java_apk.gypi
+++ b/build/java_apk.gypi
@@ -24,8 +24,8 @@
 # like:
 #
 # content/shell/android/java/content_shell_apk.xml
-# content/shell/android/java/src/chromium/base/Foo.java
-# content/shell/android/java/src/chromium/base/Bar.java
+# content/shell/android/java/src/org/chromium/base/Foo.java
+# content/shell/android/java/src/org/chromium/base/Bar.java
 #
 # Required variables:
 #  package_name - Used to name the intermediate output directory and in the
@@ -36,6 +36,9 @@
 # Optional/automatic variables:
 #  additional_input_paths - These paths will be included in the 'inputs' list
to
 #    ensure that this target is rebuilt when one of these paths changes.
+#  additional_res_dirs - Additional directories containing Android resources.
+#  additional_res_packages - Package names of the R.java files corresponding to
+#    each directory in additional_res_dirs.
 #  additional_src_dirs - Additional directories with .java files to be compiled
 #    and included in the output of this target.
 #  asset_location - The directory where assets are located (default:
@@ -46,6 +49,8 @@
 #    included in the 'inputs' list (unlike additional_src_dirs).
 #  input_jars_paths - The path to jars to be included in the classpath. This
 #    should be filled automatically by depending on the appropriate targets.
+#  is_test_apk - Set to 1 if building a test apk.  This prevents resources from
+#    dependencies from being re-included.
 #  native_libs_paths - The path to any native library to be included in this
 #    target. This should be a path in <(SHARED_LIB_DIR). A stripped copy of
 #    the library will be included in the apk and symbolic links to the
@@ -66,7 +71,10 @@
     'proguard_flags%': '',
     'native_libs_paths': [],
     'manifest_package_name%': 'unknown.package.name',
-    'resource_dir%':'',
+    'resource_dir%': '',
+    'additional_res_dirs': [],
+    'additional_res_packages': [],
+    'is_test_apk%': 0,
   },
   'sources': [
       '<@(native_libs_paths)'
@@ -112,6 +120,12 @@
         ['resource_dir!=""', {
           'inputs': ['<!@(find <(java_in_dir)/<(resource_dir) -name "*")']
         }],
+        ['is_test_apk == 1', {
+          'variables': {
+            'additional_res_dirs=': [],
+            'additional_res_packages=': [],
+          }
+        }],
       ],
       'outputs': [
         '<(PRODUCT_DIR)/apks/<(apk_name).apk',
@@ -136,6 +150,8 @@
         '-DINPUT_JARS_PATHS=>(input_jars_paths)',
         '-DPACKAGE_NAME=<(package_name)',
         '-DRESOURCE_DIR=<(resource_dir)',
+        '-DADDITIONAL_RES_DIRS=>(additional_res_dirs)',
+        '-DADDITIONAL_RES_PACKAGES=>(additional_res_packages)',
         '-DAPP_MANIFEST_VERSION_NAME=<(app_manifest_version_name)',
         '-DAPP_MANIFEST_VERSION_CODE=<(app_manifest_version_code)',
         '-DPROGUARD_FLAGS=>(proguard_flags)',

[commit-bot: I haz the power, 2012-11-16 20:32:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/newt@chromium.org/11363150/18004