Move ui and content resources into one location.

JarJar resources when building the android_webview.

The android_webview build will require there only be
a single R class (details in bug).
This change makes the necessary changes to the build
infrastructure for this to happen but the actual swap
to building the webview this way will land as a subsequent
CL.

BUG=387155
build-only change, all trybots compile fine.
NOTRY=true

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

[mkosiba (inactive), 2014-04-24 17:14:19]

<blinky text> this is work in progress </blinky text>

So I've started moving the ui/android and content/public resources into one
location and had a couple of observations/hit some issues:
 - Target folder is ui/android/resources, not ideal but src/resources seems like
overkill.

 - Can't build resources without building at least a little bit of Java. I had
to add a fake java file to the ui/android/resources/src folder for this to work.
gross.

 - Strings (grd files) will have to be moved into one file. This seems sucky.

 - we can't merge the chrome/ resources into the content/ resources since the
webview doesn't need them, so we still end up with having at least 2 res/
folders.

I'm beginning to think that maybe it's sufficient to always generate the R.java
file into the same package (org.chromium.resources) but not actually move the
layout/strings/etc.. around. WDYT?

[newt (away), 2014-04-24 17:41:03]

On 2014/04/24 17:14:19, mkosiba wrote:
> <blinky text> this is work in progress </blinky text>
> 
> So I've started moving the ui/android and content/public resources into one
> location and had a couple of observations/hit some issues:
>  - Target folder is ui/android/resources, not ideal but src/resources seems
like
> overkill.
> 
>  - Can't build resources without building at least a little bit of Java. I had
> to add a fake java file to the ui/android/resources/src folder for this to
work.
> gross.
> 
>  - Strings (grd files) will have to be moved into one file. This seems sucky.
> 
>  - we can't merge the chrome/ resources into the content/ resources since the
> webview doesn't need them, so we still end up with having at least 2 res/
> folders.
> 
> I'm beginning to think that maybe it's sufficient to always generate the
R.java
> file into the same package (org.chromium.resources) but not actually move the
> layout/strings/etc.. around. WDYT?

Yea, I'd prefer just changing the Java package to be org.chromium.resources.
Then we have a system that mirrors C++ resources (IDS_*, IDR_*). The resource
namespace is global (there can only be one IDS_FOO string), but the resources
live in various different grd files inside the directories where they're used.

[mkosiba (inactive), 2014-04-25 12:44:07]

+cjhopman
 Chris, I think you wrote some of the build stuff. 

On 2014/04/24 17:41:03, newt wrote:
> Yea, I'd prefer just changing the Java package to be org.chromium.resources.
> Then we have a system that mirrors C++ resources (IDS_*, IDR_*). The resource

I think this change won't help with the shadowing. If you define the same
resource
in 2 layers one will take precedence. This seems to be built into aapt.

> namespace is global (there can only be one IDS_FOO string), but the resources
> live in various different grd files inside the directories where they're used.

So I tried "just" moving everything into one namespace (patchset 2) and hit 2
issues:
 1) now we end up with 2 R.java classes in the same package so the build fails
when
    building the content/ layer (it sees the R.java from ui/android too).
 2) the generated R.java files seem to have different numeric values for the
same
    resources. I don't think it was like that previously. Seems scary.

For 1) I was

[mkosiba (inactive), 2014-04-25 13:10:29]

dammit, hit enter with the focus in the wrong place.

I was wondering whether we could change our R.java files to be
built into separate .jar files (this would also require them not having
final fields I think?).
In such a setup the content/ and ui/ Java code would be built against
the R.java class but not actually include it. Only when building the APK
would we include the R.java class generated for the full set of resources
that are going into the APK.
This would also allow us to generate a stricter R.java file when building
the individual layers: since the ID's won't need to match up the content/
layer could be built against an R.java files generated strictly from the
content/ layer's resources.

For the webview we'd also want to separate the grd -> .xml generation
since we'll want to run just that bit but not appt itself (which we'll
need to manually add rules for in the Android.mk file).
It would be ideal if we could refactor the grd action into a separate
target both the webview and the build/java.gypi rules could depend on.
Does that sound reasonable?

[newt (away), 2014-04-25 16:48:38]

