Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14969022:
[Android] Warning if Left/Right attributes are used in layout and style xmls. (Closed)
|
Created:
7 years, 7 months ago by Kibeom Kim (inactive) Modified:
7 years, 7 months ago Reviewers:
newt (away) CC:
chromium-reviews, craigdh+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Android] Warning if Left/Right attributes are used in layout and style xmls.
Since crbug.com/235118, Left/Right attributes are deprecated in favor of Start/End.
So warn if it contains Left/Right.
BUG=239606
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=199452
Patch Set 1 #
Total comments: 20
Patch Set 2 : fixes based on newt's comments #
Total comments: 10
Patch Set 3 : (newt's comment) a commnet fix and added .value #
Total comments: 1
Patch Set 4 : added missed ".value" again #Messages
Total messages: 10 (0 generated)
https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... File build/android/gyp/generate_v14_resources.py (right): https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... build/android/gyp/generate_v14_resources.py:93: WarnDeprecatedAttribute(name, value, 'xxx') oops, 'xxx' to file
https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... File build/android/gyp/generate_v14_resources.py (right): https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... build/android/gyp/generate_v14_resources.py:30: # Note that we are assumping 'android:' is an alias of "assuming" https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... build/android/gyp/generate_v14_resources.py:69: print >> sys.stderr, ('warning: ' +file + ' should use ' + nit: space before "file" https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... build/android/gyp/generate_v14_resources.py:72: elif name in GRAVITY_ATTRIBUTES and ('left' in value or 'right' in value): elif name in GRAVITY_ATTRIBUTES and value in ('left', 'right'): https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... build/android/gyp/generate_v14_resources.py:88: name = item_element.attributes['name'] add ".value" to the end of the line? https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... build/android/gyp/generate_v14_resources.py:93: WarnDeprecatedAttribute(name, value, 'xxx') 'xxx' -> input_file https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... build/android/gyp/generate_v14_resources.py:106: dom = minidom.parse(input_file) you could use ElementTree and xpath here: tree = ElementTree.parse(input_file) for item in tree.findall('resources/style/items'): ... https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... build/android/gyp/generate_v14_resources.py:133: dom = minidom.parse(input_file) instead of parsing input_file twice, I'd remove these three lines. Then, in GenerateV14StyleResource(), check if input_file has any style elements. if it doesn't, no need generate the output file.
https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... File build/android/gyp/generate_v14_resources.py (right): https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... build/android/gyp/generate_v14_resources.py:67: def WarnDeprecatedAttribute(name, value, file): also: it's better to use filename instead of file, which has a built-in meaning in python
https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... File build/android/gyp/generate_v14_resources.py (right): https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... build/android/gyp/generate_v14_resources.py:30: # Note that we are assumping 'android:' is an alias of On 2013/05/10 01:15:49, newt wrote: > "assuming" Done. https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... build/android/gyp/generate_v14_resources.py:67: def WarnDeprecatedAttribute(name, value, file): On 2013/05/10 01:16:43, newt wrote: > also: it's better to use filename instead of file, which has a built-in meaning > in python Done. https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... build/android/gyp/generate_v14_resources.py:69: print >> sys.stderr, ('warning: ' +file + ' should use ' + On 2013/05/10 01:15:49, newt wrote: > nit: space before "file" Done. https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... build/android/gyp/generate_v14_resources.py:72: elif name in GRAVITY_ATTRIBUTES and ('left' in value or 'right' in value): On 2013/05/10 01:15:49, newt wrote: > elif name in GRAVITY_ATTRIBUTES and value in ('left', 'right'): But what if value == 'left|top' ? https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... build/android/gyp/generate_v14_resources.py:88: name = item_element.attributes['name'] On 2013/05/10 01:15:49, newt wrote: > add ".value" to the end of the line? Done. Thanks for catching this! https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... build/android/gyp/generate_v14_resources.py:93: WarnDeprecatedAttribute(name, value, 'xxx') On 2013/05/10 01:15:49, newt wrote: > 'xxx' -> input_file Done. https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... build/android/gyp/generate_v14_resources.py:93: WarnDeprecatedAttribute(name, value, 'xxx') On 2013/05/10 00:31:39, Kibeom Kim wrote: > oops, 'xxx' to file Done. https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... build/android/gyp/generate_v14_resources.py:106: dom = minidom.parse(input_file) On 2013/05/10 01:15:49, newt wrote: > you could use ElementTree and xpath here: > > tree = ElementTree.parse(input_file) > for item in tree.findall('resources/style/items'): > ... Actually, I used cElementTree before because it's the fastest built-in xml parser http://effbot.org/zone/celementtree.htm , but then it uses xmlns:ns0="http://schemas.android.com/apk/res/android" by default and to set 'android' instead of 'ns0' I had to call xml.etree.ElementTree.register_namespace(...) which is a v2.7 function. So I switched to minidom. https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... build/android/gyp/generate_v14_resources.py:133: dom = minidom.parse(input_file) On 2013/05/10 01:15:49, newt wrote: > instead of parsing input_file twice, I'd remove these three lines. Then, in > GenerateV14StyleResource(), check if input_file has any style elements. if it > doesn't, no need generate the output file. Done.
a few (somewhat tangential) suggestions. also, be sure to verify that GenerateV14StyleResource() actually works, both in the normal case and the warning case. https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... File build/android/gyp/generate_v14_resources.py (right): https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... build/android/gyp/generate_v14_resources.py:72: elif name in GRAVITY_ATTRIBUTES and ('left' in value or 'right' in value): On 2013/05/10 02:14:27, Kibeom Kim wrote: > On 2013/05/10 01:15:49, newt wrote: > > elif name in GRAVITY_ATTRIBUTES and value in ('left', 'right'): > > But what if value == 'left|top' ? ah, good point. looks good as is. https://codereview.chromium.org/14969022/diff/1/build/android/gyp/generate_v1... build/android/gyp/generate_v14_resources.py:106: dom = minidom.parse(input_file) On 2013/05/10 02:14:27, Kibeom Kim wrote: > On 2013/05/10 01:15:49, newt wrote: > > you could use ElementTree and xpath here: > > > > tree = ElementTree.parse(input_file) > > for item in tree.findall('resources/style/items'): > > ... > > Actually, I used cElementTree before because it's the fastest built-in xml > parser http://effbot.org/zone/celementtree.htm , but then it uses > xmlns:ns0="http://schemas.android.com/apk/res/android" by default and to set > 'android' instead of 'ns0' I had to call > xml.etree.ElementTree.register_namespace(...) which is a v2.7 function. So I > switched to minidom. ah. looks like there's a solution in python 2.6: look for "ET._namespace_map[uri] = prefix" in http://effbot.org/zone/element-namespaces.htm . But I think it's fine to leave this as it is. https://codereview.chromium.org/14969022/diff/7001/build/android/gyp/generate... File build/android/gyp/generate_v14_resources.py (right): https://codereview.chromium.org/14969022/diff/7001/build/android/gyp/generate... build/android/gyp/generate_v14_resources.py:83: If the input_filename is not a style resource, do nothing. "If input_filename does not contain style resources, do nothing." https://codereview.chromium.org/14969022/diff/7001/build/android/gyp/generate... build/android/gyp/generate_v14_resources.py:88: if not style_elements: you should be able to remove this check, once you make the change to write output_filename only if dom was modified https://codereview.chromium.org/14969022/diff/7001/build/android/gyp/generate... build/android/gyp/generate_v14_resources.py:96: item_element.attributes['name'] = ATTRIBUTES_TO_MAP[name] ".value" here too? https://codereview.chromium.org/14969022/diff/7001/build/android/gyp/generate... build/android/gyp/generate_v14_resources.py:100: build_utils.MakeDirectory(os.path.dirname(output_filename)) we only need to write output_filename, if we modified dom. otherwise, we're just wasting APK space. https://codereview.chromium.org/14969022/diff/7001/build/android/gyp/generate... build/android/gyp/generate_v14_resources.py:127: build_utils.MakeDirectory(os.path.dirname(output_filename)) same deal here: we only need to create the output file if we modified dom. otherwise, we're just wasting APK space.
Verified by cherry-picking https://codereview.chromium.org/14752024/ https://codereview.chromium.org/14969022/diff/7001/build/android/gyp/generate... File build/android/gyp/generate_v14_resources.py (right): https://codereview.chromium.org/14969022/diff/7001/build/android/gyp/generate... build/android/gyp/generate_v14_resources.py:83: If the input_filename is not a style resource, do nothing. On 2013/05/10 06:13:50, newt wrote: > "If input_filename does not contain style resources, do nothing." Done. https://codereview.chromium.org/14969022/diff/7001/build/android/gyp/generate... build/android/gyp/generate_v14_resources.py:88: if not style_elements: This is kind of paired with the skipping logic in copy_v17_resources.py(line 30-31) at least currently. (more comments below) https://codereview.chromium.org/14969022/diff/7001/build/android/gyp/generate... build/android/gyp/generate_v14_resources.py:96: item_element.attributes['name'] = ATTRIBUTES_TO_MAP[name] On 2013/05/10 06:13:50, newt wrote: > ".value" here too? I just noticed that it works with and without .value. I'll put it anyways. Done. https://codereview.chromium.org/14969022/diff/7001/build/android/gyp/generate... build/android/gyp/generate_v14_resources.py:100: build_utils.MakeDirectory(os.path.dirname(output_filename)) On 2013/05/10 06:13:50, newt wrote: > we only need to write output_filename, if we modified dom. otherwise, we're just > wasting APK space. Yes you're right. Actually I thought about this, and I'd prefer to make a separate patch for that. Because I also have to selectively copy only modified files, in copy_v17_resources.py. So it makes sense to combine copy_v17_resources.py and generate_v14_resources.py. I was thinking to do that for M29 because I don't want to make big changes for M28, but if you think this is desired for M28 release, I'll do that as a separate patch.
1 nit. are you going to use .value after all? :) otherwise, lgtm https://codereview.chromium.org/14969022/diff/7001/build/android/gyp/generate... File build/android/gyp/generate_v14_resources.py (right): https://codereview.chromium.org/14969022/diff/7001/build/android/gyp/generate... build/android/gyp/generate_v14_resources.py:100: build_utils.MakeDirectory(os.path.dirname(output_filename)) On 2013/05/10 06:44:26, Kibeom Kim wrote: > On 2013/05/10 06:13:50, newt wrote: > > we only need to write output_filename, if we modified dom. otherwise, we're > just > > wasting APK space. > > Yes you're right. Actually I thought about this, and I'd prefer to make a > separate patch for that. Because I also have to selectively copy only modified > files, in copy_v17_resources.py. So it makes sense to combine > copy_v17_resources.py and generate_v14_resources.py. I was thinking to do that > for M29 because I don't want to make big changes for M28, but if you think this > is desired for M28 release, I'll do that as a separate patch. I'm happy with addressing this in another CL. https://codereview.chromium.org/14969022/diff/12001/build/android/gyp/generat... File build/android/gyp/generate_v14_resources.py (right): https://codereview.chromium.org/14969022/diff/12001/build/android/gyp/generat... build/android/gyp/generate_v14_resources.py:96: item_element.attributes['name'] = ATTRIBUTES_TO_MAP[name] .value? or not. whatever works.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/14969022/14002
Message was sent while issue was closed.
Change committed as 199452 |
