Implement config changes to UA #2037
Conversation
Core/UserAgentManager.swift
Outdated
| private func closestLogic(forUrl url: URL?, | ||
| isDesktop: Bool, | ||
| privacyConfig: PrivacyConfiguration = ContentBlocking.shared.privacyConfigurationManager.privacyConfig) -> String { | ||
| let customUAEnabled = privacyConfig.isEnabled(featureKey: .customUserAgent) |
There was a problem hiding this comment.
From the logic it turns out that func closestLogic is only called when the customUAEnabled is enabled.
|
Overall it looks fine. One small comment and the tests are failing due to user agent changes. |
Core/UserAgentManager.swift
Outdated
| return defaultSafari | ||
| } | ||
|
|
||
| private func ddgFixedLogic(isDesktop: Bool) -> String { |
There was a problem hiding this comment.
This method should be the closest policy, fixed will contain the DuckDuckGo part.
Maybe the confusion came in that this is now a fixed UA, but the fixed policy is about repairing known issues in our existing UA.
There was a problem hiding this comment.
This method shouldn't be used when the user is <16.6 also.
| if let match = regex?.firstMatch(in: baseAgent, range: NSRange(baseAgent.startIndex..., in: baseAgent)) { | ||
| let range = Range(match.range(at: 1), in: baseAgent) | ||
| if let range = range { | ||
| return String(baseAgent[range]) |
There was a problem hiding this comment.
I've not validated yet this but the approach looks correct.
| return true | ||
| private var shouldUseUpdatedLogic: Bool { | ||
| guard let webKitVersion else { return false } | ||
| return webKitVersion.versionCompare("605.1.15") != .orderedAscending |
There was a problem hiding this comment.
I think this will be true for "605.1.14" and "605.1.16" etc which is incorrect.
I think the following might be what we're wanting:
| return webKitVersion.versionCompare("605.1.15") != .orderedAscending | |
| return webKitVersion.versionCompare("605.1.14") == .orderedDescending |
There was a problem hiding this comment.
I believe this is fine - added two tests that should validate that - please could you take a look at them?
| @@ -28,6 +28,8 @@ final class UserAgentTests: XCTestCase { | |||
| private struct DefaultAgent { | |||
| static let mobile = "Mozilla/5.0 (iPhone; CPU iPhone OS 12_4 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Mobile/15E148" | |||
| static let tablet = "Mozilla/5.0 (iPad; CPU OS 12_4 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Mobile/15E148" | |||
| static let oldWebkitVersionMobile = "Mozilla/5.0 (iPhone; CPU iPhone OS 12_4 like Mac OS X) AppleWebKit/605.1.14 (KHTML, like Gecko) Mobile/15E148" | |||
| static let newWebkitVersionMobile = "Mozilla/5.0 (iPhone; CPU iPhone OS 12_4 like Mac OS X) AppleWebKit/605.1.16 (KHTML, like Gecko) Mobile/15E148" | |||
There was a problem hiding this comment.
Can you also add a test for AppleWebKit/605.1.15 too? I'm a little nervous based on that comparison
There was a problem hiding this comment.
of course, I didn't add it initially, because pretty much everything else testes this use case
| }) { return ddgFixedLogic(forUrl: url, isDesktop: isDesktop, privacyConfig: privacyConfig) } | ||
|
|
||
| if closestUserAgentVersions(forConfig: privacyConfig).contains(statistics.atbWeek ?? "") { | ||
| if canUseClosestLogic { |
There was a problem hiding this comment.
You could bury this check within the closestLogic as you have for ddgFixedLogic and just keep the single other check within ddgFixedLogic to keep the fall-back behaviour correct.
But also... we're going to rip this out quickly again hopefully too; so feel free to ignore.
There was a problem hiding this comment.
That was my initial idea, but it caused a side effect that made one test fail - when the closest user agent "could not be used" because of old webkit version it fallbacked to oldUserAgentLogic which returned the old UA string which contained "DuckDuckGo/7" component. So inside ddgFixedLogic we were appending yet another DuckDuckGo/7 component.
So my idea was to make it as specific as it can be, because we want to remove it soon.
There was a problem hiding this comment.
Yeah you'd still need the check in ddgFixedLogic to keep the same structure I think that should work (but also not tried).
Task/Issue URL: https://app.asana.com/0/0/1205442309571983/f
CC: @jonathanKingston
Description:
It allows to modify UA through config by either using the old logic, safari UA or safari UA with DDG component.
Steps to test this PR: TBD
1.
2.