On 2014/04/25 12:44:07, mkosiba wrote:
> +cjhopman
>  Chris, I think you wrote some of the build stuff. 
> 
> On 2014/04/24 17:41:03, newt wrote:
> > Yea, I'd prefer just changing the Java package to be org.chromium.resources.
> > Then we have a system that mirrors C++ resources (IDS_*, IDR_*). The
resource
> 
> I think this change won't help with the shadowing. If you define the same
> resource
> in 2 layers one will take precedence. This seems to be built into aapt.

"Shadowing" happened before and it'll happen after this change. My point is just
that by putting resources in the same namespace, it makes it clear that two
resources with the same name will conflict (in this case, one overrides the
other).

> > namespace is global (there can only be one IDS_FOO string), but the
resources
> > live in various different grd files inside the directories where they're
used.
> 
> So I tried "just" moving everything into one namespace (patchset 2) and hit 2
> issues:
>  1) now we end up with 2 R.java classes in the same package so the build fails
> when
>     building the content/ layer (it sees the R.java from ui/android too).

What if you remove the generated_R_dirs gyp variable from java.gypi? That's how
ui's R.java gets included when you compile content's java files. R.java isn't
included in ui_java.jar or content_java.jar, rather it's supplied to javac via
the --src-gendirs argument in java.gypi.

>  2) the generated R.java files seem to have different numeric values for the
> same
>     resources. I don't think it was like that previously. Seems scary.

Different numeric values for the same resource in different R.java files is fine
and expected as long as the IDs are non-final constants.

[mkosiba (inactive), 2014-04-29 16:55:22]

On 2014/04/25 16:48:38, newt wrote:
> On 2014/04/25 12:44:07, mkosiba wrote:
> > > namespace is global (there can only be one IDS_FOO string), but the
> resources
> > > live in various different grd files inside the directories where they're
> used.
> > 
> > So I tried "just" moving everything into one namespace (patchset 2) and hit
2
> > issues:
> >  1) now we end up with 2 R.java classes in the same package so the build
fails
> > when
> >     building the content/ layer (it sees the R.java from ui/android too).
> 
> What if you remove the generated_R_dirs gyp variable from java.gypi? That's
how
> ui's R.java gets included when you compile content's java files. R.java isn't
> included in ui_java.jar or content_java.jar, rather it's supplied to javac via
> the --src-gendirs argument in java.gypi.

ok, so web_contents_delegate_android doesn't compile after taking this out.
Even if I manually add a generated_R_dirs entry to web_contents_delegate_android
I hit
an error when making the content_shell_apk:

/usr/local/google/code/chromium/src/build/android/ant/apk-codegen.xml:91:
project.library.packages and project.library.bin.r.file.path must contain the
same number of items.

I'm going to try and create a separate ui_java_resourceless target that doesn't
export generated_R_dirs but this feels a bit like fighting the build system.

[mkosiba (inactive), 2014-04-30 18:34:55]

On 2014/04/29 16:55:22, mkosiba wrote:
> I'm going to try and create a separate ui_java_resourceless target that
doesn't
> export generated_R_dirs but this feels a bit like fighting the build system.

Soo... turns out all_dependent_settings are magic and overriding their values
didn't
seem to work.
I changed ui_java to export the R stuff as direct_dependen_settings and it seems
to
work, although I found I had to nuke out to make the out/Debug/..._apk/gen
folder
contain the right R.java file. Could just be a result of me meddling with the 
gyp files too much.
Anyway - wdyt about this 'solution'?

[mkosiba (inactive), 2014-05-06 20:48:25]

On 2014/04/30 18:34:55, mkosiba wrote:
> Soo... turns out all_dependent_settings are magic and overriding their values
> didn't
> seem to work.

Just to be more precise here: it wasn't possible to change the values 'exported'
by all_dependent_settings from the build/java.gypi by specifying different
all_dependent_settings values on the ui_java target. It seemed like the
resulting
lists got merged, not overwritten (which is WAI I guess..)

[newt (away), 2014-05-07 04:39:50]

What happens if a Java target (A) directly depends on two other Java targets (B
and C) that contain resources? Target A will get two different R.java files
(both org.chromium.resources.R) passed in via generated_R_dirs, and will fail to
compile. Right?

One way around this problem would be for target A itself to run aapt on the Java
resources inherited from B and C and generate a single R.java containing the
union of those resources. This would result in duplicate runs of aapt though:
e.g. autofill.gypi would re-process content's resources as would
web_contents_delegate_android.gypi.

