-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Manually map Safari versions in getMatchingBrowserVersion #16594
Conversation
This pull request has merge conflicts that must be resolved before it can be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the rest of the iOS/macOS mirroring is okay, actually, despite what I said earlier offline. In all other cases, we seem to be okay.
I've compared I don't think the 15.1→15 changes are worth reviewing at all, since that's just a bug. I'll make a list of the paths with 8→7 changes. This will also allow the mirror migration script to replace a lot more data. |
I hacked up
|
These 8→7 changes seemed worth reviewing / testing on iOS 7/8 on BrowserStack:
Then there's |
#16879 updates the IndexedDB data. |
It was implemented here: WebKit/WebKit@b195c16 https://github.com/WebKit/WebKit/blob/b195c162a9805acacbb7d32328ba6fbbc1df4453/Source/WebCore/Configurations/Version.xcconfig WebKit trunk version 538.8.0 maps to Safari 8 on both macOS and iOS. Confirmed with tested in iOS 7 and 8: mdn#16594 (comment) This was last updated based on the collector here: mdn#7929 But that was based on results from Safari 7.1: foolip/mdn-bcd-results@5067a4d
#16880 fixes getTrackById. |
They're probably worth reviewing? |
I… think this is wrong? I think it should be 15.1 for both macOS and iOS, but I haven't tested them recently. |
Ah, no: WebKit/WebKit@988a6bd is the commit that enabled it, and as the tags indicate that was in Safari 15.0 for macOS, but only Safari 15.1 for iOS. Turns out I should just trust the changes I made a while back: #13142 ;P |
It was implemented here: WebKit/WebKit@b195c16 https://github.com/WebKit/WebKit/blob/b195c162a9805acacbb7d32328ba6fbbc1df4453/Source/WebCore/Configurations/Version.xcconfig WebKit trunk version 538.8.0 maps to Safari 8 on both macOS and iOS. Confirmed with tested in iOS 7 and 8: #16594 (comment) This was last updated based on the collector here: #7929 But that was based on results from Safari 7.1: foolip/mdn-bcd-results@5067a4d
My reasoning was that any 15+15.1 pairs that we have are very likely just due to the mirroring script bug. Off-by-one versions further in the past will have been explicitly entered like this and reviewed at some point, making it more likely they're actually correct. |
@gsnedders sounds like at least one case of 15+15.1 is correct then? Maybe what we should do is check which such pairs existed before the mirroring migrations. |
I've run api.PerformanceNavigationTiming @gsnedders confirmed these are correct per #13142. |
This is not ideal, but it fixes the problem now. Tests are in place to ensure that if we fix it in some other way, there's no observable difference.
The remaining changes that will result from this are now:
|
I've sent #16959 to fix the api.Element.matches case. |
#16960 is to update css.properties.background-repeat.round_space |
Alright, sizes and srcset fixed in #16961, that's the last of it! |
Alright, the only remaining changes now are correct:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not a fan of the idea of placing this mapping in a script vs. the browser data, but it at least solves the problem right away. I'll follow up to move this into the browser JSON!
After mdn#16594 a lot more Safari data can be mirrored. Note that this just ran the script and also includes a small amount of non-Safari changes.
After #16594 a lot more Safari data can be mirrored. Note that this just ran the script and also includes a small amount of non-Safari changes.
This is not ideal, but it fixes the problem now. Tests are in place to
ensure that if we fix it in some other way, there's no observable
difference.