Summary: | import-w3c-tests doesn't prefix property values | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Manuel Rego Casasnovas <rego> | ||||||
Component: | Tools / Tests | Assignee: | Manuel Rego Casasnovas <rego> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bjonesbe, commit-queue, glenn, rhauck, youennf | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Manuel Rego Casasnovas
2014-10-10 02:17:09 PDT
Created attachment 239609 [details]
Patch
Comment on attachment 239609 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239609&action=review I haven't looked through all of the test changes yet, but here's a first pass. > Tools/Scripts/webkitpy/w3c/test_converter.py:87 > + property_values_regex = '(:\s*|^\s*)(' + "|".join(prop.replace('-webkit-', '') for prop in self.prefixed_property_values) + ')(\s*;|\s*}|\s*$)' > + self.property_values_re = re.compile(property_values_regex) Nit: I think this should be singular (property_value_re/property_value_regex). Since the other one is prop_re, I'm torn a bit on if this should be "property", to match webkit style better, or if it should be "prop" for consistency with existing code. Your call on that one. > Tools/Scripts/webkitpy/w3c/test_converter.py:95 > + def read_webkit_prefixed_css_property_list(self, contents): Why not pass in the file name instead of the contents? It would make the call sites of this method much more readable. > Tools/Scripts/webkitpy/w3c/test_converter.py:143 > + _log.info(' converting property %s', prop) > + > + text = ''.join(text_chunks) > + converted_property_values = set() > + text_chunks = [] > + cur_pos = 0 > + for m in self.property_values_re.finditer(text): > + text_chunks.extend([text[cur_pos:m.start()], m.group(1), '-webkit-', m.group(2), m.group(3)]) > + converted_property_values.add(m.group(2)) > + cur_pos = m.end() > + text_chunks.append(text[cur_pos:]) > + > + for value in converted_property_values: > + _log.info(' converting property value %s', value) I don't like the code duplication. You should at least factor the common code out into a helper method and just call it twice. Ideally, we'd only iterate once, but that's tricky, and might be premature optimization, so I'm not asking for that. :-) (In reply to comment #2) > (From update of attachment 239609 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=239609&action=review > > I haven't looked through all of the test changes yet, but here's a first pass. Thanks for the quick review! > > Tools/Scripts/webkitpy/w3c/test_converter.py:87 > > + property_values_regex = '(:\s*|^\s*)(' + "|".join(prop.replace('-webkit-', '') for prop in self.prefixed_property_values) + ')(\s*;|\s*}|\s*$)' > > + self.property_values_re = re.compile(property_values_regex) > > Nit: I think this should be singular (property_value_re/property_value_regex). Using singular here. > Since the other one is prop_re, I'm torn a bit on if this should be "property", to match webkit style better, or if it should be "prop" for consistency with existing code. Your call on that one. I'm keeping "prop" and using "prop_value" for this case, as there are more variables with short names in this script. Probably a bigger refactoring should be done at some point. > > Tools/Scripts/webkitpy/w3c/test_converter.py:95 > > + def read_webkit_prefixed_css_property_list(self, contents): > > Why not pass in the file name instead of the contents? It would make the call sites of this method much more readable. You're right, it makes things simpler. > > Tools/Scripts/webkitpy/w3c/test_converter.py:143 > > + _log.info(' converting property %s', prop) > > + > > + text = ''.join(text_chunks) > > + converted_property_values = set() > > + text_chunks = [] > > + cur_pos = 0 > > + for m in self.property_values_re.finditer(text): > > + text_chunks.extend([text[cur_pos:m.start()], m.group(1), '-webkit-', m.group(2), m.group(3)]) > > + converted_property_values.add(m.group(2)) > > + cur_pos = m.end() > > + text_chunks.append(text[cur_pos:]) > > + > > + for value in converted_property_values: > > + _log.info(' converting property value %s', value) > > I don't like the code duplication. You should at least factor the common code out into a helper method and just call it twice. > > Ideally, we'd only iterate once, but that's tricky, and might be premature optimization, so I'm not asking for that. :-) Fair enough :-) Created attachment 239658 [details]
Patch
Comment on attachment 239658 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239658&action=review Looks good. r=me. > Tools/Scripts/webkitpy/w3c/test_converter.py:125 > + return (converted_properties[0], converted_property_values[0], converted_property_values[1]) One of these days, we should refactor this to return an object with proper field names instead of a tuple. Comment on attachment 239658 [details] Patch Clearing flags on attachment: 239658 Committed r174672: <http://trac.webkit.org/changeset/174672> All reviewed patches have been landed. Closing bug. |