[webkit-reviews] review granted: [Bug 179621] webkitpy: Better name-version mapping : [Attachment 329134] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has granted Jonathan Bedard
<jbedard at apple.com>'s request for review:
Bug 179621: webkitpy: Better name-version mapping
https://bugs.webkit.org/show_bug.cgi?id=179621

Attachment 329134: Patch

https://bugs.webkit.org/attachment.cgi?id=329134&action=review




--- Comment #41 from David Kilzer (:ddkilzer) <ddkilzer at webkit.org> ---
Comment on attachment 329134
  --> https://bugs.webkit.org/attachment.cgi?id=329134
Patch

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

r=me with a few questions below.

> Tools/Scripts/webkitpy/common/system/platforminfo_unittest.py:81
> -	   self.assertNotEquals(info.os_version, '')
> +	   if info.is_mac() or info.is_win():
> +	       self.assertNotEquals(info.os_version, None)

Would this make more sense or be easier to read?

    if info.is_mac() or info.is_win():
	self.assertIsNotNone(info.os_version)

> Tools/Scripts/webkitpy/port/base.py:776
> -    def version(self):
> +    def version_name(self):
>	   """Returns a string indicating the version of a given platform, e.g.
>	   'leopard' or 'xp'.
>  
>	   This is used to help identify the exact port when parsing test
>	   expectations, determining search paths, and logging information."""
> -	   return self._version
> +	   if self._os_version is None:
> +	       return None
> +	   return
VersionNameMap.map(self.host.platform).to_name(self._os_version,
table=PUBLIC_TABLE)

Can this be @memoized, or will it change during the life of the process?

> Tools/Scripts/webkitpy/port/factory_unittest.py:59
> -	   self.assert_port(port_name='mac', os_name='mac', os_version='lion',
cls=mac.MacPort)
> -	   self.assert_port(port_name=None,  os_name='mac', os_version='lion',
cls=mac.MacPort)
> +	   self.assert_port(port_name='mac', os_name='mac',
os_version=Version(10, 7), cls=mac.MacPort)
> +	   self.assert_port(port_name=None,  os_name='mac',
os_version=Version(10, 7), cls=mac.MacPort)

I think 'lion' is a bit more descriptive here, but the alternative seems to be
this, which is way too verbose:

    VersionNameMap.map().from_name('lion')

I guess I wish there was some kind of a (static) helper function like this:

    Version.fromName('lion')

This isn't necessary to fix; just thinking about whether this change would make
the tests easier or harder to update (or to simply read) later.

I guess one benefit of doing this would be not having to import PUBLIC_TABLE,
VersionNameMap in as many places.  (I'm not sure if this is possible given the
current dependencies and use of apple_additions(), though.)

Or maybe this is what you warned me about when you told me about the patch, and
I forgot! :)

> Tools/Scripts/webkitpy/port/factory_unittest.py:66
> -	   self.assert_port(port_name='win', os_name='win', os_version='xp',
cls=win.WinPort)
> -	   self.assert_port(port_name=None, os_name='win', os_version='xp',
cls=win.WinPort)
> -	   self.assert_port(port_name=None, os_name='win', os_version='xp',
options=self.webkit_options, cls=win.WinPort)
> +	   self.assert_port(port_name='win', os_name='win',
os_version=Version(5, 1), cls=win.WinPort)
> +	   self.assert_port(port_name=None, os_name='win',
os_version=Version(5, 1), cls=win.WinPort)
> +	   self.assert_port(port_name=None, os_name='win',
os_version=Version(5, 1), options=self.webkit_options, cls=win.WinPort)

I don't think I'd know/remember that Windows XP was version 5.1.  (See above.
:)

> Tools/Scripts/webkitpy/port/ios.py:41
> +    VERSION_MIN = Version(11)
> +    VERSION_MAX = Version(12)

Is VERSION_MAX correct here?  (I ask because I asked about the mac port below
using (10, 14).)

> Tools/Scripts/webkitpy/port/ios_simulator.py:250
> -	       if mac_os_version in ['elcapitan', 'yosemite', 'mavericks']:
> +	       if mac_os_version < Version(10, 11):
>		   time.sleep(2.5)

Here's where I wish Version.fromName('elcapitan') existed, too.

Shouldn't this be change to the following (since the old check was inclusive of
El Capitan, but the new check is exclusive of El Capitan):

	if mac_os_version < Version(10, 12):

> Tools/Scripts/webkitpy/port/mac.py:48
> -    VERSION_FALLBACK_ORDER = ['mac-snowleopard', 'mac-lion',
'mac-mountainlion', 'mac-mavericks', 'mac-yosemite', 'mac-elcapitan',
'mac-sierra', 'mac-highsierra']
> +    VERSION_MIN = Version(10, 6)
> +    VERSION_MAX = Version(10, 14)

Is VERSION_MAX correct here?  The old VERSION_FALLBACK_ORDER stopped at (10,
13).

> Tools/Scripts/webkitpy/port/win.py:-439
> -    VERSION_FALLBACK_ORDER = ["wincairo-" + os_name.lower() for os_name in
VersionNameMap.map().names()]

Does removing this mean that folder names on disk have to change, too?