Another idea that Chris mentioned is to build everything using the existing
system, and use jarjar at the very end to rename all the R packages to
org.chromium.ui.resources.R. I don't know much about jarjar, and whether this is
possible, but if it is, it seems like it might be the easiest solution.

Am I understanding the problem correctly?

[mkosiba (inactive), 2014-05-07 18:51:47]

On 2014/05/07 04:39:50, newt wrote:
> What happens if a Java target (A) directly depends on two other Java targets
(B
> and C) that contain resources? Target A will get two different R.java files
> (both org.chromium.resources.R) passed in via generated_R_dirs, and will fail
to
> compile. Right?

Right. That kinda sucks.

> One way around this problem would be for target A itself to run aapt on the
Java
> resources inherited from B and C and generate a single R.java containing the
> union of those resources. This would result in duplicate runs of aapt though:
> e.g. autofill.gypi would re-process content's resources as would
> web_contents_delegate_android.gypi.

We have about 50 java targets in Chromium, that sounds like a lot of aapt
invocations...

How about a variation of what I proposed in #4 (btw. does #4 make any sense?):
 1) split build/java.gypi into:
   - build/java_javac.gypi
   - build/java_resources.gypi
   - build/java_grd.gypi*
   - add build/java.gypi that includes the files above,
 2) split both content_java and ui_java into separate java/resources rules,
 3) define a resource-only rule that the content and ui java rules depend on,
 4) curse how complex the build is now,
 5) profit?

This would essentially mean that building 'all' of the resources is a
prerequisite
of building any of the java. The upside would be that 

* not needed for the resource rename but I need separate rules for generating
strings.xml from grd files for the webview build anyway...

> Another idea that Chris mentioned is to build everything using the existing
> system, and use jarjar at the very end to rename all the R packages to
> org.chromium.ui.resources.R. I don't know much about jarjar, and whether this
is
> possible, but if it is, it seems like it might be the easiest solution.

Even if it's possible it'll be a bit fiddly to maintain - if we add an extra R
class
the content layer (and don't reflect that in the jarjar rules) this will
continue to
compile and run on Clank (and the webview test shell) but will fail when the
webview
is loaded in the system image.

It is tempting though and not a lot of work to see if it works. I'll give it a
shot
and report back.

[cjhopman, 2014-05-07 19:37:07]

First, do we understand why Android can't support what we are doing?

Second, I really don't like the idea of putting content and ui resources in the
same package but keeping everything else in a separate package -- both because
it's inconsistent and because it's much, much harder to support both ways of
doing resources.

Third, I would prefer that we handle resources in whatever way we think is best
for development (as long as there is a simple way to handle the Android
limitations). This was why I suggested the jarjar all the things approach
(hoping that it is simple enough).

On 2014/05/07 18:51:47, mkosiba wrote:
> On 2014/05/07 04:39:50, newt wrote:
> > What happens if a Java target (A) directly depends on two other Java targets
> (B
> > and C) that contain resources? Target A will get two different R.java files
> > (both org.chromium.resources.R) passed in via generated_R_dirs, and will
fail
> to
> > compile. Right?
> 
> Right. That kinda sucks.
> 
> > One way around this problem would be for target A itself to run aapt on the
> Java
> > resources inherited from B and C and generate a single R.java containing the
> > union of those resources. This would result in duplicate runs of aapt
though:
> > e.g. autofill.gypi would re-process content's resources as would
> > web_contents_delegate_android.gypi.
> 
> We have about 50 java targets in Chromium, that sounds like a lot of aapt
> invocations...

Agreed, but I don't actually think that this approach would add that many new
aapt invocations. And any that it does add, are probably invocations that should
be done anyway (because of the fact that we already do #4, for a java target to
refer to resources in its dependents it has to invoke aapt).

> 
> How about a variation of what I proposed in #4 (btw. does #4 make any sense?):

My understanding is that we already do #4 since aapt can't assign IDs until it
knows all the resources in the apk (and aapt has a flag for generating the
R.java files with non-final fields and no value). See jar_excluded_classes in
build/java.gypi.

>  1) split build/java.gypi into:
>    - build/java_javac.gypi
>    - build/java_resources.gypi
>    - build/java_grd.gypi*
>    - add build/java.gypi that includes the files above,
>  2) split both content_java and ui_java into separate java/resources rules,

