Some property values like "display: -webkit-grid;" need to include the vendor prefix in WebKit. Currently the import-w3c-tests is only prefixing the properties (example: "grid-row: 3;" => "-webkit-grid-row: 3;") but not the property values. CSSValueKeywords.in has the list of property values from where we can extract the prefixed ones: http://trac.webkit.org/browser/trunk/Source/WebCore/css/CSSValueKeywords.in
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.