Skip to content
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.

Sign up for GitHub

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

Implement config changes to UA #2037

Merged
merged 8 commits into from
Oct 3, 2023
Merged

Implement config changes to UA #2037

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0b88ec5
Implement config changes to UA
jaceklyp Sep 25, 2023
d70da24
Fix PR comments
jaceklyp Oct 1, 2023
f86b67c
Add tests
jaceklyp Oct 1, 2023
8276f59
Add existing users experimentation part
jaceklyp Oct 1, 2023
cb75566
Fix atb logic
jaceklyp Oct 2, 2023
e6d53bf
Change device profile logic
jaceklyp Oct 2, 2023
a1e761e
Add tests for webkit version
jaceklyp Oct 2, 2023
3e60d53
Add missing test
jaceklyp Oct 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 22 additions & 14 deletions Core/UserAgentManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,11 @@ struct UserAgent {
}) { return ddgFixedLogic(forUrl: url, isDesktop: isDesktop, privacyConfig: privacyConfig) }

if closestUserAgentVersions(forConfig: privacyConfig).contains(statistics.atbWeek ?? "") {
return closestLogic(forUrl: url, isDesktop: isDesktop, privacyConfig: privacyConfig)
if canUseClosestLogic {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jaceklyp jaceklyp Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you'd still need the check in ddgFixedLogic to keep the same structure I think that should work (but also not tried).

return closestLogic(forUrl: url, isDesktop: isDesktop, privacyConfig: privacyConfig)
} else {
return oldLogic(forUrl: url, isDesktop: isDesktop, privacyConfig: privacyConfig)
}
}