It's not that easy. If you are going to take this approach, it would be much
simpler to split resources out of every java.gypi target (and ideally
java_apk.gypi target). I actually have long wanted to do that, but there's just
been little benefit.

>  3) define a resource-only rule that the content and ui java rules depend on,
>  4) curse how complex the build is now,
>  5) profit?
> 
> This would essentially mean that building 'all' of the resources is a
> prerequisite
> of building any of the java. The upside would be that 
"Would be that" what?

I think that having all java targets depend on the top of the resources tree is
just not going to happen. With the splitting approach and generating everything
into a single package, aapt invocations would have to happen in the java target
(with different packages the resources target could do the aapt invocation).

> 
> * not needed for the resource rename but I need separate rules for generating
> strings.xml from grd files for the webview build anyway...
> 
> > Another idea that Chris mentioned is to build everything using the existing
> > system, and use jarjar at the very end to rename all the R packages to
> > org.chromium.ui.resources.R. I don't know much about jarjar, and whether
this
> is
> > possible, but if it is, it seems like it might be the easiest solution.
> 
> Even if it's possible it'll be a bit fiddly to maintain - if we add an extra R
> class
> the content layer (and don't reflect that in the jarjar rules) this will
> continue to
> compile and run on Clank (and the webview test shell) but will fail when the
> webview
> is loaded in the system image.

I think it is easy to handle this aspect of maintaining it (i.e. making sure all
the generated packages are specified). You can just have the jarjar rules
generated by the build (where it is easy to know what resources are being
packaged).

> 
> It is tempting though and not a lot of work to see if it works. I'll give it a
> shot
> and report back.

[mkosiba (inactive), 2014-05-07 22:40:21]

I just had a meeting with Newt on this. The conclusions we came to are:
a) it may be possible for the content gyp target to replace the
all_dependent_setting variables coming from ui_java with its own values, I
was going to check if this is possible (and works for the diamond
dependency case)
b) it's unclear what is the reason for the ui/android content/ split for
java code, from looking at the kind of stuff that's in these folders it
seems like all of the ui/android code could equally well live in content/,
so maybe physically moving stuff from ui/ to content/ is an option,
c) given the above we were thinking of changing ui_java to:
 - run aapt for the ui/ resources
 - compile java coude against R.java generated from that step
 - run aapt for ui + content resources and put that into generated_R_dir
(or whatever the name of that variable is)

On Wed, May 7, 2014 at 8:37 PM, <cjhopman@chromium.org> wrote:

> First, do we understand why Android can't support what we are doing?
>
Out-of-band communication sent.


>
> Second, I really don't like the idea of putting content and ui resources
> in the
> same package but keeping everything else in a separate package -- both
> because
> it's inconsistent and because it's much, much harder to support both ways
> of
> doing resources.
>
which both ways?


>
> Third, I would prefer that we handle resources in whatever way we think is
> best
> for development (as long as there is a simple way to handle the Android
> limitations). This was why I suggested the jarjar all the things approach
> (hoping that it is simple enough).


I'm not sure jarjar will let us squash 2 classes. The rules would have to
be:

ui.R -> resources.R
zap resources.R
content.R -> resources.R

I'll try that out tomorrow.


>
>
> On 2014/05/07 18:51:47, mkosiba wrote:
>
>> On 2014/05/07 04:39:50, newt wrote:
>> > What happens if a Java target (A) directly depends on two other Java
>> targets
>> (B
>> > and C) that contain resources? Target A will get two different R.java
>> files
>> > (both org.chromium.resources.R) passed in via generated_R_dirs, and will
>>
> fail
>
>> to
>> > compile. Right?
>>
>
>  Right. That kinda sucks.
>>
>
>  > One way around this problem would be for target A itself to run aapt on
>> the
>> Java
>> > resources inherited from B and C and generate a single R.java
>> containing the
>> > union of those resources. This would result in duplicate runs of aapt
>>
> though:
>
>> > e.g. autofill.gypi would re-process content's resources as would
>> > web_contents_delegate_android.gypi.
>>
>
>  We have about 50 java targets in Chromium, that sounds like a lot of aapt
>> invocations...
>>
>
> Agreed, but I don't actually think that this approach would add that many
> new
> aapt invocations. And any that it does add, are probably invocations that
> should
> be done anyway (because of the fact that we already do #4, for a java
> target to
> refer to resources in its dependents it has to invoke aapt).


