Bug 64486

Created attachment 100714 [details] Patch

Created attachment 100720 [details] Patch

Comment on attachment 100720 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100720&action=review > Tools/ChangeLog:10 > + I've tried to write a patch for bug 64439 twice now, and both times > + I've ended up re-writing half the port system. So I'm breaking > + things up into smaller pieces, this being the first. Yay! > Tools/Scripts/webkitpy/layout_tests/port/win.py:54 > + def _version_string_from_windows_version_tuple(self, windows_version_tuple): > + if windows_version_tuple[:3] == (6, 1, 7600): > + return '7sp0' > + if windows_version_tuple[:2] == (6, 0): > + return 'vista' > + if windows_version_tuple[:2] == (5, 1): > + return 'xp' > + return None Maybe it would be clearer to use a dot-separated string instead of a tuple? That would have the advantage of requiring fewer conversions between strings, tuples, and lists. > Tools/Scripts/webkitpy/layout_tests/port/win_unittest.py:52 > + def mock_run_command(cmd): > + self.assertEquals(cmd, ['cmd', '/c', 'ver']) > + return "6.1.7600" Is it worth testing other version strings we recognize? Is it worth testing any version strings we don't recognize? > Tools/Scripts/webkitpy/layout_tests/port/win_unittest.py:61 > + # Failures log to the pyton error log, but we have no easy way to capture/test that. Typo: pyton > Tools/Scripts/webkitpy/layout_tests/port/win_unittest.py:74 > + self._assert_search_path(['win-xp', 'win-vista', 'win-7sp0', 'win', 'mac-snowleopard', 'mac'], 'xp') I think these _assert_search_path calls would be a little more readable if the order of arguments were flipped. (I.e., the version should come first.)

I thought I had landed this. Oops.

(In reply to comment #3) > (From update of attachment 100720 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100720&action=review > > Tools/Scripts/webkitpy/layout_tests/port/win.py:54 > > + def _version_string_from_windows_version_tuple(self, windows_version_tuple): > > + if windows_version_tuple[:3] == (6, 1, 7600): > > + return '7sp0' > > + if windows_version_tuple[:2] == (6, 0): > > + return 'vista' > > + if windows_version_tuple[:2] == (5, 1): > > + return 'xp' > > + return None > > Maybe it would be clearer to use a dot-separated string instead of a tuple? That would have the advantage of requiring fewer conversions between strings, tuples, and lists. This was written this way because it was originally written to work with os.getwindowsversion() instead of our manual string parsing of ver. I'm not sure it matters much. tuples are easy to compare between. But I guess strings are too? > > Tools/Scripts/webkitpy/layout_tests/port/win_unittest.py:52 > > + def mock_run_command(cmd): > > + self.assertEquals(cmd, ['cmd', '/c', 'ver']) > > + return "6.1.7600" > > Is it worth testing other version strings we recognize? Is it worth testing any version strings we don't recognize? Those are covered in a separate test. > > Tools/Scripts/webkitpy/layout_tests/port/win_unittest.py:61 > > + # Failures log to the pyton error log, but we have no easy way to capture/test that. > > Typo: pyton Will fix. > > Tools/Scripts/webkitpy/layout_tests/port/win_unittest.py:74 > > + self._assert_search_path(['win-xp', 'win-vista', 'win-7sp0', 'win', 'mac-snowleopard', 'mac'], 'xp') > > I think these _assert_search_path calls would be a little more readable if the order of arguments were flipped. (I.e., the version should come first.) Will fix.

Created attachment 103401 [details] Updated patch, preparing to land

Committed r92717: <http://trac.webkit.org/changeset/92717>

This resulted in a test-webkitpy regression which is tracked by bug 66325.