Bug 185387

Summary: webkitpy: Clarify construction of VersionNameMap
Product: WebKit Reporter: Daniel Bates <dbates>
Component: Tools / TestsAssignee: Jonathan Bedard <jbedard>
Status: NEW    
Severity: Normal CC: ews-watchlist, glenn, jbedard, lforschler, rniwa
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=185386
Attachments:
Description Flags
Patch
none
Patch jbedard: commit-queue-

Daniel Bates
Reported 2018-05-07 11:24:52 PDT
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.
Attachments
Patch (7.18 KB, patch)
2018-05-07 17:12 PDT, Jonathan Bedard 		no flags
DetailsFormatted DiffDiff
Patch (28.23 KB, patch)
2018-05-10 08:27 PDT, Jonathan Bedard 		jbedard: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Jonathan Bedard
Comment 1 2018-05-07 12:12:03 PDT
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
Comment 2 2018-05-07 17:12:48 PDT
Created attachment 339775 [details] Patch
Jonathan Bedard
Comment 3 2018-05-07 17:16:38 PDT
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
Comment 4 2018-05-10 08:27:32 PDT
Created attachment 340091 [details] Patch
Jonathan Bedard
Comment 5 2018-05-10 08:29:12 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.