Stop reporting more recent user agent version strings, freezing the version at a specific moment in time. We should do this for a number of reasons: (1) User Agent sniffing is a terrible way to determine whether a browser supports certain features. (2) Bad User Agent sniffing code on the web create compatibility problems every time we update the versions. (3) Overly-specific version information provides useful fingerprinting data while providing almost no user benefit.
<rdar://problem/34550617>
Created attachment 328364 [details] Patch
Created attachment 328366 [details] Patch
Comment on attachment 328366 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328366&action=review > Source/WebCore/page/cocoa/UserAgent.mm:55 > +#if __MAC_OS_X_VERSION_MIN_REQUIRED < 101304 Looking at the WebKit builds that Apple has shipped so far, they seem to always build with a __MAC_OS_X_VERSION_MIN_REQUIRED ending with “00” (both in system updates and in Safari Technology Preview), so this condition is practically equivalent to __MAC_OS_X_VERSION_MIN_REQUIRED < 101300. So this will make all builds targeting some version of High Sierra or later to use 10_13_4, which seems fine, but is better expressed by explicitly writing 101300. On the other hand, when targeting Sierra and El Capitan, we already know their currently shipping versions, so it seems unnecessary to refer to systemMarketingVersion(). A way to write this that is both explicit and will lend itself to gradual simplification as WebKit drops support for older OSes and that follows our convention of putting the “present and future” case ahead of the “legacy” case would be something like #if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300 return "10_13_4"; #elif __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200 return "10_12_6"; #else return "10_11_6"; #endif
Comment on attachment 328366 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328366&action=review >> Source/WebCore/page/cocoa/UserAgent.mm:55 >> +#if __MAC_OS_X_VERSION_MIN_REQUIRED < 101304 > > Looking at the WebKit builds that Apple has shipped so far, they seem to always build with a __MAC_OS_X_VERSION_MIN_REQUIRED ending with “00” (both in system updates and in Safari Technology Preview), so this condition is practically equivalent to __MAC_OS_X_VERSION_MIN_REQUIRED < 101300. So this will make all builds targeting some version of High Sierra or later to use 10_13_4, which seems fine, but is better expressed by explicitly writing 101300. On the other hand, when targeting Sierra and El Capitan, we already know their currently shipping versions, so it seems unnecessary to refer to systemMarketingVersion(). > > A way to write this that is both explicit and will lend itself to gradual simplification as WebKit drops support for older OSes and that follows our convention of putting the “present and future” case ahead of the “legacy” case would be something like > #if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300 > return "10_13_4"; > #elif __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200 > return "10_12_6"; > #else > return "10_11_6"; > #endif I like this much better! :-)
Created attachment 328485 [details] Patch w/ Dan's improvements
Comment on attachment 328485 [details] Patch w/ Dan's improvements View in context: https://bugs.webkit.org/attachment.cgi?id=328485&action=review > Source/WebCore/page/cocoa/UserAgent.mm:37 > +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300 I think this means that the UserAgent numbers are fixed from the system that built WebKit and not from the system that WebKit is running on? Is that what we expect? I guess it is irrelevant as time goes by though, so the simplicity is nice. > Source/WebCore/page/cocoa/UserAgent.mm:38 > + return "10_13_4"; Nit: ASCIILiteral() can save a copy. (For each of these char* => WTF::String conversions from a literal): https://trac.webkit.org/wiki/EfficientStrings > Source/WebCore/page/cocoa/UserAgent.mm:51 > + return "605.1.15"; Nit: ASCIILiteral() can save a copy. > Source/WebKitLegacy/mac/ChangeLog:11 > + creatin the User Agent string. Typo: creatin > LayoutTests/fast/dom/navigator-userAgent-frozen.html:64 > + if (window.testRunner) > + testRunner.dumpAsText(); > + > + testVersion(navigator.userAgent); Style: Weird indentation.
(In reply to Joseph Pecoraro from comment #7) > Comment on attachment 328485 [details] > Patch w/ Dan's improvements > > View in context: > https://bugs.webkit.org/attachment.cgi?id=328485&action=review > > > Source/WebCore/page/cocoa/UserAgent.mm:37 > > +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300 > > I think this means that the UserAgent numbers are fixed from the system that > built WebKit and not from the system that WebKit is running on? __MAC_OS_X_VERSION_MIN_REQUIRED is based on the deployment target. WebKit built for one major macOS release can’t run on earlier releases and isn’t supported on later releases, so there is nothing to query at runtime that isn’t know at build time.
Comment on attachment 328485 [details] Patch w/ Dan's improvements View in context: https://bugs.webkit.org/attachment.cgi?id=328485&action=review >> Source/WebCore/page/cocoa/UserAgent.mm:38 >> + return "10_13_4"; > > Nit: ASCIILiteral() can save a copy. (For each of these char* => WTF::String conversions from a literal): > https://trac.webkit.org/wiki/EfficientStrings Will do! >> Source/WebCore/page/cocoa/UserAgent.mm:51 >> + return "605.1.15"; > > Nit: ASCIILiteral() can save a copy. Will do. >> Source/WebKitLegacy/mac/ChangeLog:11 >> + creatin the User Agent string. > > Typo: creatin Oops! >> LayoutTests/fast/dom/navigator-userAgent-frozen.html:64 >> + testVersion(navigator.userAgent); > > Style: Weird indentation. Whoops! Fixed.
Created attachment 328500 [details] Patch
> > > Source/WebCore/page/cocoa/UserAgent.mm:37 > > > +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300 > > > > I think this means that the UserAgent numbers are fixed from the system that > > built WebKit and not from the system that WebKit is running on? > > __MAC_OS_X_VERSION_MIN_REQUIRED is based on the deployment target. WebKit > built for one major macOS release can’t run on earlier releases and isn’t > supported on later releases, so there is nothing to query at runtime that > isn’t know at build time. I see. Thanks for the explanation!
Comment on attachment 328500 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328500&action=review r=me, but since neither of us are a WebKit Owner I suspect you'll need a nod from one. > LayoutTests/fast/dom/navigator-userAgent-frozen.html:34 > + var versionComponents = version.split('.'); > + if (versionComponents.length == 0) { I don't think String.prototype.split can ever return an empty array, it would always return at least 1 value (the original string). But you could compare `length <= 1` and I think that would be fine for this test.
Created attachment 328533 [details] Patch for landing
Comment on attachment 328533 [details] Patch for landing Clearing flags on attachment: 328533 Committed r225562: <https://trac.webkit.org/changeset/225562>
All reviewed patches have been landed. Closing bug.
Note that OS marketing version was re-added to UserAgent in Bug 182629.