Bug 137600 – import-w3c-tests doesn't prefix property values

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Manuel Rego Casasnovas]

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

[Manuel Rego Casasnovas]

Created attachment 239609 [details]
Patch

[Bem Jones-Bey]

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. :-)

[Manuel Rego Casasnovas]

(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 :-)

[Manuel Rego Casasnovas]

Created attachment 239658 [details]
Patch

[Bem Jones-Bey]

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.

[WebKit Commit Bot]

Comment on attachment 239658 [details]
Patch

Clearing flags on attachment: 239658

Committed r174672: <http://trac.webkit.org/changeset/174672>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.