>
>
>  How about a variation of what I proposed in #4 (btw. does #4 make any
>> sense?):
>>
>
> My understanding is that we already do #4 since aapt can't assign IDs
> until it
> knows all the resources in the apk (and aapt has a flag for generating the
> R.java files with non-final fields and no value). See jar_excluded_classes
> in
> build/java.gypi.


We do something with the same effect. We add the R.java file to the list of
javac inputs
and exclude it from the resulting jar.
Regardless, #4 moves the problem around - now we have 2
org.chromium.resources.R
in the classpath.


>
>
>    1) split build/java.gypi into:
>>     - build/java_javac.gypi
>>     - build/java_resources.gypi
>>     - build/java_grd.gypi*
>>     - add build/java.gypi that includes the files above,
>>   2) split both content_java and ui_java into separate java/resources
>> rules,
>>
>
> It's not that easy. If you are going to take this approach, it would be
> much
> simpler to split resources out of every java.gypi target (and ideally
> java_apk.gypi target). I actually have long wanted to do that, but there's
> just
> been little benefit.
>
>
>    3) define a resource-only rule that the content and ui java rules
>> depend on,
>>   4) curse how complex the build is now,
>>   5) profit?
>>
>
>  This would essentially mean that building 'all' of the resources is a
>> prerequisite
>> of building any of the java. The upside would be that
>>
> "Would be that" what?
>
ah, sorry. would be that it is closer conceptually to what I'm trying to do
- that is
stick both ui and content resources into the same target.


>
> I think that having all java targets depend on the top of the resources
> tree is
> just not going to happen. With the splitting approach and generating
> everything
>

this would be just content and ui (although I agree that "just" tends to
grow over time).


> into a single package, aapt invocations would have to happen in the java
> target
> (with different packages the resources target could do the aapt
> invocation).


>

>  * not needed for the resource rename but I need separate rules for
>> generating
>> strings.xml from grd files for the webview build anyway...
>>
>
>  > Another idea that Chris mentioned is to build everything using the
>> existing
>> > system, and use jarjar at the very end to rename all the R packages to
>> > org.chromium.ui.resources.R. I don't know much about jarjar, and whether
>>
> this
>
>> is
>> > possible, but if it is, it seems like it might be the easiest solution.
>>
>
>  Even if it's possible it'll be a bit fiddly to maintain - if we add an
>> extra R
>> class
>> the content layer (and don't reflect that in the jarjar rules) this will
>> continue to
>> compile and run on Clank (and the webview test shell) but will fail when
>> the
>> webview
>> is loaded in the system image.
>>
>
> I think it is easy to handle this aspect of maintaining it (i.e. making
> sure all
> the generated packages are specified). You can just have the jarjar rules
> generated by the build (where it is easy to know what resources are being
> packaged).


ok. it is not as simple as that since the jarjar invocation will have to
happen from
an Android.mk file. We could have a gyp rule that the Android.mk target
depends
on, however this would require special android_webview_build-only rules
that don't
try and compile any java or run aapt.


>
>
>  It is tempting though and not a lot of work to see if it works. I'll give
>> it a
>> shot
>> and report back.
>>
>
>
> https://codereview.chromium.org/255613003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mkosiba (inactive), 2014-05-08 17:19:42]

ok, sweet! looks like it's totally possible to do this with jarjar. Generating
the jarjar rules from gyp will be a follow-up.

Also, would we want to do this for all targets to remove duplicated R classes?

[newt (away), 2014-05-08 17:33:16]

On 2014/05/08 17:19:42, mkosiba wrote:
> ok, sweet! looks like it's totally possible to do this with jarjar. Generating
> the jarjar rules from gyp will be a follow-up.

Sweet :)

What do we need to generate the jarjar rules from gyp for? I was thinking that
the package renaming would happen only when building webview via Android.mk
files.

> Also, would we want to do this for all targets to remove duplicated R classes?

My feeling is that this might introduce more complexity than benefit. In theory,
proguard could remove our R classes anyway (if we enabled it). If this is super
simple and localized, though, then maybe.

[mkosiba (inactive), 2014-05-08 18:15:07]

