[webkit-reviews] review denied: [Bug 54248] new-run-webkit-tests: clean up version handling for the --platform argument : [Attachment 82069] Patch

[bugzilla-daemon at webkit.org]

Mihai Parparita <mihaip at chromium.org> has denied Dirk Pranke
<dpranke at chromium.org>'s request for review:
Bug 54248: new-run-webkit-tests: clean up version handling for the --platform
argument
https://bugs.webkit.org/show_bug.cgi?id=54248

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

------- Additional Comments from Mihai Parparita <mihaip at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=82069&action=review

> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:57
> +	       self._version = port_name[port_name.index('-mac') + 4:]

Should this assert that the resulting version is in SUPPORTED_OS_VERSIONS?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:61
> +	   fallback_paths = {

Can this be moved to be a class-level attribute like SUPPORTED_OS_VERSIONS?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:-91
> -	   # FIXME: It's strange that this string is -version, not just
version.

I agree with Adam, if we're cleaning this up anyway, can we switch the
convention for version to drop the leading dash?

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_win.py:52
>> +	    (5, 0): '-xp',
> 
> 5.0 is Windows 2000, so "-xp" seems strange. Do you need to include 5.2
(XP64, Server 2003) in here, too?

Agreed. We don't have any Win2K bots, so that version should be an error, the
same way that Tiger is for chromium-mac.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_win.py:69
> +	       self._version = port_name[port_name.index('-win') + 4:]

Ditto about asserting here.