[webkit-reviews] review denied: [Bug 135929] import-w3c-tests doesn't handle relative paths to support files in ref files correctly : [Attachment 237605] Patch

[bugzilla-daemon at webkit.org]

Bem Jones-Bey <bjonesbe at adobe.com> has denied Rebecca Hauck
<rhauck at adobe.com>'s request for review:
Bug 135929: import-w3c-tests doesn't handle relative paths to support files in
ref files correctly
https://bugs.webkit.org/show_bug.cgi?id=135929

Attachment 237605: Patch
https://bugs.webkit.org/attachment.cgi?id=237605&action=review

------- Additional Comments from Bem Jones-Bey <bjonesbe at adobe.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=237605&action=review


> Tools/ChangeLog:8
> +	The recent refactor of the W3C test repo falsified a bunch of
assmumptions that

Fix the indentation here. (The style bot also complained. :-)

> Tools/Scripts/webkitpy/w3c/test_converter.py:40
> +def convert_for_webkit(new_path, filename, refrelpath, host=Host()):

Nit: I don't really like the name refrelpath. I think for consistency with
resources_relpath below, this should at least be ref_relpath. reference_relpath
would be better, I think. (I'd really like refernce_relative_path, but relpath
is used in a bunch of places already, so probably best to stick with that for
now.) It's up to you if you change it.

> Tools/Scripts/webkitpy/w3c/test_converter.py:174
> +		   if attr_name == 'src' and  not
attr_value.startswith('data:'):

Do you ever want to replace if you have a URL scheme at all? For example, if
you have http: or file:, you probably don't want to do the replacement, either.


Also, shouldn't you be checking the scheme in the case of a url() in
convert_relative_reffile_paths()? It seems like a helper function to actually
do the replacement (or not if you have a path that doesn't need a replacement)
would be a good idea.

> Tools/Scripts/webkitpy/w3c/test_parser.py:103
> +		       test_info['refrelpath'] =
self.filesystem.relpath(self.filename, ref_file).replace('../', '',
1).replace(testfile[1], '')

Why do you remove the first '../'?

> Tools/Scripts/webkitpy/w3c/test_parser.py:-137
> -	   """ Searches the file for all paths specified in url()'s, href or
src attributes."""

Why did you remove the search for href attributes?