On 2014/05/08 17:33:16, newt wrote:
> On 2014/05/08 17:19:42, mkosiba wrote:
> > ok, sweet! looks like it's totally possible to do this with jarjar.
Generating
> > the jarjar rules from gyp will be a follow-up.
> 
> Sweet :)
> 
> What do we need to generate the jarjar rules from gyp for? I was thinking that
> the package renaming would happen only when building webview via Android.mk
> files.

Yes, the jar-jar rule will only run from the Android.mk file. The issue I want
to avoid is someone adding a new R file that the content layer depends on but
not updating the jar-jar rules to match. I don't see this as likely but the
less things we keep in sync by hand, the better.

> 
> > Also, would we want to do this for all targets to remove duplicated R
classes?
> 
> My feeling is that this might introduce more complexity than benefit. In
theory,
> proguard could remove our R classes anyway (if we enabled it). If this is
super
> simple and localized, though, then maybe.

Ah, having proguard do that sounds like a more interesting option then. I'm not
sure
that would be super-simple.

[mkosiba (inactive), 2014-05-12 17:35:49]

Hey guys, PTAL (please take another look ;)

newt/cjhopman - please review the build changes. I had to a thing to jar-jar
layout.xml files since those reference class names.

torne - please review android_webview changes + take a look at the gyp stuff if
you can

bulach - ptal at jni generator changes

https://codereview.chromium.org/255613003/diff/100001/build/android/gyp/jarja...
File build/android/gyp/jarjar_resources.py (right):

https://codereview.chromium.org/255613003/diff/100001/build/android/gyp/jarja...
build/android/gyp/jarjar_resources.py:14: class JarJarRules(object):
I know this is duplicated from the jni generator. I spoke about this with Marcus
and because the jni generator is used elsewhere (outside of Chrome codebase)
it's simpler to duplicate the code than to refactor the generator to allow for
reuse.

[mkosiba (inactive), 2014-05-12 17:36:51]

+ torne and bulach for real this time

torne - please review android_webview changes + take a look at the gyp stuff if
you can

bulach - ptal at jni generator changes

[newt (away), 2014-05-12 20:11:22]

lgtm after a few small comments. Didn't look at the Android.mk file.

https://codereview.chromium.org/255613003/diff/100001/android_webview/android...
File android_webview/android_webview.gyp (right):

https://codereview.chromium.org/255613003/diff/100001/android_webview/android...
android_webview/android_webview.gyp:263: 'android_unmangled_name': 1,
android_unmangled_name never seems to be used. Remove this?

https://codereview.chromium.org/255613003/diff/100001/android_webview/build/j...
File android_webview/build/jarjar_resources.gypi (right):

