[webkit-reviews] review denied: [Bug 111513] Create a script to import W3C tests : [Attachment 201279] cleaned-up patch for relanding

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 111513: Create a script to import W3C tests
https://bugs.webkit.org/show_bug.cgi?id=111513

Attachment 201279: cleaned-up patch for relanding
https://bugs.webkit.org/attachment.cgi?id=201279&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=201279&action=review


> Tools/Scripts/webkitpy/w3c/test_parser.py:50
> +	       f = open(file_to_parse)
> +	       html = f.read()
> +	       f.close()

This duplicates the code in TestConverter.

> Tools/Scripts/webkitpy/w3c/test_parser.py:64
> +	   if test_contents is not None:

Nit:
if test_contents:

> Tools/Scripts/webkitpy/w3c/test_parser.py:67
> +	   if ref_contents is not None:

Ditto.

> Tools/Scripts/webkitpy/w3c/test_parser.py:72
> +	   if len(matches) != 0:

Nit: Don’t compare with 0.

> Tools/Scripts/webkitpy/w3c/test_parser.py:103
> +	   elif self.options['all'] is True and \
> +		not('-ref' in self.filename) and \
> +		not('reference' in self.filename):

It seems like all of this fit in one line. If not, then we have a wrong
indentation.

> Tools/Scripts/webkitpy/w3c/test_parser.py:107
> +	   # Courtesy check and warning if there are any mismatches
> +	   mismatches = self.get_reftests(False)

We do! r- because of this.

> Tools/Scripts/webkitpy/w3c/test_parser.py:108
> +	   if len(mismatches) != 0:

Nit: Don’t compare with 0.

> Tools/Scripts/webkitpy/w3c/test_parser.py:119
> +	   if find_match:
> +	       return self.test_doc.findAll(rel='match')
> +	   else:
> +	       return self.test_doc.findAll(rel='mismatch')

Why not
return self.test_doc.findAll(rel=‘match’ if find_match else 'mismatch’)

> Tools/Scripts/webkitpy/w3c/test_parser.py:127
> +	   if
self.test_doc.find(src=re.compile('[\'\"/]?/resources/testharness')) is None:
> +	       return False
> +	   else:
> +	       return True

return not
self.test_doc.find(src=re.compile('[\'\"/]?/resources/testharness’)).

> Tools/Scripts/webkitpy/w3c/test_parser.py:141
> +	   # Look for tags with src or href attributes
> +	   src_attrs = doc.findAll(src=re.compile('.*'))
> +	   href_attrs = doc.findAll(href=re.compile('.*'))
> +
> +	   # Look for urls

These two comments repeat the code.

> Tools/Scripts/webkitpy/w3c/test_parser.py:157
> +	       if not(path.startswith('http:')) and \
> +		  not(path.startswith('mailto:')):

Fit in one line.

> Tools/Scripts/webkitpy/w3c/test_parser_unittest.py:53
> +	   # Verify the correct test info was found

Nit: Repeat the code, and ditto elsewhere in the tests.