[Android] Auto-generate only necessary v14 resources.

[Android] Auto-generate only necessary v14 resources.

V14 Resource auto-generation was introduced by crbug.com/235118.
However, some resources doesn't use v17 attributes, so we don't
need to generate for all resources.

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

[newt (away), 2013-05-10 17:10:00]

a few small comments.

also: could you measure the impact of this change on APK size? It probably isn't
much, but it'd be nice to know.

https://codereview.chromium.org/14812015/diff/3001/build/android/gyp/generate...
File build/android/gyp/generate_v14_overlay_resources.py (right):

https://codereview.chromium.org/14812015/diff/3001/build/android/gyp/generate...
build/android/gyp/generate_v14_overlay_resources.py:67: def
WarnDeprecatedAttribute(name, value, filename):
I'd change this function name to "WarnIfDeprecatedAttribute()", so it's clear
that it usually *doesn't* emit a warning

https://codereview.chromium.org/14812015/diff/3001/build/android/gyp/generate...
build/android/gyp/generate_v14_overlay_resources.py:92: # Convert any other API
17 Start/End attributes to Left/Right attributes.
what does "other" mean? I'd remove this now.

https://codereview.chromium.org/14812015/diff/3001/build/android/gyp/generate...
build/android/gyp/generate_v14_overlay_resources.py:130: def
GenerateV14Resource(input_filename,
slightly crazy functional idea:

you could pass GenerateV14StyleResource or GenerateV14LayoutResource as an
argument called "generateResourceFunction" to this method. then you could
replace lines 143-147 with:

    generateResourceFunction(dom, input_filename)

I probably wouldn't actually do this though...

I would add a comment though explaining that line 145 corresponds to XML files
in "values" and line 147 is for XML files in "layouts" or (sadly) "xml"

https://codereview.chromium.org/14812015/diff/3001/build/java.gypi
File build/java.gypi (right):

https://codereview.chromium.org/14812015/diff/3001/build/java.gypi#newcode86
build/java.gypi:86: 'res_v14_overlay_stamp':
'<(intermediate_dir)/res_v14.stamp',
why add "overlay"?  I'm not saying you shouldn't, but I am curious why.

[Kibeom Kim (inactive), 2013-05-10 17:46:14]

https://codereview.chromium.org/14812015/diff/3001/build/android/gyp/generate...
File build/android/gyp/generate_v14_overlay_resources.py (right):

https://codereview.chromium.org/14812015/diff/3001/build/android/gyp/generate...
build/android/gyp/generate_v14_overlay_resources.py:67: def
WarnDeprecatedAttribute(name, value, filename):
On 2013/05/10 17:10:01, newt wrote:
> I'd change this function name to "WarnIfDeprecatedAttribute()", so it's clear
> that it usually *doesn't* emit a warning

Done.

https://codereview.chromium.org/14812015/diff/3001/build/android/gyp/generate...
build/android/gyp/generate_v14_overlay_resources.py:92: # Convert any other API
17 Start/End attributes to Left/Right attributes.
On 2013/05/10 17:10:01, newt wrote:
> what does "other" mean? I'd remove this now.

Done.

https://codereview.chromium.org/14812015/diff/3001/build/android/gyp/generate...
build/android/gyp/generate_v14_overlay_resources.py:130: def
GenerateV14Resource(input_filename,
Yeah. I like what you suggested and looks more elegant. But maybe decrease
readability slightly? Because this is not a huge logic, I'll keep plain.

Added comments at 145, 147. Done.


On 2013/05/10 17:10:01, newt wrote:
> slightly crazy functional idea:
> 
> you could pass GenerateV14StyleResource or GenerateV14LayoutResource as an
> argument called "generateResourceFunction" to this method. then you could
> replace lines 143-147 with:
> 
>     generateResourceFunction(dom, input_filename)
> 
> I probably wouldn't actually do this though...
> 
> I would add a comment though explaining that line 145 corresponds to XML files
> in "values" and line 147 is for XML files in "layouts" or (sadly) "xml"

https://codereview.chromium.org/14812015/diff/3001/build/java.gypi
File build/java.gypi (right):

https://codereview.chromium.org/14812015/diff/3001/build/java.gypi#newcode86
build/java.gypi:86: 'res_v14_overlay_stamp':
'<(intermediate_dir)/res_v14.stamp',
Actually, I don't like this 'overlay' naming either. But If I keep 'res_v14/'
dir name, because now I'm copying/generating in a single directory,
'layout-v17/' will also under 'res_v14' which feels wrong.

copying and generating are now coupled so I think they deserve a single output
directory, but I couldn't come up with a good naming. Any suggestion?

On 2013/05/10 17:10:01, newt wrote:
> why add "overlay"?  I'm not saying you shouldn't, but I am curious why.

[Kibeom Kim (inactive), 2013-05-10 17:52:46]

note: this CL reduced apk size by 52k. The initial autogeneration CL added
~100k.

[newt (away), 2013-05-10 18:06:27]

sweet, that's terrabytes of bandwidth saved by users :)

https://codereview.chromium.org/14812015/diff/3001/build/android/gyp/generate...
File build/android/gyp/generate_v14_overlay_resources.py (right):

https://codereview.chromium.org/14812015/diff/3001/build/android/gyp/generate...
build/android/gyp/generate_v14_overlay_resources.py:130: def
GenerateV14Resource(input_filename,
On 2013/05/10 17:46:14, Kibeom Kim wrote:
> Yeah. I like what you suggested and looks more elegant. But maybe decrease
> readability slightly? Because this is not a huge logic, I'll keep plain.
> 
> Added comments at 145, 147. Done.
> 
> 
> On 2013/05/10 17:10:01, newt wrote:
> > slightly crazy functional idea:
> > 
> > you could pass GenerateV14StyleResource or GenerateV14LayoutResource as an
> > argument called "generateResourceFunction" to this method. then you could
> > replace lines 143-147 with:
> > 
> >     generateResourceFunction(dom, input_filename)
> > 
> > I probably wouldn't actually do this though...
> > 
> > I would add a comment though explaining that line 145 corresponds to XML
files
> > in "values" and line 147 is for XML files in "layouts" or (sadly) "xml"
> 

thanks. I agree about the decrease in readability of the other option,
especially since it's python and the types aren't clear at all.

https://codereview.chromium.org/14812015/diff/3001/build/android/gyp/generate...
build/android/gyp/generate_v14_overlay_resources.py:179: 'used to generate v14
resources')
be sure to update these:

"v14 compatible resources"

https://codereview.chromium.org/14812015/diff/3001/build/java.gypi
File build/java.gypi (right):

https://codereview.chromium.org/14812015/diff/3001/build/java.gypi#newcode86
build/java.gypi:86: 'res_v14_overlay_stamp':
'<(intermediate_dir)/res_v14.stamp',
On 2013/05/10 17:46:14, Kibeom Kim wrote:
> copying and generating are now coupled so I think they deserve a single output
> directory, but I couldn't come up with a good naming. Any suggestion?

maybe "res_v14_compatibility" or "res_v14_support"? that's really what it is
now. if we leave out the directory, we lose v14 compatibility.

[Kibeom Kim (inactive), 2013-05-10 18:42:55]

Renamed overlay to compatibility

https://codereview.chromium.org/14812015/diff/3001/build/android/gyp/generate...
File build/android/gyp/generate_v14_overlay_resources.py (right):

https://codereview.chromium.org/14812015/diff/3001/build/android/gyp/generate...
build/android/gyp/generate_v14_overlay_resources.py:179: 'used to generate v14
resources')
On 2013/05/10 18:06:27, newt wrote:
> be sure to update these:
> 
> "v14 compatible resources"

Done.

[newt (away), 2013-05-10 18:47:33]

lgtm (1 nit)

https://codereview.chromium.org/14812015/diff/16001/build/android/gyp/generat...
File build/android/gyp/generate_v14_compatible_resources.py (right):

https://codereview.chromium.org/14812015/diff/16001/build/android/gyp/generat...
build/android/gyp/generate_v14_compatible_resources.py:186: 'v14 resources will
be generated')
add "compatible" here

[Kibeom Kim (inactive), 2013-05-10 18:50:56]

https://codereview.chromium.org/14812015/diff/16001/build/android/gyp/generat...
File build/android/gyp/generate_v14_compatible_resources.py (right):

https://codereview.chromium.org/14812015/diff/16001/build/android/gyp/generat...
build/android/gyp/generate_v14_compatible_resources.py:186: 'v14 resources will
be generated')
On 2013/05/10 18:47:33, newt wrote:
> add "compatible" here

Done.

[commit-bot: I haz the power, 2013-05-10 18:51:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/14812015/16004

[commit-bot: I haz the power, 2013-05-10 23:11:39]

Change committed as 199564