https://codereview.chromium.org/255613003/diff/100001/android_webview/build/j...
android_webview/build/jarjar_resources.gypi:34: 'inputs': [
add rules file to inputs

https://codereview.chromium.org/255613003/diff/100001/build/android/gyp/jarja...
File build/android/gyp/jarjar_resources.py (right):

https://codereview.chromium.org/255613003/diff/100001/build/android/gyp/jarja...
build/android/gyp/jarjar_resources.py:24: 
nit: remove extra empty line

[bulach, 2014-05-13 08:42:39]

lgtm, tiny suggestions below:

https://codereview.chromium.org/255613003/diff/100001/build/android/gyp/jarja...
File build/android/gyp/jarjar_resources.py (right):

https://codereview.chromium.org/255613003/diff/100001/build/android/gyp/jarja...
build/android/gyp/jarjar_resources.py:13: 
nit: need another \n, i.e., two between top-levels (ditto fro RenameNodes,
ProcessLayoutFile and the other functions below...)

https://codereview.chromium.org/255613003/diff/100001/build/android/gyp/jarja...
build/android/gyp/jarjar_resources.py:31: print self._rules
nit: maybe remove this print?

https://codereview.chromium.org/255613003/diff/100001/build/android/gyp/jarja...
build/android/gyp/jarjar_resources.py:58: return {name for name in names if
name.endswith('.xml')}
nit: perhaps:
filter(lambda n: n.endswith('.xml'), names)

https://codereview.chromium.org/255613003/diff/100001/build/android/gyp/jarja...
build/android/gyp/jarjar_resources.py:75: parser.add_option('--input-dir',
action="append",
nit: s/"/'/

https://codereview.chromium.org/255613003/diff/100001/build/android/gyp/jarja...
build/android/gyp/jarjar_resources.py:78: help='Directory to hold processed
resources.')
nit: perhaps add a note saying it'll be clobbered?

[Torne, 2014-05-13 13:46:51]

android_webview/ and general gyp stuff looks reasonable in general. Just one
comment...

https://codereview.chromium.org/255613003/diff/100001/android_webview/android...
File android_webview/android_webview.gyp (right):

https://codereview.chromium.org/255613003/diff/100001/android_webview/android...
android_webview/android_webview.gyp:263: 'android_unmangled_name': 1,
On 2014/05/12 20:11:23, newt wrote:
> android_unmangled_name never seems to be used. Remove this?

This isn't a varible, it's a parameter for the generator, which disables the
normal name mangling applied to gyp targets. This makes it easier to refer to
the gyp-generated target from the Android.mk.

https://codereview.chromium.org/255613003/diff/100001/android_webview/build/j...
File android_webview/build/jarjar-rules.txt (right):

https://codereview.chromium.org/255613003/diff/100001/android_webview/build/j...
android_webview/build/jarjar-rules.txt:1: zap org.chromium.ui.R**
Is a ** the right thing here? Isn't that going to zap/rename anything whose name
happens to start with an R in these packages? Do we maybe want an explicit rule
for R, and then a separate one for R.** instead?

[cjhopman, 2014-05-13 16:41:31]

lgtm, with a couple small nitty things

https://codereview.chromium.org/255613003/diff/100001/android_webview/build/j...
File android_webview/build/jarjar_resources.gypi (right):

https://codereview.chromium.org/255613003/diff/100001/android_webview/build/j...
android_webview/build/jarjar_resources.gypi:9: # {
Update this example usage.

https://codereview.chromium.org/255613003/diff/100001/build/android/gyp/jarja...
File build/android/gyp/jarjar_resources.py (right):

https://codereview.chromium.org/255613003/diff/100001/build/android/gyp/jarja...
build/android/gyp/jarjar_resources.py:5: 
Nit: add a file-level docstring (as long or short as you feel is appropriate)

[mkosiba (inactive), 2014-06-12 11:11:19]

Marcus, I needed to make more JNI generator changes. Could you take a quick
look?

https://codereview.chromium.org/255613003/diff/100001/android_webview/build/j...
File android_webview/build/jarjar-rules.txt (right):

https://codereview.chromium.org/255613003/diff/100001/android_webview/build/j...
android_webview/build/jarjar-rules.txt:1: zap org.chromium.ui.R**
On 2014/05/13 13:46:52, Torne wrote:
> Is a ** the right thing here? Isn't that going to zap/rename anything whose
name
> happens to start with an R in these packages? Do we maybe want an explicit
rule
> for R, and then a separate one for R.** instead?

Good point. The resources are inner classes so we need a rule for R and a
separate one for R$

https://codereview.chromium.org/255613003/diff/100001/build/android/gyp/jarja...
File build/android/gyp/jarjar_resources.py (right):

https://codereview.chromium.org/255613003/diff/100001/build/android/gyp/jarja...
build/android/gyp/jarjar_resources.py:5: 
On 2014/05/13 16:41:31, cjhopman wrote:
> Nit: add a file-level docstring (as long or short as you feel is appropriate)

Done.

https://codereview.chromium.org/255613003/diff/100001/build/android/gyp/jarja...
build/android/gyp/jarjar_resources.py:13: 
On 2014/05/13 08:42:39, bulach wrote:
> nit: need another \n, i.e., two between top-levels (ditto fro RenameNodes,
> ProcessLayoutFile and the other functions below...)

Done.

https://codereview.chromium.org/255613003/diff/100001/build/android/gyp/jarja...
build/android/gyp/jarjar_resources.py:24: 
On 2014/05/12 20:11:23, newt wrote:
> nit: remove extra empty line

Done.

https://codereview.chromium.org/255613003/diff/100001/build/android/gyp/jarja...
build/android/gyp/jarjar_resources.py:31: print self._rules
On 2014/05/13 08:42:39, bulach wrote:
> nit: maybe remove this print?

Done.

https://codereview.chromium.org/255613003/diff/100001/build/android/gyp/jarja...
build/android/gyp/jarjar_resources.py:58: return {name for name in names if
name.endswith('.xml')}
On 2014/05/13 08:42:39, bulach wrote:
> nit: perhaps:
> filter(lambda n: n.endswith('.xml'), names)

Done.

https://codereview.chromium.org/255613003/diff/100001/build/android/gyp/jarja...
build/android/gyp/jarjar_resources.py:75: parser.add_option('--input-dir',
action="append",
On 2014/05/13 08:42:39, bulach wrote:
> nit: s/"/'/

Done.

https://codereview.chromium.org/255613003/diff/100001/build/android/gyp/jarja...
build/android/gyp/jarjar_resources.py:78: help='Directory to hold processed
resources.')
On 2014/05/13 08:42:39, bulach wrote:
> nit: perhaps add a note saying it'll be clobbered?

Done.

[bulach, 2014-06-12 12:46:32]

lgtm for jni changes, thanks!

[mkosiba (inactive), 2014-06-12 22:35:14]

cjhopman@, could you take a look at build/java_strings_grd.gypi?
The webview doesn't need the zip step and it was failing on our build because
the zip.py script didn't like getting an absolute path.

[cjhopman, 2014-06-12 22:56:56]

On 2014/06/12 22:35:14, mkosiba wrote:
> cjhopman@, could you take a look at build/java_strings_grd.gypi?
> The webview doesn't need the zip step and it was failing on our build because
> the zip.py script didn't like getting an absolute path.

Hm, I don't know enough about how the webview build works to say if using zip or
not zip will even work as expected. Does webview use our process_resources? Or
do they use some standard android way of doing resources?

That being said, those absolute paths should have been fine. In DoZip() where we
do CheckZipPath(f) we should be doing CheckZipPath(os.path.relpath(f,
base_dir)). That is, we should be checking the path in the archive not the path
to the file.

[mkosiba (inactive), 2014-06-13 17:18:40]

On 2014/06/12 22:56:56, cjhopman wrote:
> On 2014/06/12 22:35:14, mkosiba wrote:
> > cjhopman@, could you take a look at build/java_strings_grd.gypi?
> > The webview doesn't need the zip step and it was failing on our build
because
> > the zip.py script didn't like getting an absolute path.
> 
> Hm, I don't know enough about how the webview build works to say if using zip
or
> not zip will even work as expected. Does webview use our process_resources? Or
> do they use some standard android way of doing resources?

The Android.mk files have a LOCAL_RESOURCE_DIR where we pass in the
intermediates dirs
for the ui_strings_grd and content_strings_grd targets, so we don't really need
the .zip
files for anything.
The only thing we the webview needs here is the .grd -> strings.xml step.

> 
> That being said, those absolute paths should have been fine. In DoZip() where
we
> do CheckZipPath(f) we should be doing CheckZipPath(os.path.relpath(f,
> base_dir)). That is, we should be checking the path in the archive not the
path
> to the file.

Ah, ok, that makes sense, thanks. This change fixes the problem so I removed the
android_webview_build condition from the gyp file.

[cjhopman, 2014-06-13 17:40:10]

lgtm

[mkosiba (inactive), 2014-06-20 16:45:31]

jam, could you stamp the content.gyp change please?

[Torne, 2014-06-20 17:20:54]

Android makefile changes LGTM

[mkosiba (inactive), 2014-06-20 19:11:39]

The CQ bit was checked by mkosiba@chromium.org

[commit-bot: I haz the power, 2014-06-20 19:13:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mkosiba@chromium.org/255613003/280001

[mkosiba (inactive), 2014-06-20 19:23:35]

The CQ bit was checked by mkosiba@chromium.org

[commit-bot: I haz the power, 2014-06-20 19:27:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mkosiba@chromium.org/255613003/300001

[jam, 2014-06-20 20:05:35]

lgtm

[mkosiba (inactive), 2014-06-20 22:17:41]

The CQ bit was unchecked by mkosiba@chromium.org

[mkosiba (inactive), 2014-06-20 22:18:51]

The CQ bit was checked by mkosiba@chromium.org

[commit-bot: I haz the power, 2014-06-20 22:21:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mkosiba@chromium.org/255613003/300001

[commit-bot: I haz the power, 2014-06-20 22:47:25]

Change committed as 278849