[webkit-reviews] review denied: [Bug 51222] new-run-webkit-tests : need to support more modifiers (gpu, x86-64, etc.) : [Attachment 77297] rebase to head post webkittools -> tools rename

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 51222: new-run-webkit-tests : need to support more modifiers (gpu, x86-64,
etc.)
https://bugs.webkit.org/show_bug.cgi?id=51222

Attachment 77297: rebase to head post webkittools -> tools rename
https://bugs.webkit.org/attachment.cgi?id=77297&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=77297&action=review

r- for my nits + mihaip and eric's unaddressed comments. Overall, this is much
cleaner.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:534
>> +	    parts = line.split(':')
> 
> Though not a regression from your patch, should this be line.split(':', 1)
since you only look at parts[0] and [1]?

Here and below, it would be an error in the test_expectations.txt file if there
were a line with more than one : or =. Maybe as a separate patch we should
modify the error check above to check that there is exactly 1 : and 1 =?

In either case, seems better for a separate patch.

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:599
> +    def _read_line(self, line, lineno, matcher, overrides_allowed):

Nit: This does a lot more than just read the line. Maybe call this
_process_line or something like that?

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:827
> +	   prev_entry = self._test_list_paths[test]
> +	   prev_base_path, prev_num_matches, prev_lineno = prev_entry

Nit: this can just be 1 line since you don't use prev_entry elsewhere.
Remember, we don't have an 80 col requirement. :)

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:839
> +	   if (not overrides_allowed or test in self._overridding_tests):

This would be easier to read if you inverted the if-statement as most of this
wouldn't need to be so indented. It would be:

if (...):
    # We have seen this path ...
    # ...
    # ...
    return False

...

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:890
> +class ModifierMatcher(object):

This is a fairly involved class. Perhaps it should go in it's own file?
ModifierMatchResult should probably go in that file as well?

> Tools/Scripts/webkitpy/layout_tests/port/base.py:879
> +class TestConfiguration(object):

This is also a pretty involved class. Put it in it's own file?