Bug 161789 – W3C test downloader should be able to import specific files/sub-directories in a skipped directory

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

W3C test downloader should be able to import specific files/sub-directories in a skipped directory

[youenn fablet]

Created attachment 288396 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 288396 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288396&action=review

r=me assuming both problems are addressed.

> Tools/Scripts/webkitpy/w3c/test_downloader.py:137
> +            potential_copy_paths = [copy_directory for copy_directory in copy_paths if copy_directory in relative_path]

I don't think "copy_directory in relative_path" is right. We should do relative_path.startswith(copy_directory) instead.
If there is / differences, then we should be using filesystem helper functions instead.

> Tools/Scripts/webkitpy/w3c/test_downloader.py:141
> +            longest_copy_path_length = len(max(potential_copy_paths, key=len))
> +            return not any([skip_directory for skip_directory in self.paths_to_skip if skip_directory in relative_path and len(skip_directory) > longest_copy_path_length])

This is such a convoluted way of checking the condition we need to be checking.
Relying on the length to check that skip_directory is more specific than a path in potential_copy_paths is not obvious at all.
I think it's better to just have two nested loops instead for clarify unless the performance is actually important.

[youenn fablet]

Created attachment 291083 [details]
Patch for landing

[youenn fablet]

Thanks for the review.

(In reply to comment #2)
> Comment on attachment 288396 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288396&action=review
> 
> r=me assuming both problems are addressed.
> 
> > Tools/Scripts/webkitpy/w3c/test_downloader.py:137
> > +            potential_copy_paths = [copy_directory for copy_directory in copy_paths if copy_directory in relative_path]
> 
> I don't think "copy_directory in relative_path" is right. We should do
> relative_path.startswith(copy_directory) instead.
> If there is / differences, then we should be using filesystem helper
> functions instead.

Right. Fixed.

> > Tools/Scripts/webkitpy/w3c/test_downloader.py:141
> > +            longest_copy_path_length = len(max(potential_copy_paths, key=len))
> > +            return not any([skip_directory for skip_directory in self.paths_to_skip if skip_directory in relative_path and len(skip_directory) > longest_copy_path_length])
> 
> This is such a convoluted way of checking the condition we need to be
> checking.
> Relying on the length to check that skip_directory is more specific than a
> path in potential_copy_paths is not obvious at all.
> I think it's better to just have two nested loops instead for clarify unless
> the performance is actually important.

I updated the patch by getting the longest matching copy path, the longest matching skip path, and checking whether the copy path starts with the skip path.

[WebKit Commit Bot]

Comment on attachment 291083 [details]
Patch for landing

Clearing flags on attachment: 291083

Committed r206993: <http://trac.webkit.org/changeset/206993>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.