[Android] Auto-generate v14 style xml resources from the exisitng v17 style resources.

[Android] Auto-generate v14 style xml resources from the exisitng v17 style resources.

In addition to the previous v14 layout xml auto-generation patch,
(https://codereview.chromium.org/14476011/#ps69001)
this patch adds style xml resource auto-generation.

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

[Kibeom Kim (inactive), 2013-05-06 11:39:45]

Additional resource auto-generation. Ready for review.

[newt (away), 2013-05-06 16:59:34]

a couple larger questions:
 - Do style resources cause crashes on pre-17 devices, or do they work fine as
long as we include "paddingLeft" and "paddingStart"
 - how long does these build steps take to run? we could consider optimizing 
GenerateV14StyleResources() by only looking at styles.xml, instead of
considering all values/*.xml files

https://codereview.chromium.org/14948008/diff/1001/build/android/gyp/copy_v17...
File build/android/gyp/copy_v17_resources.py (right):

https://codereview.chromium.org/14948008/diff/1001/build/android/gyp/copy_v17...
build/android/gyp/copy_v17_resources.py:89: CopyXmlResourcesInDir(input_dir,
output_dir, True)
I'd add "only_styles=" for clarity:

    CopyXmlResourcesInDir(input_dir, output_dir, only_styles=True)

https://codereview.chromium.org/14948008/diff/1001/build/android/gyp/generate...
File build/android/gyp/generate_v14_resources.py (right):

https://codereview.chromium.org/14948008/diff/1001/build/android/gyp/generate...
build/android/gyp/generate_v14_resources.py:66: def
GenerateV14StyleXmlResource(dom, output_file):
I'd remove "Xml" from the function name here and below, since it's completely
redundant.

https://codereview.chromium.org/14948008/diff/1001/build/android/gyp/generate...
build/android/gyp/generate_v14_resources.py:75: if name in ATTRIBUTES_TO_MAP:
technically you should check the namespace too.

    name = item_element.attributes['name'].value.split(':')
    if name in ATTRIBUTES_TO_MAP_NS:
      mapped_name = ATTRIBUTES_TO_MAP_NS[name]
      item_element.attributes['name'] = ':'.join(mapped_name)

https://codereview.chromium.org/14948008/diff/1001/build/android/gyp/generate...
build/android/gyp/generate_v14_resources.py:133: if
dom.getElementsByTagName('resources'):
only the root node should be named resources. I'd just check this instead of
looking for arbitrary elements named "resources"

https://codereview.chromium.org/14948008/diff/1001/build/android/gyp/generate...
build/android/gyp/generate_v14_resources.py:134: if
dom.getElementsByTagName('style'):
similarly, the style nodes should all be children of the resources node.

[Kibeom Kim (inactive), 2013-05-06 19:18:16]

> - Do style resources cause crashes on pre-17 devices, or do they work fine as
> long as we include "paddingLeft" and "paddingStart"

Haven't verified. I asked skumar (who originally reported the bug) to borrow the
device today. But anyways, I verified that b/8654490 still presents with styles,
so anyways we have to do this I think.

> - how long does these build steps take to run? we could consider optimizing 
> GenerateV14StyleResources() by only looking at styles.xml, instead of
> considering all values/*.xml files

generate_v14_resources.py is called 4 times and it sums up to 0.32s. Because
speed is not really an issue and style resource filename can be arbitrary, I
would just keep this.

https://codereview.chromium.org/14948008/diff/1001/build/android/gyp/copy_v17...
File build/android/gyp/copy_v17_resources.py (right):

https://codereview.chromium.org/14948008/diff/1001/build/android/gyp/copy_v17...
build/android/gyp/copy_v17_resources.py:89: CopyXmlResourcesInDir(input_dir,
output_dir, True)
On 2013/05/06 16:59:34, newt wrote:
> I'd add "only_styles=" for clarity:
> 
>     CopyXmlResourcesInDir(input_dir, output_dir, only_styles=True)

Done.

https://codereview.chromium.org/14948008/diff/1001/build/android/gyp/generate...
File build/android/gyp/generate_v14_resources.py (right):

https://codereview.chromium.org/14948008/diff/1001/build/android/gyp/generate...
build/android/gyp/generate_v14_resources.py:66: def
GenerateV14StyleXmlResource(dom, output_file):
On 2013/05/06 16:59:34, newt wrote:
> I'd remove "Xml" from the function name here and below, since it's completely
> redundant.

Done.

https://codereview.chromium.org/14948008/diff/1001/build/android/gyp/generate...
build/android/gyp/generate_v14_resources.py:75: if name in ATTRIBUTES_TO_MAP:
You're right. I was skipping the namespace check.

but in this context, namespace is 'android' not
'http://schemas.android.com/apk/res/android'. To check the namespace correctly,
I should visit all the layout xml files that uses this style and check if the
namespace in this context is aliased to
http://schemas.android.com/apk/res/android.
(xmlns:android="http://schemas.android.com/apk/res/android")

Let me know if you think I should check precisely. For now, I'll just check
'android' and add a comment explaining that.

http://schemas.android.com/apk/res/android
On 2013/05/06 16:59:34, newt wrote:
> technically you should check the namespace too.
> 
>     name = item_element.attributes['name'].value.split(':')
>     if name in ATTRIBUTES_TO_MAP_NS:
>       mapped_name = ATTRIBUTES_TO_MAP_NS[name]
>       item_element.attributes['name'] = ':'.join(mapped_name)

https://codereview.chromium.org/14948008/diff/1001/build/android/gyp/generate...
build/android/gyp/generate_v14_resources.py:133: if
dom.getElementsByTagName('resources'):
On 2013/05/06 16:59:34, newt wrote:
> only the root node should be named resources. I'd just check this instead of
> looking for arbitrary elements named "resources"

Done.

https://codereview.chromium.org/14948008/diff/1001/build/android/gyp/generate...
build/android/gyp/generate_v14_resources.py:134: if
dom.getElementsByTagName('style'):
On 2013/05/06 16:59:34, newt wrote:
> similarly, the style nodes should all be children of the resources node.

Done.

[newt (away), 2013-05-06 20:55:55]

https://codereview.chromium.org/14948008/diff/8001/build/android/gyp/generate...
File build/android/gyp/generate_v14_resources.py (right):

https://codereview.chromium.org/14948008/diff/8001/build/android/gyp/generate...
build/android/gyp/generate_v14_resources.py:75: # Note: namespace == 'android'
is not precise because
agreed, checking for "android" is plenty. if we start aliasing "android" to some
other namespace in our xml files, we'll have a lot more problems than just this
to deal with.

https://codereview.chromium.org/14948008/diff/8001/build/android/gyp/generate...
build/android/gyp/generate_v14_resources.py:194: if resource_type in ('layout',
'xml', 'values'):
we know that style resources live in "values", and layouts live in "layout" or
"xml". I think you should differentiate here, i.e. only call
GenerateV14StyleResource() for resources in values, and vice versa. (like you do
in copy_v17_resources.py)

Also, I'm not sure why we have any layouts living in the res/xml folder. I
believe those should be removed, though that can be a later change.

[Kibeom Kim (inactive), 2013-05-06 22:28:01]

https://codereview.chromium.org/14948008/diff/8001/build/android/gyp/generate...
File build/android/gyp/generate_v14_resources.py (right):

https://codereview.chromium.org/14948008/diff/8001/build/android/gyp/generate...
build/android/gyp/generate_v14_resources.py:194: if resource_type in ('layout',
'xml', 'values'):
On 2013/05/06 20:55:55, newt wrote:
> we know that style resources live in "values", and layouts live in "layout" or
> "xml". I think you should differentiate here, i.e. only call
> GenerateV14StyleResource() for resources in values, and vice versa. (like you
do
> in copy_v17_resources.py)
> 
> Also, I'm not sure why we have any layouts living in the res/xml folder. I
> believe those should be removed, though that can be a later change.

Done.

[newt (away), 2013-05-06 22:52:50]

lgtm plus a request to do some resource cleanup once this lands (see comments).
this should be pretty simple and quick. thanks!

https://codereview.chromium.org/14948008/diff/15001/build/android/gyp/copy_v1...
File build/android/gyp/copy_v17_resources.py (right):

https://codereview.chromium.org/14948008/diff/15001/build/android/gyp/copy_v1...
build/android/gyp/copy_v17_resources.py:86: if resource_type in ('layout',
'xml'):
please add "TODO(kkimlabs): don't process xml directly once all layouts have
been moved out of XML directory. see http://crbug.com/238458"

https://codereview.chromium.org/14948008/diff/15001/build/android/gyp/generat...
File build/android/gyp/generate_v14_resources.py (right):

https://codereview.chromium.org/14948008/diff/15001/build/android/gyp/generat...
build/android/gyp/generate_v14_resources.py:196: if resource_type in ('layout',
'xml'):
please add "TODO(kkimlabs): don't process xml directly once all layouts have
been moved out of XML directory. see http://crbug.com/238458"

[cjhopman, 2013-05-06 22:56:57]

lgtm

[Kibeom Kim (inactive), 2013-05-06 22:59:17]

https://codereview.chromium.org/14948008/diff/15001/build/android/gyp/copy_v1...
File build/android/gyp/copy_v17_resources.py (right):

https://codereview.chromium.org/14948008/diff/15001/build/android/gyp/copy_v1...
build/android/gyp/copy_v17_resources.py:86: if resource_type in ('layout',
'xml'):
ok!

https://codereview.chromium.org/14948008/diff/15001/build/android/gyp/generat...
File build/android/gyp/generate_v14_resources.py (right):

https://codereview.chromium.org/14948008/diff/15001/build/android/gyp/generat...
build/android/gyp/generate_v14_resources.py:196: if resource_type in ('layout',
'xml'):
On 2013/05/06 22:52:50, newt wrote:
> please add "TODO(kkimlabs): don't process xml directly once all layouts have
> been moved out of XML directory. see http://crbug.com/238458%22

Done.

[commit-bot: I haz the power, 2013-05-06 23:00:37]

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

[commit-bot: I haz the power, 2013-05-07 00:03:36]

Retried try job too often on linux_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-05-07 00:06:02]

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

[commit-bot: I haz the power, 2013-05-07 01:07:38]

Retried try job too often on linux_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-05-07 21:04:20]

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

[commit-bot: I haz the power, 2013-05-08 18:32:50]

Change committed as 198947