WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
NEW
185387
webkitpy: Clarify construction of VersionNameMap
https://bugs.webkit.org/show_bug.cgi?id=185387
Summary
webkitpy: Clarify construction of VersionNameMap
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
Details
Formatted Diff
Diff
Patch
(28.23 KB, patch)
2018-05-10 08:27 PDT
,
Jonathan Bedard
jbedard
: commit-queue-
Details
Formatted Diff
Diff
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.
Top of Page
Format For Printing
XML
Clone This Bug