Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14476011:
[Android] Auto-generate API 14 resources from the existing API 17 resources. (Closed)
|
Created:
7 years, 8 months ago by Kibeom Kim (inactive) Modified:
7 years, 7 months ago CC:
fdimeglio_google.com Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Android] Auto-generate RTL layout xmls from existing layout xmls.
There are several attributes introduced in API 17, mostly for BiDi(RTL) support, e.g., paddingStart.
This build script will generate another set of resource that is API 14 compatible by
converting those API 17 attributes to API 14 attributes, e.g., paddingStart -> paddingLeft.
The goal of this script is for programmers to use those attributes without worrying
about backward-compatibility care and related bugs. About the bugs, please refer to crbug.com/235118 .
BUG=235118
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198325
Patch Set 1 #
Total comments: 18
Patch Set 2 : Fixed Newton's comments and polished. #
Total comments: 12
Patch Set 3 : Fixed an error caused by ">(res_dir)" #
Total comments: 6
Patch Set 4 : Not finished. Script is done but I have to make it an independent gyp action, one for copying, -v17… #Patch Set 5 : Maked the script call as a gyp action. Copy gyp action should be added. #Patch Set 6 : Reverted empty line number chanes introduced by the last patch #Patch Set 7 : Same as the above. #Patch Set 8 : Indenting fix #
Total comments: 28
Patch Set 9 : Added copying script action. Pretty much complete I think #
Total comments: 16
Patch Set 10 : Fixed Newton's comment. Investigating warnings. #
Total comments: 33
Patch Set 11 : Fixed Newton and Chris' comments. Currently, not overriding exisitng resources + some warnings #Patch Set 12 : Worked around setAttributeNS(...) because of it has a bug with printing. #Patch Set 13 : Excluded v17/v14/crunch resources from process_resources.py and included only at apk resource packa… #
Total comments: 20
Patch Set 14 : Fixed style issues that newt mentioned. #Patch Set 15 : uploaded a missing comment style fix. #
Total comments: 4
Patch Set 16 : Fixed one missing dependencies_res_input_dirs renaming #
Total comments: 12
Patch Set 17 : fixed more comment styles #Patch Set 18 : #Patch Set 19 : #Patch Set 20 : #
Messages
Total messages: 39 (0 generated)
This is not finished, but could you take a look at java.gypi and process_resources.py? I'm really not sure if it's a correct way of doing or not.
this looks good. I think this is the right way to handle layout mirroring on the gyp side. I added some comments in XmlLayoutCopyMirror.py, but didn't look at it thoroughly yet. https://codereview.chromium.org/14476011/diff/1/build/android/gyp/XmlLayoutCo... File build/android/gyp/XmlLayoutCopyMirror.py (right): https://codereview.chromium.org/14476011/diff/1/build/android/gyp/XmlLayoutCo... build/android/gyp/XmlLayoutCopyMirror.py:1: #!/usr/bin/env python copyright also, this filename should be in underscore_case not CamelCase. I think something like "generate_rtl_layouts.py" would be a clearer name. https://codereview.chromium.org/14476011/diff/1/build/android/gyp/XmlLayoutCo... build/android/gyp/XmlLayoutCopyMirror.py:9: pre = '{http://schemas.android.com/apk/res/android}' constants should be in ALL_CAPS. also, I'd give this a clearer name, e.g. "NAMESPACE_PREFIX" http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Naming... https://codereview.chromium.org/14476011/diff/1/build/android/gyp/XmlLayoutCo... build/android/gyp/XmlLayoutCopyMirror.py:12: 'drawableLeft', add more spaces to line these up with 'paddingLeft' https://codereview.chromium.org/14476011/diff/1/build/android/gyp/XmlLayoutCo... build/android/gyp/XmlLayoutCopyMirror.py:42: element.attrib[directional_attribute].replace('left', 'XXX').replace('right', 'left').replace('XXX', 'right') it'd be nice to avoid using XXX like this, e.g.: re.sub('left|right', lambda x: 'right' if x.group(0) == 'left' else 'left', element.attrib[directional_attribute]) https://codereview.chromium.org/14476011/diff/1/build/android/gyp/XmlLayoutCo... build/android/gyp/XmlLayoutCopyMirror.py:45: if not os.path.exists(output_dir): build_utils.MakeDirectory(output_dir) https://codereview.chromium.org/14476011/diff/1/build/android/gyp/process_res... File build/android/gyp/process_resources.py (right): https://codereview.chromium.org/14476011/diff/1/build/android/gyp/process_res... build/android/gyp/process_resources.py:97: # Reverse xmls "Generate RTL versions of layouts and other XML resources." https://codereview.chromium.org/14476011/diff/1/build/android/gyp/process_res... build/android/gyp/process_resources.py:99: ldrtl_cmd = ['/usr/local/google/code/clankium-master/src/build/android/gyp/XmlLayoutCopyMirror.py', you need to find the correct path to XmlLayoutCopyMirror.py, of course. Since it's in the same directory as process_resources.py, you could just use "XmlLayoutCopyMirror.py" https://codereview.chromium.org/14476011/diff/1/build/java.gypi File build/java.gypi (right): https://codereview.chromium.org/14476011/diff/1/build/java.gypi#newcode86 build/java.gypi:86: 'res_ldrtl_dir': '<(intermediate_dir)/res_ldrtl', maybe "res_mirrored_dir" https://codereview.chromium.org/14476011/diff/1/build/java.gypi#newcode164 build/java.gypi:164: '--xml-ldrtl-input-dir', '<(res_dir)', ">(res_dir)"
Thanks for the detailed comment on near-trash quality script. I cleaned up quite a bit. Just one issue is that... it's not working... https://codereview.chromium.org/14476011/diff/1/build/android/gyp/XmlLayoutCo... File build/android/gyp/XmlLayoutCopyMirror.py (right): https://codereview.chromium.org/14476011/diff/1/build/android/gyp/XmlLayoutCo... build/android/gyp/XmlLayoutCopyMirror.py:1: #!/usr/bin/env python On 2013/04/24 23:34:38, newt wrote: > copyright > > also, this filename should be in underscore_case not CamelCase. I think > something like "generate_rtl_layouts.py" would be a clearer name. Done. https://codereview.chromium.org/14476011/diff/1/build/android/gyp/XmlLayoutCo... build/android/gyp/XmlLayoutCopyMirror.py:9: pre = '{http://schemas.android.com/apk/res/android}' On 2013/04/24 23:34:38, newt wrote: > constants should be in ALL_CAPS. also, I'd give this a clearer name, e.g. > "NAMESPACE_PREFIX" > > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Naming... Done. https://codereview.chromium.org/14476011/diff/1/build/android/gyp/XmlLayoutCo... build/android/gyp/XmlLayoutCopyMirror.py:12: 'drawableLeft', On 2013/04/24 23:34:38, newt wrote: > add more spaces to line these up with 'paddingLeft' Done. https://codereview.chromium.org/14476011/diff/1/build/android/gyp/XmlLayoutCo... build/android/gyp/XmlLayoutCopyMirror.py:42: element.attrib[directional_attribute].replace('left', 'XXX').replace('right', 'left').replace('XXX', 'right') I haven't used regex much, but this simple thing looks nice! thanks! On 2013/04/24 23:34:38, newt wrote: > it'd be nice to avoid using XXX like this, e.g.: > > re.sub('left|right', > lambda x: 'right' if x.group(0) == 'left' else 'left', > element.attrib[directional_attribute]) https://codereview.chromium.org/14476011/diff/1/build/android/gyp/XmlLayoutCo... build/android/gyp/XmlLayoutCopyMirror.py:45: if not os.path.exists(output_dir): On 2013/04/24 23:34:38, newt wrote: > build_utils.MakeDirectory(output_dir) Done. https://codereview.chromium.org/14476011/diff/1/build/android/gyp/process_res... File build/android/gyp/process_resources.py (right): https://codereview.chromium.org/14476011/diff/1/build/android/gyp/process_res... build/android/gyp/process_resources.py:97: # Reverse xmls On 2013/04/24 23:34:38, newt wrote: > "Generate RTL versions of layouts and other XML resources." Done. https://codereview.chromium.org/14476011/diff/1/build/android/gyp/process_res... build/android/gyp/process_resources.py:99: ldrtl_cmd = ['/usr/local/google/code/clankium-master/src/build/android/gyp/XmlLayoutCopyMirror.py', On 2013/04/24 23:34:38, newt wrote: > you need to find the correct path to XmlLayoutCopyMirror.py, of course. Since > it's in the same directory as process_resources.py, you could just use > "XmlLayoutCopyMirror.py" Done. https://codereview.chromium.org/14476011/diff/1/build/java.gypi File build/java.gypi (right): https://codereview.chromium.org/14476011/diff/1/build/java.gypi#newcode86 build/java.gypi:86: 'res_ldrtl_dir': '<(intermediate_dir)/res_ldrtl', On 2013/04/24 23:34:38, newt wrote: > maybe "res_mirrored_dir" Done. https://codereview.chromium.org/14476011/diff/1/build/java.gypi#newcode164 build/java.gypi:164: '--xml-ldrtl-input-dir', '<(res_dir)', On 2013/04/24 23:34:38, newt wrote: > ">(res_dir)" Hmm.. but strangely it gives an error. It doesn't put anything for --res-mirror-input-dir . [1/157] ACTION processing resources for ui_java FAILED: cd ../../ui; python ../build/android/gyp/process_resources.py --android-sdk /usr/local/google/code/clankium-master/src/third_party/android_tools/sdk//platforms/android-17 --android-sdk-tools /usr/local/google/code/clankium-master/src/third_party/android_tools/sdk//platform-tools --R-dir ../out/Debug/gen/ui_java/java_R --res-dirs "../ui/android/java/res \"../out/Debug/gen/ui_java/res_grit\"" --crunch-input-dir ../ui/android/java/res --crunch-output-dir ../out/Debug/gen/ui_java/res_crunched --res-mirror-input-dir --res-mirror-output-dir ../out/Debug/gen/ui_java/res_mirrored --android-manifest ../build/android/AndroidManifest.xml --non-constant-id --custom-package org.chromium.ui --stamp ../out/Debug/gen/ui_java/resources.stamp "--ignore=1beb8152c918bd6c93ccb4b767346289 -" Usage: process_resources.py [options]
I get some warning so investigating... nothing matches overlay file date_time_picker_dialog.xml, for flavor ,,,ldrtl,,,,,,,,,,,,,,,, nothing matches overlay file date_time_picker_dialog.xml, for flavor ,,,ldrtl,,,,,,land,,,,,,,,,, nothing matches overlay file month_picker.xml, for flavor ,,,ldrtl,,,,,,,,,,,,,,,, [87/106] ACTION processing resources for chrome_private_java nothing matches overlay file date_time_picker_dialog.xml, for flavor ,,,ldrtl,,,,,,,,,,,,,,,, nothing matches overlay file date_time_picker_dialog.xml, for flavor ,,,ldrtl,,,,,,land,,,,,,,,,, nothing matches overlay file month_picker.xml, for flavor ,,,ldrtl,,,,,,,,,,,,,,,, nothing matches overlay file autofill_dialog_content.xml, for flavor ,,,ldrtl,,,,,,,,,,,,,,,, nothing matches overlay file autofill_dialog_title.xml, for flavor ,,,ldrtl,,,,,,,,,,,,,,,, nothing matches overlay file autofill_editing_layout_credit_card.xml, for flavor ,,,ldrtl,,,,,,,,,,,,,,,, nothing matches overlay file autofill_editing_layout_email.xml, for flavor ,,,ldrtl,,,,,,,,,,,,,,,, nothing matches overlay file autofill_editing_layout_shipping.xml, for flavor ,,,ldrtl,,,,,,,,,,,,,,,, nothing matches overlay file autofill_general_layout.xml, for flavor ,,,ldrtl,,,,,,,,,,,,,,,, nothing matches overlay file autofill_menu_item.xml, for flavor ,,,ldrtl,,,,,,,,,,,,,,,, nothing matches overlay file autofill_text.xml, for flavor ,,,ldrtl,,,,,,,,,,,,,,,, nothing matches overlay file js_modal_dialog.xml, for flavor ,,,ldrtl,,,,,,,,,,,,,,,, nothing matches overlay file website_settings.xml, for flavor ,,,ldrtl,,,,,,,,,,,,,,,,
https://codereview.chromium.org/14476011/diff/7001/build/android/gyp/mirror_r... File build/android/gyp/mirror_resources.py (right): https://codereview.chromium.org/14476011/diff/7001/build/android/gyp/mirror_r... build/android/gyp/mirror_resources.py:18: ATTRIBUTES_TO_MIRROR = ['paddingLeft', completely optional / just an idea. you could make this a dictionary: ATTRIBUTES_TO_SWAP = { 'paddingLeft' : 'paddingRight', ..., 'layout_toLeftOf': 'layout_toRightOf'} for k, v in ATTRIBUTES_TO_SWAP.items(): ATTRIBUTES_TO_SWAP[v] = k then below you can just say mirrored_attribute_name = ATTRIBUTES_TO_SWAP.get(attribute_name) if mirrored_attribute_name: ... elif attribute_name in ['gravity', ''']: ... https://codereview.chromium.org/14476011/diff/7001/build/android/gyp/mirror_r... build/android/gyp/mirror_resources.py:73: def mirror_resources_in_dir(input_dir, output_dir): doc string here. mention that it's recursive. https://codereview.chromium.org/14476011/diff/7001/build/android/gyp/mirror_r... build/android/gyp/mirror_resources.py:121: if '-ldrtl' in res_sub_dir: I'd get the resource type and list of qualifiers here: dir_pieces = res_sub_dir.split('-') resource_type = dir_pieces[0] qualifiers = dir_pieces[1:] if 'ldrtl' in qualifiers: continue if resource_type in ('layout', 'xml'): ... https://codereview.chromium.org/14476011/diff/7001/build/android/gyp/mirror_r... build/android/gyp/mirror_resources.py:129: res_sub_dir_split.insert(1, 'ldrtl') I'd add a comment here about why it's OK to insert ldrtl as the first qualifier. (because it's _almost_ at the top of http://developer.android.com/guide/topics/resources/providing-resources.html#... and we never use the qualifiers that are higher) https://codereview.chromium.org/14476011/diff/7001/build/java.gypi File build/java.gypi (right): https://codereview.chromium.org/14476011/diff/7001/build/java.gypi#newcode110 build/java.gypi:110: '<(res_mirrored_dir)', move res_mirrored_dir after res_input_dirs. this will ensure that if someone has to create a custom rtl layout, it will override the autogenerated layout https://codereview.chromium.org/14476011/diff/7001/build/java.gypi#newcode166 build/java.gypi:166: '--res-mirror-input-dir', '>(res_dir)', this is a gyp bug :(
https://codereview.chromium.org/14476011/diff/11001/build/android/gyp/process... File build/android/gyp/process_resources.py (right): https://codereview.chromium.org/14476011/diff/11001/build/android/gyp/process... build/android/gyp/process_resources.py:30: help='directory containing images to be crunched and' \ don't need \ http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Line_l... https://codereview.chromium.org/14476011/diff/11001/build/android/gyp/process... build/android/gyp/process_resources.py:99: mirror_cmd = [sys.executable, Chris may know the best way to do this
Talked with David&Ted and decided to generate layout/ from layout-v17/ instead of current layout-ldrtl/ from layout/ . So it will require a source directory change, from layout/ to layout-v17/ and use paddingStart/paddingEnd there for example. The main reason is that, it's more explicit that people are writing layout files that supports both LTR and RTL, because they're writing Start/End.
https://codereview.chromium.org/14476011/diff/11001/build/android/gyp/process... File build/android/gyp/process_resources.py (right): https://codereview.chromium.org/14476011/diff/11001/build/android/gyp/process... build/android/gyp/process_resources.py:99: mirror_cmd = [sys.executable, On 2013/04/25 00:47:58, newt wrote: > Chris may know the best way to do this Haven't looked at the rest of the change yet, but for this: Much better than calling this in a subprocess would be to import mirror_resources and call things directly. I think I would prefer that this be done as a separate gyp action, though. Feel free to ping me for details of what that would take.
https://codereview.chromium.org/14476011/diff/7001/build/android/gyp/mirror_r... File build/android/gyp/mirror_resources.py (right): https://codereview.chromium.org/14476011/diff/7001/build/android/gyp/mirror_r... build/android/gyp/mirror_resources.py:18: ATTRIBUTES_TO_MIRROR = ['paddingLeft', On 2013/04/25 00:41:24, newt wrote: > completely optional / just an idea. > > you could make this a dictionary: > > ATTRIBUTES_TO_SWAP = { > 'paddingLeft' : 'paddingRight', > ..., > 'layout_toLeftOf': 'layout_toRightOf'} > > for k, v in ATTRIBUTES_TO_SWAP.items(): > ATTRIBUTES_TO_SWAP[v] = k > > then below you can just say > > mirrored_attribute_name = ATTRIBUTES_TO_SWAP.get(attribute_name) > if mirrored_attribute_name: > ... > elif attribute_name in ['gravity', ''']: > ... Done. https://codereview.chromium.org/14476011/diff/7001/build/android/gyp/mirror_r... build/android/gyp/mirror_resources.py:73: def mirror_resources_in_dir(input_dir, output_dir): On 2013/04/25 00:41:24, newt wrote: > doc string here. mention that it's recursive. Done. https://codereview.chromium.org/14476011/diff/7001/build/android/gyp/mirror_r... build/android/gyp/mirror_resources.py:121: if '-ldrtl' in res_sub_dir: I like that much better. Thanks. Done. On 2013/04/25 00:41:24, newt wrote: > I'd get the resource type and list of qualifiers here: > > dir_pieces = res_sub_dir.split('-') > resource_type = dir_pieces[0] > qualifiers = dir_pieces[1:] > > if 'ldrtl' in qualifiers: > continue > > if resource_type in ('layout', 'xml'): > ... https://codereview.chromium.org/14476011/diff/7001/build/android/gyp/mirror_r... build/android/gyp/mirror_resources.py:129: res_sub_dir_split.insert(1, 'ldrtl') On 2013/04/25 00:41:24, newt wrote: > I'd add a comment here about why it's OK to insert ldrtl as the first qualifier. > (because it's _almost_ at the top of > http://developer.android.com/guide/topics/resources/providing-resources.html#... > and we never use the qualifiers that are higher) Done. https://codereview.chromium.org/14476011/diff/7001/build/java.gypi File build/java.gypi (right): https://codereview.chromium.org/14476011/diff/7001/build/java.gypi#newcode110 build/java.gypi:110: '<(res_mirrored_dir)', On 2013/04/25 00:41:24, newt wrote: > move res_mirrored_dir after res_input_dirs. this will ensure that if someone > has to create a custom rtl layout, it will override the autogenerated layout Done. https://codereview.chromium.org/14476011/diff/7001/build/java.gypi#newcode166 build/java.gypi:166: '--res-mirror-input-dir', '>(res_dir)', On 2013/04/25 00:41:24, newt wrote: > this is a gyp bug :( Done. https://codereview.chromium.org/14476011/diff/11001/build/android/gyp/process... File build/android/gyp/process_resources.py (right): https://codereview.chromium.org/14476011/diff/11001/build/android/gyp/process... build/android/gyp/process_resources.py:30: help='directory containing images to be crunched and' \ On 2013/04/25 00:47:58, newt wrote: > don't need \ > > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Line_l... Done. https://codereview.chromium.org/14476011/diff/11001/build/android/gyp/process... build/android/gyp/process_resources.py:99: mirror_cmd = [sys.executable, OK I'll try to make this a separate gyp action after converting this script to handle layout-v17 On 2013/04/25 01:02:06, cjhopman wrote: > On 2013/04/25 00:47:58, newt wrote: > > Chris may know the best way to do this > Haven't looked at the rest of the change yet, but for this: > > Much better than calling this in a subprocess would be to import > mirror_resources and call things directly. > > I think I would prefer that this be done as a separate gyp action, though. Feel > free to ping me for details of what that would take.
On 2013/04/25 00:58:59, Kibeom Kim wrote: > Talked with David&Ted and decided to generate layout/ from layout-v17/ instead > of current layout-ldrtl/ from layout/ . So it will require a source directory > change, from layout/ to layout-v17/ and use paddingStart/paddingEnd there for > example. > > The main reason is that, it's more explicit that people are writing layout files > that supports both LTR and RTL, because they're writing Start/End. Is there a way to do this that doesn't require adding -v17 to all of our res folders? Adding -v17, means we'll have the following folders: - drawable-hdpi - for png drawables - drawable-hdpi-v17 - for xml drawables - etc etc etc If someone accidentally puts a png file in drawable-hdpi-v17, we'll crash at run time when it's accessed. On the other hand, I like the idea of using the "new" attributes (paddingStart, paddingEnd, etc) in the checked in files, and using the old attributes in the auto-generated files.
On 2013/04/25 02:09:34, newt wrote: > On 2013/04/25 00:58:59, Kibeom Kim wrote: > > Talked with David&Ted and decided to generate layout/ from layout-v17/ instead > > of current layout-ldrtl/ from layout/ . So it will require a source directory > > change, from layout/ to layout-v17/ and use paddingStart/paddingEnd there for > > example. > > > > The main reason is that, it's more explicit that people are writing layout > files > > that supports both LTR and RTL, because they're writing Start/End. > > Is there a way to do this that doesn't require adding -v17 to all of our res > folders? Maybe, for example, just keep layout/ in our source folder and the script copies it to layout-v17/ and also generate layout/ to override? >Adding -v17, means we'll have the following folders: > > - drawable-hdpi - for png drawables > - drawable-hdpi-v17 - for xml drawables > - etc etc etc > > If someone accidentally puts a png file in drawable-hdpi-v17, we'll crash at run > time when it's accessed. > > On the other hand, I like the idea of using the "new" attributes (paddingStart, > paddingEnd, etc) in the checked in files, and using the old attributes in the > auto-generated files. Actually I'm not sure if we want drawable-v17 . All I can see is gravity and android:left/right, but for gravity, end/start is already backward compatible. And they don't seem to have start/end pairs for the drawable attributes, for example, They don't have <layer-list><item android:start=10dp></item></layer-list>. Not sure if it's by a choice or a bug. I'm thinking adding -17 (if we add,) only to layout/ xml/ and possibly values/ just for values/styles.xml .
Here's another option: - Using paddingStart, paddingEnd, etc in all xml files - Don't include -v17 in the folder names. Just use e.g. "drawable" - At build time, copy all xml files into "drawable-v17" *and* generate old school LTR versions (with paddingLeft, paddingRight) into "drawable". The old school versions in "drawable" will override the checked in layouts, so two [near-]copies of each layout will be included in the APK. Then we don't have the messy situation where -v17 is a codeword for RTL-friendly, and we don't have to double the number of drawable and layout folders
On 2013/04/25 02:45:03, newt wrote: > Here's another option: > - Using paddingStart, paddingEnd, etc in all xml files > - Don't include -v17 in the folder names. Just use e.g. "drawable" > - At build time, copy all xml files into "drawable-v17" *and* generate old > school LTR versions (with paddingLeft, paddingRight) into "drawable". The old > school versions in "drawable" will override the checked in layouts, so two > [near-]copies of each layout will be included in the APK. > > Then we don't have the messy situation where -v17 is a codeword for > RTL-friendly, and we don't have to double the number of drawable and layout > folders I like that!
Ready for review. Except copying gyp. @Chris Q: In addition to the current patch, I need to copy directories layout*/ xml*/ under clank/java/apps/chrome/res to out/Debug/gen/chrome_private_java/res_mirrored and add -v17. For example, clank/java/apps/chrome/res/layout-hdpi/ out/Debug/gen/chrome_private_java/res_mirrored/layout-hdpi-v17 What would be the best way to do? https://code.google.com/p/gyp/wiki/GypLanguageSpecification#Copies ??
> @Chris > Q: In addition to the current patch, I need to copy directories layout*/ xml*/ I might need to selectively copy depending on the content in the future (RelativeLayout), so I'll just make another python script gyp action to copy
ok, I had a lot of comments, but I do think this is good! https://codereview.chromium.org/14476011/diff/11001/build/android/gyp/mirror_... File build/android/gyp/mirror_resources.py (right): https://codereview.chromium.org/14476011/diff/11001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:46: attribute_names = ATTRIBUTES_TO_MIRROR.intersection(element.keys()) if this file changes to convert RTL-friendly layouts into LTR-only layouts (e.g. paddingStart -> paddingLeft), then we could check for "illegal" attributes here. e.g. make sure that the input files don't contain paddingLeft or paddingRight. https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... File build/android/gyp/mirror_resources.py (right): https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:11: import xml.etree.cElementTree as ET remove this https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:33: ATTRIBUTES_TO_MAP = {(ATTRIBUTE_NAMESPACE, k) : (ATTRIBUTE_NAMESPACE , v) dict comprehensions are python 2.7+. not sure if that's allowed in Chromium https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:69: # This is actually not necessary because start/end is backward-compatible. then why do it? :) https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:76: elif name in ATTRIBUTES_TO_MAP.keys(): don't need ".keys()" https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:82: if value != element.getAttributeNS(*mapped_name): I'd skip this check. We can just make it a rule that paddingLeft and paddingStart should never both be defined. https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:83: error_message = '[ERROR]' + input_filename + '\n' I'd write the error like this: Error: some_layout.xml: These attributes are... https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:97: I think it makes sense to check for non-RTL-friendly attributes and emit a warning or error if they're found. elif name in ATTRIBUTES_TO_MAP.values(): # TODO(kkimlabs): enable this warning once layouts have been converted #print >> sys.stderror, 'Warning: layout should use xxx instead of yyy' pass It's possible (though I think unlikely) that we'll have some super fancy layout in the future where we need to specify paddingLeft for some reason. If that happens, we can change the warning system then. The extremely likely case, however, is that someone in unaware of the RTL issues, and uses paddingLeft because that's what we've always done. https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:106: for root, dirs, files in os.walk(input_dir, followlinks=True): I'd remove followlinks=True. actually, this doesn't need to be recursive at all, so you could just use for name in os.listdir(input_dir): if os.isfile(name): ... and get rid of relative_dir https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:123: help='directory containing resources to be mirrored to RTL') these help messages need updating https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:139: """Convert Android xml resources to API 14 compatible. I'd mention the issue with Galaxy Note 2 (or whatever it is), and explain when this script can be removed (once we Galaxy Note 2 market share is 0? once we drop support for JB MR0?) https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:144: res_sub_dir_names = os.walk(options.res_dir).next()[1] add a comment explaining what this non-obvious call does https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:150: only one blank line here https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:159: if 'ldrtl' in qualifiers: move this to the top of the if block https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:164: os.path.join(options.res_mirrored_dir, maybe assign this to a variable and pass in the variable name, to make it clearer what this argument represents
Thanks for the helpful comments. All fixed. Investigating the warnings I mentioned at comment #4. https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... File build/android/gyp/mirror_resources.py (right): https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:11: import xml.etree.cElementTree as ET On 2013/04/26 18:46:26, newt wrote: > remove this Done. https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:33: ATTRIBUTES_TO_MAP = {(ATTRIBUTE_NAMESPACE, k) : (ATTRIBUTE_NAMESPACE , v) On 2013/04/26 18:46:26, newt wrote: > dict comprehensions are python 2.7+. not sure if that's allowed in Chromium Done. https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:69: # This is actually not necessary because start/end is backward-compatible. On 2013/04/26 18:46:26, newt wrote: > then why do it? :) yeah.. actually I was on the edge, because it didn't matter; but I guess less code is better. https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:76: elif name in ATTRIBUTES_TO_MAP.keys(): On 2013/04/26 18:46:26, newt wrote: > don't need ".keys()" Done. https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:82: if value != element.getAttributeNS(*mapped_name): On 2013/04/26 18:46:26, newt wrote: > I'd skip this check. We can just make it a rule that paddingLeft and > paddingStart should never both be defined. Done. https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:83: error_message = '[ERROR]' + input_filename + '\n' On 2013/04/26 18:46:26, newt wrote: > I'd write the error like this: > > Error: some_layout.xml: These attributes are... Done. (Removed) https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:97: On 2013/04/26 18:46:26, newt wrote: > I think it makes sense to check for non-RTL-friendly attributes and emit a > warning or error if they're found. > > elif name in ATTRIBUTES_TO_MAP.values(): > # TODO(kkimlabs): enable this warning once layouts have been converted > #print >> sys.stderror, 'Warning: layout should use xxx instead of yyy' > pass > > It's possible (though I think unlikely) that we'll have some super fancy layout > in the future where we need to specify paddingLeft for some reason. If that > happens, we can change the warning system then. The extremely likely case, > however, is that someone in unaware of the RTL issues, and uses paddingLeft > because that's what we've always done. Done. https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:106: for root, dirs, files in os.walk(input_dir, followlinks=True): On 2013/04/26 18:46:26, newt wrote: > I'd remove followlinks=True. actually, this doesn't need to be recursive at all, > so you could just use > > for name in os.listdir(input_dir): > if os.isfile(name): > ... > > and get rid of relative_dir Done. https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:123: help='directory containing resources to be mirrored to RTL') On 2013/04/26 18:46:26, newt wrote: > these help messages need updating Done. https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:139: """Convert Android xml resources to API 14 compatible. On 2013/04/26 18:46:26, newt wrote: > I'd mention the issue with Galaxy Note 2 (or whatever it is), and explain when > this script can be removed (once we Galaxy Note 2 market share is 0? once we > drop support for JB MR0?) Done. https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:144: res_sub_dir_names = os.walk(options.res_dir).next()[1] On 2013/04/26 18:46:26, newt wrote: > add a comment explaining what this non-obvious call does Done. replaced to listdir(...) https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:150: On 2013/04/26 18:46:26, newt wrote: > only one blank line here Done. https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:159: if 'ldrtl' in qualifiers: On 2013/04/26 18:46:26, newt wrote: > move this to the top of the if block Done. (I just made the above if resource_type... to call continue instead) https://codereview.chromium.org/14476011/diff/39001/build/android/gyp/mirror_... build/android/gyp/mirror_resources.py:164: os.path.join(options.res_mirrored_dir, On 2013/04/26 18:46:26, newt wrote: > maybe assign this to a variable and pass in the variable name, to make it > clearer what this argument represents Done.
https://codereview.chromium.org/14476011/diff/41001/build/android/gyp/copy_re... File build/android/gyp/copy_resources_v17.py (right): https://codereview.chromium.org/14476011/diff/41001/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:14: def copy_resources_in_dir(input_dir, output_dir): Chrome uses MixedCase for function names (except for main()) https://codereview.chromium.org/14476011/diff/41001/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:22: continue You could use build_utils.FindInDirectory() here. Something like: for input_file in build_utils.FindInDirectory(input_dir, '*.xml'): ..output_path = os.path.join(output_dir, ......os.path.relpath(input_file, input_dir)) ..build_utils.MakeDirectory(os.path.dirname(output_path)) ..build_utils.CopyFile(input_file, output_path) https://codereview.chromium.org/14476011/diff/41001/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:26: build_utils.MakeDirectory(os.path.dirname(input_filename)) I don't think you need to make the input directory. https://codereview.chromium.org/14476011/diff/41001/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:43: (options, args) = parser.parse_args() don't need parentheses, just "options, args" https://codereview.chromium.org/14476011/diff/41001/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:55: """Copy resource files and add -v17 to the sub directory names. I'd prefer a file-level docstring (or have both). https://codereview.chromium.org/14476011/diff/41001/build/android/gyp/generat... File build/android/gyp/generate_v14_resources.py (right): https://codereview.chromium.org/14476011/diff/41001/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:140: """Convert Android xml resources to API 14 compatible. I'd prefer file-level docstring https://codereview.chromium.org/14476011/diff/41001/build/java.gypi File build/java.gypi (right): https://codereview.chromium.org/14476011/diff/41001/build/java.gypi#newcode156 build/java.gypi:156: '<(res_v14_dir)', Having a directory as an input/output doesn't really work. See below. https://codereview.chromium.org/14476011/diff/41001/build/java.gypi#newcode191 build/java.gypi:191: '<(res_v17_dir)', A directory as an output doesn't really work. Use a stamp file like process_resources above.
close! https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/copy_re... File build/android/gyp/copy_resources_v17.py (right): https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:1: #!/usr/bin/env python maybe name this copy_v17_resources.py for consistency https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:15: """Copy resources in the directory. "Copy all XML resources from input_dir to output_dir" https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:28: build_utils.MakeDirectory(input_dir) it doesn't make sense to create the input directory... https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:30: build_utils.CopyFile(input_filename, output_filename) Did you consider using symlinks here? I believe that's what gyp copies does. https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:43: help='dest directory to be copied.') "output directory to which resources will be copied" https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:56: def main(argv): we should probably delete the output directory before starting. that way, if someone deletes resources, they won't be included in the APK via an old copy in res_v17_dir https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/generat... File build/android/gyp/generate_v14_resources.py (right): https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:35: ATTRIBUTES_TO_MAP_NS[(ATTRIBUTE_NAMESPACE, k)] = (ATTRIBUTE_NAMESPACE , v) remove space before comma https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:63: for name, value in list(element.attributes.itemsNS()): you should be able to remove "list" https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:77: elif name in ATTRIBUTES_TO_MAP.values(): ATTRIBUTES_TO_MAP_NS also, not sure how long this all takes, but it might be worth storing ATTRIBUTES_TO_MAP_NS.values() as a set, so you don't have to regenerate the list for every attribute in every layout. https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:83: dom.writexml(open(output_filename,"w"), ' ', '\n') be sure to close these files! :) with open(output_filename, 'w') as f: dom.writexml(f, ...) p.s. use single quotes everywhere for consistency https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:111: 'be used to generate v14 resources') oops.. remove "be" https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:113: help='directory containing v14 resources to be generated') "output directory into which v14 resources will be generated" https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:126: def main(argv): we should probably delete the output directory before starting. that way, if someone deletes resources, they won't be included in the APK via an old copy in res_v14_dir
https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/copy_re... File build/android/gyp/copy_resources_v17.py (right): https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:30: build_utils.CopyFile(input_filename, output_filename) On 2013/04/26 21:22:24, newt wrote: > Did you consider using symlinks here? I believe that's what gyp copies does. Ah! please don't use symlinks -- it tends to break random things.
https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/generat... File build/android/gyp/generate_v14_resources.py (right): https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:35: ATTRIBUTES_TO_MAP_NS[(ATTRIBUTE_NAMESPACE, k)] = (ATTRIBUTE_NAMESPACE , v) On 2013/04/26 21:22:24, newt wrote: > remove space before comma Also, remove one space after first comma
https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/copy_re... File build/android/gyp/copy_resources_v17.py (right): https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:1: #!/usr/bin/env python On 2013/04/26 21:22:24, newt wrote: > maybe name this copy_v17_resources.py for consistency p.s. if you rename it, do it as a separate patch set. otherwise, rietveld won't show diffs. https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:30: build_utils.CopyFile(input_filename, output_filename) ok fine :) My preference is to minimize utility functions. build_utils.CopyFile == shutil.copy2, but as a casual reader, I don't know that... I now have to check in build_utils.py to figure out what's going on. I'd just write shutil.copy2 here. Remember: indirection is bad... except when it's useful :/
Fixed. But I found that v14 is not overriding the original resources. It overrided before, back at when I converted to ldrtl resources. digging. https://codereview.chromium.org/14476011/diff/41001/build/android/gyp/copy_re... File build/android/gyp/copy_resources_v17.py (right): https://codereview.chromium.org/14476011/diff/41001/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:14: def copy_resources_in_dir(input_dir, output_dir): On 2013/04/26 20:57:31, cjhopman wrote: > Chrome uses MixedCase for function names (except for main()) Done. https://codereview.chromium.org/14476011/diff/41001/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:22: continue On 2013/04/26 20:57:31, cjhopman wrote: > You could use build_utils.FindInDirectory() here. Something like: > for input_file in build_utils.FindInDirectory(input_dir, '*.xml'): > ..output_path = os.path.join(output_dir, > ......os.path.relpath(input_file, input_dir)) > ..build_utils.MakeDirectory(os.path.dirname(output_path)) > ..build_utils.CopyFile(input_file, output_path) Done. Much shorter thanks! https://codereview.chromium.org/14476011/diff/41001/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:26: build_utils.MakeDirectory(os.path.dirname(input_filename)) On 2013/04/26 20:57:31, cjhopman wrote: > I don't think you need to make the input directory. Done. https://codereview.chromium.org/14476011/diff/41001/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:43: (options, args) = parser.parse_args() On 2013/04/26 20:57:31, cjhopman wrote: > don't need parentheses, just "options, args" Done. https://codereview.chromium.org/14476011/diff/41001/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:55: """Copy resource files and add -v17 to the sub directory names. On 2013/04/26 20:57:31, cjhopman wrote: > I'd prefer a file-level docstring (or have both). Done. https://codereview.chromium.org/14476011/diff/41001/build/android/gyp/generat... File build/android/gyp/generate_v14_resources.py (right): https://codereview.chromium.org/14476011/diff/41001/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:140: """Convert Android xml resources to API 14 compatible. On 2013/04/26 20:57:31, cjhopman wrote: > I'd prefer file-level docstring Done. https://codereview.chromium.org/14476011/diff/41001/build/java.gypi File build/java.gypi (right): https://codereview.chromium.org/14476011/diff/41001/build/java.gypi#newcode156 build/java.gypi:156: '<(res_v14_dir)', On 2013/04/26 20:57:31, cjhopman wrote: > Having a directory as an input/output doesn't really work. See below. Done. https://codereview.chromium.org/14476011/diff/41001/build/java.gypi#newcode191 build/java.gypi:191: '<(res_v17_dir)', On 2013/04/26 20:57:31, cjhopman wrote: > A directory as an output doesn't really work. Use a stamp file like > process_resources above. Done. https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/copy_re... File build/android/gyp/copy_resources_v17.py (right): https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:1: #!/usr/bin/env python On 2013/04/26 21:22:24, newt wrote: > maybe name this copy_v17_resources.py for consistency Done. https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:1: #!/usr/bin/env python On 2013/04/26 22:50:47, newt wrote: > On 2013/04/26 21:22:24, newt wrote: > > maybe name this copy_v17_resources.py for consistency > > p.s. if you rename it, do it as a separate patch set. otherwise, rietveld won't > show diffs. Done. https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:15: """Copy resources in the directory. On 2013/04/26 21:22:24, newt wrote: > "Copy all XML resources from input_dir to output_dir" Done. https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:28: build_utils.MakeDirectory(input_dir) On 2013/04/26 21:22:24, newt wrote: > it doesn't make sense to create the input directory... Done. https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:30: build_utils.CopyFile(input_filename, output_filename) On 2013/04/26 21:48:47, cjhopman wrote: > On 2013/04/26 21:22:24, newt wrote: > > Did you consider using symlinks here? I believe that's what gyp copies does. > Ah! please don't use symlinks -- it tends to break random things. ok.. https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:30: build_utils.CopyFile(input_filename, output_filename) On 2013/04/26 22:50:47, newt wrote: > ok fine :) > > My preference is to minimize utility functions. build_utils.CopyFile == > shutil.copy2, but as a casual reader, I don't know that... I now have to check > in build_utils.py to figure out what's going on. I'd just write shutil.copy2 > here. Remember: indirection is bad... except when it's useful :/ Done. https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:43: help='dest directory to be copied.') On 2013/04/26 21:22:24, newt wrote: > "output directory to which resources will be copied" Done. https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/copy_re... build/android/gyp/copy_resources_v17.py:56: def main(argv): On 2013/04/26 21:22:24, newt wrote: > we should probably delete the output directory before starting. that way, if > someone deletes resources, they won't be included in the APK via an old copy in > res_v17_dir Done. https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/generat... File build/android/gyp/generate_v14_resources.py (right): https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:35: ATTRIBUTES_TO_MAP_NS[(ATTRIBUTE_NAMESPACE, k)] = (ATTRIBUTE_NAMESPACE , v) On 2013/04/26 21:22:24, newt wrote: > remove space before comma Done. https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:35: ATTRIBUTES_TO_MAP_NS[(ATTRIBUTE_NAMESPACE, k)] = (ATTRIBUTE_NAMESPACE , v) On 2013/04/26 21:50:00, cjhopman wrote: > On 2013/04/26 21:22:24, newt wrote: > > remove space before comma > Also, remove one space after first comma Done. https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:63: for name, value in list(element.attributes.itemsNS()): Is it ok? I have del element.attributes[name] in this loop below. https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:77: elif name in ATTRIBUTES_TO_MAP.values(): On 2013/04/26 21:22:24, newt wrote: > ATTRIBUTES_TO_MAP_NS > > also, not sure how long this all takes, but it might be worth storing > ATTRIBUTES_TO_MAP_NS.values() as a set, so you don't have to regenerate the list > for every attribute in every layout. Done. https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:83: dom.writexml(open(output_filename,"w"), ' ', '\n') On 2013/04/26 21:22:24, newt wrote: > be sure to close these files! :) > > with open(output_filename, 'w') as f: > dom.writexml(f, ...) > > p.s. use single quotes everywhere for consistency Done. https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:111: 'be used to generate v14 resources') On 2013/04/26 21:22:24, newt wrote: > oops.. remove "be" Done. https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:113: help='directory containing v14 resources to be generated') On 2013/04/26 21:22:24, newt wrote: > "output directory into which v14 resources will be generated" Done. https://codereview.chromium.org/14476011/diff/41002/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:126: def main(argv): On 2013/04/26 21:22:24, newt wrote: > we should probably delete the output directory before starting. that way, if > someone deletes resources, they won't be included in the APK via an old copy in > res_v14_dir Done.
Actually, it's overriding. confirmed it by converting paddingStart="..." to paddingLeft="100dp" in generate_v14_resources.py and saw that it screws up UI on ICS device. But for some reason, if I remove Left/Right and only leave Start/End in the original resource, that also looks slightly wrong on ICS device. digging...
ready for review.
https://codereview.chromium.org/14476011/diff/54001/build/java.gypi File build/java.gypi (right): https://codereview.chromium.org/14476011/diff/54001/build/java.gypi#newcode158 build/java.gypi:158: 'all_res_dirs': ['<@(res_input_dirs)', '>@(additional_res_input_dirs)'], line too long.
looks good, just a couple picky nits https://codereview.chromium.org/14476011/diff/54001/build/android/gyp/copy_v1... File build/android/gyp/copy_v17_resources.py (right): https://codereview.chromium.org/14476011/diff/54001/build/android/gyp/copy_v1... build/android/gyp/copy_v17_resources.py:6: """Copy resource files and add -v17 to the sub directory names. empty line before docstring https://codereview.chromium.org/14476011/diff/54001/build/android/gyp/copy_v1... build/android/gyp/copy_v17_resources.py:22: """Copy all XML resources from input_dir to output_dir period and closing quotes at the end of the line https://codereview.chromium.org/14476011/diff/54001/build/android/gyp/copy_v1... build/android/gyp/copy_v17_resources.py:24: don't need an empty line after docstring https://codereview.chromium.org/14476011/diff/54001/build/android/gyp/copy_v1... build/android/gyp/copy_v17_resources.py:38: don't need an empty line after docstring https://codereview.chromium.org/14476011/diff/54001/build/android/gyp/generat... File build/android/gyp/generate_v14_resources.py (right): https://codereview.chromium.org/14476011/diff/54001/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:6: """Convert Android xml resources to API 14 compatible. empty line before this https://codereview.chromium.org/14476011/diff/54001/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:55: """minidom helper function that iterates all the element nodes. Depth-first. Move closing quotes up and make this a proper sentence. https://codereview.chromium.org/14476011/diff/54001/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:108: """Convert resources to API 14 compatible XML resources in the directory. docstring on one line (move """ up) https://codereview.chromium.org/14476011/diff/54001/build/java.gypi File build/java.gypi (right): https://codereview.chromium.org/14476011/diff/54001/build/java.gypi#newcode114 build/java.gypi:114: 'additional_res_input_dirs': ['<@(res_input_dirs)'], this should be "dependencies_res_input_dirs" https://codereview.chromium.org/14476011/diff/54001/build/java.gypi#newcode120 build/java.gypi:120: '<(res_v17_dir)', put v14 before v17 for consistency https://codereview.chromium.org/14476011/diff/54001/build/java.gypi#newcode158 build/java.gypi:158: 'all_res_dirs': ['<@(res_input_dirs)', '>@(additional_res_input_dirs)'], On 2013/05/03 18:46:03, Kibeom Kim wrote: > line too long. gyp doesn't have strict limits on line length, so go for what's most readable
https://codereview.chromium.org/14476011/diff/54001/build/android/gyp/copy_v1... File build/android/gyp/copy_v17_resources.py (right): https://codereview.chromium.org/14476011/diff/54001/build/android/gyp/copy_v1... build/android/gyp/copy_v17_resources.py:6: """Copy resource files and add -v17 to the sub directory names. On 2013/05/03 20:16:16, newt wrote: > empty line before docstring Done. https://codereview.chromium.org/14476011/diff/54001/build/android/gyp/copy_v1... build/android/gyp/copy_v17_resources.py:22: """Copy all XML resources from input_dir to output_dir On 2013/05/03 20:16:16, newt wrote: > period and closing quotes at the end of the line Done. https://codereview.chromium.org/14476011/diff/54001/build/android/gyp/copy_v1... build/android/gyp/copy_v17_resources.py:24: On 2013/05/03 20:16:16, newt wrote: > don't need an empty line after docstring Done. https://codereview.chromium.org/14476011/diff/54001/build/android/gyp/copy_v1... build/android/gyp/copy_v17_resources.py:38: On 2013/05/03 20:16:16, newt wrote: > don't need an empty line after docstring Done. https://codereview.chromium.org/14476011/diff/54001/build/android/gyp/generat... File build/android/gyp/generate_v14_resources.py (right): https://codereview.chromium.org/14476011/diff/54001/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:55: """minidom helper function that iterates all the element nodes. Depth-first. On 2013/05/03 20:16:16, newt wrote: > Move closing quotes up and make this a proper sentence. Done. https://codereview.chromium.org/14476011/diff/54001/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:108: """Convert resources to API 14 compatible XML resources in the directory. On 2013/05/03 20:16:16, newt wrote: > docstring on one line (move """ up) Done. https://codereview.chromium.org/14476011/diff/54001/build/java.gypi File build/java.gypi (right): https://codereview.chromium.org/14476011/diff/54001/build/java.gypi#newcode114 build/java.gypi:114: 'additional_res_input_dirs': ['<@(res_input_dirs)'], On 2013/05/03 20:16:16, newt wrote: > this should be "dependencies_res_input_dirs" Done. https://codereview.chromium.org/14476011/diff/54001/build/java.gypi#newcode120 build/java.gypi:120: '<(res_v17_dir)', On 2013/05/03 20:16:16, newt wrote: > put v14 before v17 for consistency Done. https://codereview.chromium.org/14476011/diff/54001/build/java.gypi#newcode158 build/java.gypi:158: 'all_res_dirs': ['<@(res_input_dirs)', '>@(additional_res_input_dirs)'], On 2013/05/03 20:16:16, newt wrote: > On 2013/05/03 18:46:03, Kibeom Kim wrote: > > line too long. > > gyp doesn't have strict limits on line length, so go for what's most readable Done.
two more nits... and lgtm https://codereview.chromium.org/14476011/diff/65002/build/android/gyp/generat... File build/android/gyp/generate_v14_resources.py (right): https://codereview.chromium.org/14476011/diff/65002/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:69: on the attribute names.""" if the docstring is one line of text, then the closing quotes should go on the same line. otherwise, they should go on their own line. So, here the quotes should go on their own line. http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Commen... https://codereview.chromium.org/14476011/diff/65002/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:108: """Convert resources to API 14 compatible XML resources oops. let me rephrase: this should be on one line, including the closing quotes.
lgtm with a couple small nits/thoughts https://codereview.chromium.org/14476011/diff/69001/build/android/gyp/copy_v1... File build/android/gyp/copy_v17_resources.py (right): https://codereview.chromium.org/14476011/diff/69001/build/android/gyp/copy_v1... build/android/gyp/copy_v17_resources.py:7: """Copy resource files and add -v17 to the sub directory names. Nit: Copy xml resource files? See comment below. https://codereview.chromium.org/14476011/diff/69001/build/android/gyp/copy_v1... build/android/gyp/copy_v17_resources.py:11: Or crbug.com/235118 . Nit: add http:// (many linkifiers require it). https://codereview.chromium.org/14476011/diff/69001/build/android/gyp/copy_v1... build/android/gyp/copy_v17_resources.py:22: def CopyResourcesInDir(input_dir, output_dir): Nit: Maybe rename to CopyXmlResourcesInDir? Or is it obvious in context that this only deals with xml resources? https://codereview.chromium.org/14476011/diff/69001/build/android/gyp/generat... File build/android/gyp/generate_v14_resources.py (right): https://codereview.chromium.org/14476011/diff/69001/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:19: Please refer to crbug.com/235118 for the details. Nit: http:// https://codereview.chromium.org/14476011/diff/69001/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:33: # The list of the attributes were taken from go/android-rtl go/android-rtl is an internal link. Is this list available publicly? If not, I'd just add a comment on the bug and link to that here. https://codereview.chromium.org/14476011/diff/69001/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:93: # However, there is a minidom bug that doesn't print namespace set by Nit: is there some kind of minidom bug report you can link to here?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/14476011/82001
https://codereview.chromium.org/14476011/diff/65002/build/android/gyp/generat... File build/android/gyp/generate_v14_resources.py (right): https://codereview.chromium.org/14476011/diff/65002/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:69: on the attribute names.""" On 2013/05/03 20:57:34, newt wrote: > if the docstring is one line of text, then the closing quotes should go on the > same line. otherwise, they should go on their own line. So, here the quotes > should go on their own line. > > > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Commen... Done. https://codereview.chromium.org/14476011/diff/65002/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:108: """Convert resources to API 14 compatible XML resources On 2013/05/03 20:57:34, newt wrote: > oops. let me rephrase: this should be on one line, including the closing quotes. Done. https://codereview.chromium.org/14476011/diff/69001/build/android/gyp/copy_v1... File build/android/gyp/copy_v17_resources.py (right): https://codereview.chromium.org/14476011/diff/69001/build/android/gyp/copy_v1... build/android/gyp/copy_v17_resources.py:7: """Copy resource files and add -v17 to the sub directory names. On 2013/05/03 21:23:04, cjhopman wrote: > Nit: Copy xml resource files? See comment below. Done. https://codereview.chromium.org/14476011/diff/69001/build/android/gyp/copy_v1... build/android/gyp/copy_v17_resources.py:11: Or crbug.com/235118 . On 2013/05/03 21:23:04, cjhopman wrote: > Nit: add http:// (many linkifiers require it). Done. https://codereview.chromium.org/14476011/diff/69001/build/android/gyp/copy_v1... build/android/gyp/copy_v17_resources.py:22: def CopyResourcesInDir(input_dir, output_dir): On 2013/05/03 21:23:04, cjhopman wrote: > Nit: Maybe rename to CopyXmlResourcesInDir? Or is it obvious in context that > this only deals with xml resources? Done. https://codereview.chromium.org/14476011/diff/69001/build/android/gyp/generat... File build/android/gyp/generate_v14_resources.py (right): https://codereview.chromium.org/14476011/diff/69001/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:19: Please refer to crbug.com/235118 for the details. On 2013/05/03 21:23:04, cjhopman wrote: > Nit: http:// Done. https://codereview.chromium.org/14476011/diff/69001/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:33: # The list of the attributes were taken from go/android-rtl On 2013/05/03 21:23:04, cjhopman wrote: > go/android-rtl is an internal link. Is this list available publicly? If not, I'd > just add a comment on the bug and link to that here. Done. https://codereview.chromium.org/14476011/diff/69001/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:93: # However, there is a minidom bug that doesn't print namespace set by On 2013/05/03 21:23:04, cjhopman wrote: > Nit: is there some kind of minidom bug report you can link to here? Done.
Failed to trigger a try job on win_x64_rel HTTP Error 400: Bad Request
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/14476011/89001
Sorry for I got bad news for ya. Compile failed with a clobber build on win_x64_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/14476011/89001
Message was sent while issue was closed.
Change committed as 198325 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
