Bug 185387 – webkitpy: Clarify construction of VersionNameMap

• Component: Tools / Tests
• Status: NEW

[Daniel Bates]

From my understanding the purpose of VersionNameMap is to map an OS version number to a human readable name (e.g. High Sierra) and vice-versa. Looking at operations to map a version number to a name I count at least 8 ways to do this:

Let P be a PlatformInfo object.

VersionNameMap().to_name(...)
VersionNameMap().to_name(..., platform="...")
VersionNameMap(P).to_name(...)
VersionNameMap(P).to_name(..., platform="...")
VersionNameMap.map().to_name(...)
VersionNameMap.map().to_name(..., platform="...")
VersionNameMap.map(P).to_name(...)
VersionNameMap.map(P).to_name(..., platform="...")

And quickly glancing through the code these invocation may give different results regardless of whether both P and platform="..." are in agreement depending on the presence of Apple Additions.

[Jonathan Bedard]

Some of these invocations are only exist because Python doesn't allow a constructor to be hidden. It is always the case, especially for Apple's ports, that the VersionNameMap.map(...) function should be used, specifically because of apple_additions.

In some of the other cases, you're explicitly overriding the default PlatformInfo  object. Given the fact that you shouldn't be using the default constructor, we can definitely eliminate 1 and 2.  There is also an argument to be made that we shouldn't define a default PlatformInfo object if the user does not provide us with one.  That would eliminate 5 and 6.

However, we definitely need to keep the ability to override the default platform in to_name.  As mentioned in <https://bugs.webkit.org/show_bug.cgi?id=185386>, we rely on this in iOS where we don't have an iOS PlatformInfo object when we need to extract version information.

[Jonathan Bedard]

Created attachment 339775 [details]
Patch

[Jonathan Bedard]

I've uploaded a potential improvement which eliminates 1 and 2, and indicates through a comment that 3 and 4 should be avoided.

There is another way to clarify this code, although it would require more synchronization with apple_additions.  We could have apple_additions update the underlying mapping of the VersionNameMap in the constructor. This would totally eliminate the map(...) function, leaving 1-4 but eliminating 5-8 explicitly.

[Jonathan Bedard]

Created attachment 340091 [details]
Patch

[Jonathan Bedard]

Comment on attachment 340091 [details]
Patch

This patch combines what will eventually be 3 changes (2 associated with this bug and one associated with <https://bugs.webkit.org/show_bug.cgi?id=185386>).  I wanted to first ensure that the direction of this patch satisfies Dan's concerns about the clarity of VersionNameMap before moving forward.