[Webkit-unassigned] [Bug 135929] import-w3c-tests doesn't handle relative paths to support files in ref files correctly

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=135929





--- Comment #5 from Rebecca Hauck <rhauck at adobe.com>  2014-09-05 12:11:15 PST ---
(From update of attachment 237605)
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. :-)

Fixed in new patch.

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

In the new patch I've changed this to be reference_relpath to be consistent with resources_relpath. I'm also now passing in a reference_support dictionary here with 2 keys- reference_relpath and reference_support_files to the replacement code will replace them specifically rather than using regex's.

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

In the new patch, the converter gets the full list of ref support paths, so instead of this check, I check if the attr_value is any of the support file paths. Similar thing in convert_relative_reffile_paths. Also, I've expanded the list of tags that can have src attributes. May be overkill, but it's harmless.

>> 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 '../'?

Because filesystem.relpath (which is os.relpath) assumes that its arguments are directories. In the new patch, I've changed the call to relpath to be on each file's parent dir to avoid this confusion.

>> 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?

I assumed that hrefs are only in metadata and the only place where there's an actual path to a file in metadata is for reference files, which are retrieved and handled specifically through their rel=match attribute. I guess there could be a path in an a href though, so I put this back in the new patch.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.