if ddgFixedUserAgentVersions(forConfig: privacyConfig).contains(statistics.atbWeek ?? "") {
Expand All @@ -208,7 +212,12 @@ struct UserAgent {
switch defaultPolicy(forConfig: privacyConfig) {
case .ddg: return oldLogic(forUrl: url, isDesktop: isDesktop, privacyConfig: privacyConfig)
case .ddgFixed: return ddgFixedLogic(forUrl: url, isDesktop: isDesktop, privacyConfig: privacyConfig)
case .closest: return closestLogic(forUrl: url, isDesktop: isDesktop, privacyConfig: privacyConfig)
case .closest:
if canUseClosestLogic {
return closestLogic(forUrl: url, isDesktop: isDesktop, privacyConfig: privacyConfig)
} else {
return oldLogic(forUrl: url, isDesktop: isDesktop, privacyConfig: privacyConfig)
}
}
}

Expand Down Expand Up @@ -240,31 +249,30 @@ struct UserAgent {
}
let resolvedApplicationComponent = !omitApplicationComponent ? applicationComponent : nil

var defaultSafari = closestLogic(forUrl: url, isDesktop: isDesktop, privacyConfig: privacyConfig)
// If the UA should have DuckDuckGo append it prior to Safari
if let resolvedApplicationComponent {
if let index = defaultSafari.range(of: "Safari")?.lowerBound {
defaultSafari.insert(contentsOf: resolvedApplicationComponent + " ", at: index)
if canUseClosestLogic {
var defaultSafari = closestLogic(forUrl: url, isDesktop: isDesktop, privacyConfig: privacyConfig)
// If the UA should have DuckDuckGo append it prior to Safari
if let resolvedApplicationComponent {
if let index = defaultSafari.range(of: "Safari")?.lowerBound {
defaultSafari.insert(contentsOf: resolvedApplicationComponent + " ", at: index)
}
}
return defaultSafari
} else {
return oldLogic(forUrl: url, isDesktop: isDesktop, privacyConfig: privacyConfig)
}
return defaultSafari
}

private func closestLogic(forUrl url: URL?,
isDesktop: Bool,
privacyConfig: PrivacyConfiguration) -> String {
guard shouldUseUpdatedLogic else {
return oldLogic(forUrl: url, isDesktop: isDesktop, privacyConfig: privacyConfig)
}

if isDesktop {
return "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/16.5 Safari/605.1.15"
}

return "Mozilla/5.0 (" + deviceProfile + ") AppleWebKit/605.1.15 (KHTML, like Gecko) Version/16.5 Mobile/15E148 Safari/604.1"
}

private var shouldUseUpdatedLogic: Bool {
private var canUseClosestLogic: Bool {
guard let webKitVersion else { return false }
return webKitVersion.versionCompare("605.1.15") != .orderedAscending
Copy link
Collaborator

@jonathanKingston jonathanKingston Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
return webKitVersion.versionCompare("605.1.15") != .orderedAscending
return webKitVersion.versionCompare("605.1.14") == .orderedDescending

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is fine - added two tests that should validate that - please could you take a look at them?

}
Expand Down
29 changes: 23 additions & 6 deletions DuckDuckGoTests/UserAgentTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,14 @@
import BrowserServicesKit
@testable import Core

// swiftlint:disable file_length type_body_length

Check failure on line 25 in DuckDuckGoTests/UserAgentTests.swift

View workflow job for this annotation

GitHub Actions / SwiftLint 

Blanket Disable Command Violation: The disabled 'type_body_length' rule should be re-enabled before the end of the file (blanket_disable_command)
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"

Check failure on line 31 in DuckDuckGoTests/UserAgentTests.swift

View workflow job for this annotation

GitHub Actions / SwiftLint 

Line Length Violation: Line should be 150 characters or less; currently it has 155 characters (line_length)
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"

Check failure on line 32 in DuckDuckGoTests/UserAgentTests.swift

View workflow job for this annotation

GitHub Actions / SwiftLint 

Line Length Violation: Line should be 150 characters or less; currently it has 155 characters (line_length)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a test for AppleWebKit/605.1.15 too? I'm a little nervous based on that comparison

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

of course, I didn't add it initially, because pretty much everything else testes this use case

}

private struct ExpectedAgent {
Expand All @@ -37,21 +39,24 @@
static let mobile = "Mozilla/5.0 (iPhone; CPU iPhone OS 12_4 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.4 Mobile/15E148 DuckDuckGo/7 Safari/605.1.15"
static let tablet = "Mozilla/5.0 (iPad; CPU OS 12_4 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.4 Mobile/15E148 DuckDuckGo/7 Safari/605.1.15"
static let desktop = "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.4 DuckDuckGo/7 Safari/605.1.15"

static let oldWebkitVersionMobile = "Mozilla/5.0 (iPhone; CPU iPhone OS 12_4 like Mac OS X) AppleWebKit/605.1.14 (KHTML, like Gecko) Version/12.4 Mobile/15E148 DuckDuckGo/7 Safari/605.1.14"
static let newWebkitVersionMobile = "Mozilla/5.0 (iPhone; CPU iPhone OS 12_4 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/16.5 Mobile/15E148 DuckDuckGo/7 Safari/604.1"

static let mobileNoApplication = "Mozilla/5.0 (iPhone; CPU iPhone OS 12_4 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.4 Mobile/15E148 Safari/605.1.15"

// Based on fallback constants in UserAgent
static let mobileFallback = "Mozilla/5.0 (iPhone; CPU iPhone OS 13_5 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/13.5 Mobile/15E148 DuckDuckGo/7 Safari/605.1.15"
static let desktopFallback = "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/13.5 DuckDuckGo/7 Safari/605.1.15"

static let mobileFixed = "Mozilla/5.0 (iPhone; CPU iPhone OS 16_6 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/16.5 Mobile/15E148 DuckDuckGo/7 Safari/604.1"
static let tabletFixed = "Mozilla/5.0 (iPad; CPU OS 16_6 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/16.5 Mobile/15E148 DuckDuckGo/7 Safari/604.1"
static let mobileFixed = "Mozilla/5.0 (iPhone; CPU iPhone OS 12_4 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/16.5 Mobile/15E148 DuckDuckGo/7 Safari/604.1"
static let tabletFixed = "Mozilla/5.0 (iPad; CPU OS 12_4 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/16.5 Mobile/15E148 DuckDuckGo/7 Safari/604.1"
static let desktopFixed = "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/16.5 DuckDuckGo/7 Safari/605.1.15"

static let mobileClosest = "Mozilla/5.0 (iPhone; CPU iPhone OS 16_6 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/16.5 Mobile/15E148 Safari/604.1"
static let tabletClosest = "Mozilla/5.0 (iPad; CPU OS 16_6 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/16.5 Mobile/15E148 Safari/604.1"
static let mobileClosest = "Mozilla/5.0 (iPhone; CPU iPhone OS 12_4 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/16.5 Mobile/15E148 Safari/604.1"
static let tabletClosest = "Mozilla/5.0 (iPad; CPU OS 12_4 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/16.5 Mobile/15E148 Safari/604.1"
static let desktopClosest = "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/16.5 Safari/605.1.15"



// swiftlint:enable line_length
}

Expand Down Expand Up @@ -422,4 +427,16 @@
XCTAssertEqual(ExpectedAgent.mobileFixed, testee.agent(forUrl: Constants.url, isDesktop: false, privacyConfig: config))
}

func testWhenOldWebKitVersionThenDefaultMobileAgentUsed() {
let testee = UserAgent(defaultAgent: DefaultAgent.oldWebkitVersionMobile)
let config = makePrivacyConfig(from: ddgFixedConfig)
XCTAssertEqual(ExpectedAgent.oldWebkitVersionMobile, testee.agent(forUrl: Constants.url, isDesktop: false, privacyConfig: config))
}

func testWhenNewerWebKitVersionThenFixedAgentUsed() {
let testee = UserAgent(defaultAgent: DefaultAgent.newWebkitVersionMobile)
let config = makePrivacyConfig(from: ddgFixedConfig)
XCTAssertEqual(ExpectedAgent.newWebkitVersionMobile, testee.agent(forUrl: Constants.url, isDesktop: false, privacyConfig: config))
}

}
Loading