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.

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

Remove duplicated methods after performance optimizations #5017

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -673,7 +673,7 @@ class WebViewRequestInterceptorTest {

val uri = "host.com".toUri()
whenever(mockRequest.url).thenReturn(uri)
whenever(mockCloakedCnameDetector.detectCnameCloakedHost(anyString(), any())).thenReturn(null)
whenever(mockCloakedCnameDetector.detectCnameCloakedHost(any(), any())).thenReturn(null)

val response = testee.shouldIntercept(
request = mockRequest,
Expand All @@ -682,7 +682,7 @@ class WebViewRequestInterceptorTest {
webViewClientListener = null,
)

verify(mockCloakedCnameDetector).detectCnameCloakedHost("foo.com", uri)
verify(mockCloakedCnameDetector).detectCnameCloakedHost("foo.com".toUri(), uri)
assertRequestCanContinueToLoad(response)
}

Expand All @@ -694,7 +694,7 @@ class WebViewRequestInterceptorTest {

val uri = "host.com".toUri()
whenever(mockRequest.url).thenReturn(uri)
whenever(mockCloakedCnameDetector.detectCnameCloakedHost(anyString(), any())).thenReturn("uncloaked-host.com")
whenever(mockCloakedCnameDetector.detectCnameCloakedHost(any(), any())).thenReturn("uncloaked-host.com".toUri())

val response = testee.shouldIntercept(
request = mockRequest,
Expand All @@ -703,26 +703,26 @@ class WebViewRequestInterceptorTest {
webViewClientListener = null,
)

verify(mockCloakedCnameDetector).detectCnameCloakedHost("foo.com", uri)
verify(mockCloakedCnameDetector).detectCnameCloakedHost("foo.com".toUri(), uri)
assertRequestCanContinueToLoad(response)
}

private suspend fun assertRequestBlockedWhenUncloakedHostFound() {
configureShouldNotUpgrade()
configureBlockedCnameTrackingEvent()

val uri = "host.com".toUri()
val uri = "http://host.com".toUri()
whenever(mockRequest.url).thenReturn(uri)
whenever(mockCloakedCnameDetector.detectCnameCloakedHost(anyString(), any())).thenReturn("uncloaked-host.com")
whenever(mockCloakedCnameDetector.detectCnameCloakedHost(any(), any())).thenReturn("http://uncloaked-host.com".toUri())

val response = testee.shouldIntercept(
request = mockRequest,
documentUri = "foo.com".toUri(),
documentUri = "http://foo.com".toUri(),
webView = webView,
webViewClientListener = null,
)

verify(mockCloakedCnameDetector).detectCnameCloakedHost("foo.com", uri)
verify(mockCloakedCnameDetector).detectCnameCloakedHost("http://foo.com".toUri(), uri)
assertCancelledResponse(response)
}

Expand Down Expand Up @@ -753,13 +753,12 @@ class WebViewRequestInterceptorTest {
surrogateId = "testId",
)
whenever(mockRequest.isForMainFrame).thenReturn(false)
whenever(mockTrackerDetector.evaluate(anyString(), any<Uri>(), eq(true), anyMap())).thenReturn(trackingEvent)
whenever(mockTrackerDetector.evaluate(any<Uri>(), any<Uri>(), eq(true), anyMap())).thenReturn(trackingEvent)
}

private fun configureNull() {
whenever(mockRequest.isForMainFrame).thenReturn(false)
whenever(mockTrackerDetector.evaluate(anyString(), any<Uri>(), eq(true), anyMap())).thenReturn(null)
whenever(mockTrackerDetector.evaluate(any<Uri>(), any<Uri>(), eq(true), anyMap())).thenReturn(null)
}

private fun configureBlockedCnameTrackingEvent() {
Expand All @@ -781,7 +780,7 @@ class WebViewRequestInterceptorTest {
surrogateId = null,
)
whenever(mockRequest.isForMainFrame).thenReturn(false)
whenever(mockTrackerDetector.evaluate(anyString(), any<Uri>(), eq(false), anyMap())).thenReturn(trackingEvent)
whenever(mockTrackerDetector.evaluate(any<Uri>(), any<Uri>(), eq(false), anyMap())).thenReturn(trackingEvent)
}

private fun configureUrlExistsInTheStack(uri: Uri = validUri()) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package com.duckduckgo.app.browser.pageloadpixel

import com.duckduckgo.app.pixels.remoteconfig.OptimizeTrackerEvaluationRCWrapper
import com.duckduckgo.autoconsent.api.Autoconsent
import com.duckduckgo.browser.api.WebViewVersionProvider
import com.duckduckgo.common.test.CoroutineTestRule
Expand Down Expand Up @@ -36,10 +35,6 @@ class PageLoadedHandlerTest {
TestScope(),
coroutinesTestRule.testDispatcherProvider,
autoconsent,
object : OptimizeTrackerEvaluationRCWrapper {
override val enabled: Boolean
get() = true
},
)

@Before
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ class DomainsReferenceTest(private val testCase: TestCase) {
val entities = tdsJson.jsonToEntities()
val domainEntities = tdsJson.jsonToDomainEntities()
val cnameEntities = tdsJson.jsonToCnameEntities()
val client = TdsClient(Client.ClientName.TDS, trackers, RealUrlToTypeMapper(), false)
val client = TdsClient(Client.ClientName.TDS, trackers, RealUrlToTypeMapper())

tdsEntityDao.insertAll(entities)
tdsDomainEntityDao.insertAll(domainEntities)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ class SurrogatesReferenceTest(private val testCase: TestCase) {
val entities = tdsJson.jsonToEntities()
val domainEntities = tdsJson.jsonToDomainEntities()
val cnameEntities = tdsJson.jsonToCnameEntities()
val client = TdsClient(Client.ClientName.TDS, trackers, RealUrlToTypeMapper(), false)
val client = TdsClient(Client.ClientName.TDS, trackers, RealUrlToTypeMapper())

tdsEntityDao.insertAll(entities)
tdsDomainEntityDao.insertAll(domainEntities)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ class WebViewRequestInterceptor(
trackingEvent.status == TrackerStatus.ALLOWED ||
trackingEvent.status == TrackerStatus.SAME_ENTITY_ALLOWED
) {
cloakedCnameDetector.detectCnameCloakedHost(documentUrl.toString(), request.url)?.let { uncloakedHost ->
cloakedCnameDetector.detectCnameCloakedHost(documentUrl, request.url)?.let { uncloakedHost ->
trackingEvent(request, documentUrl, webViewClientListener, false, uncloakedHost)?.let { cloakedTrackingEvent ->
if (cloakedTrackingEvent.status == TrackerStatus.BLOCKED) {
return blockRequest(cloakedTrackingEvent, request, webViewClientListener)
Expand Down Expand Up @@ -253,23 +253,7 @@ class WebViewRequestInterceptor(
documentUrl: Uri?,
webViewClientListener: WebViewClientListener?,
checkFirstParty: Boolean = true,
): TrackingEvent? {
val url = request.url
if (request.isForMainFrame || documentUrl == null) {
return null
}

val trackingEvent = trackerDetector.evaluate(url, documentUrl, checkFirstParty, request.requestHeaders) ?: return null
webViewClientListener?.trackerDetected(trackingEvent)
return trackingEvent
}

private fun trackingEvent(
request: WebResourceRequest,
documentUrl: Uri?,
webViewClientListener: WebViewClientListener?,
checkFirstParty: Boolean = true,
url: String = request.url.toString(),
url: Uri = request.url,
): TrackingEvent? {
if (request.isForMainFrame || documentUrl == null) {
return null
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ package com.duckduckgo.app.browser.pageloadpixel
import com.duckduckgo.app.browser.UriString
import com.duckduckgo.app.browser.pageloadpixel.PageLoadedSites.Companion.sites
import com.duckduckgo.app.di.AppCoroutineScope
import com.duckduckgo.app.pixels.remoteconfig.OptimizeTrackerEvaluationRCWrapper
import com.duckduckgo.autoconsent.api.Autoconsent
import com.duckduckgo.browser.api.WebViewVersionProvider
import com.duckduckgo.common.utils.DispatcherProvider
Expand All @@ -42,7 +41,6 @@ class RealPageLoadedHandler @Inject constructor(
@AppCoroutineScope private val appCoroutineScope: CoroutineScope,
private val dispatcherProvider: DispatcherProvider,
private val autoconsent: Autoconsent,
private val optimizeTrackerEvaluationRCWrapper: OptimizeTrackerEvaluationRCWrapper,
) : PageLoadedHandler {

override fun onPageLoaded(url: String, title: String?, start: Long, end: Long) {
Expand All @@ -54,7 +52,6 @@ class RealPageLoadedHandler @Inject constructor(
webviewVersion = webViewVersionProvider.getMajorVersion(),
appVersion = deviceInfo.appVersion,
cpmEnabled = autoconsent.isAutoconsentEnabled(),
trackerOptimizationEnabled = optimizeTrackerEvaluationRCWrapper.enabled,
),
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,6 @@ class PageLoadedPixelEntity(
val appVersion: String,
val elapsedTime: Long,
val webviewVersion: String,
val trackerOptimizationEnabled: Boolean,
val trackerOptimizationEnabled: Boolean = true,
val cpmEnabled: Boolean,
)
Original file line number Diff line number Diff line change
Expand Up @@ -51,20 +51,4 @@ interface AndroidBrowserConfigFeature {
*/
@Toggle.DefaultValue(false)
fun screenLock(): Toggle

/**
* @return `true` when the remote config has the global "optimizeTrackerEvaluation" androidBrowserConfig
* sub-feature flag enabled
* If the remote feature is not present defaults to `false`
*/
@Toggle.DefaultValue(false)
fun optimizeTrackerEvaluation(): Toggle

/**
* @return `true` when the remote config has the global "optimizeTrackerEvaluationV2" androidBrowserConfig
* sub-feature flag enabled
* If the remote feature is not present defaults to `false`
*/
@Toggle.DefaultValue(false)
fun optimizeTrackerEvaluationV2(): Toggle
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import javax.inject.Inject
import timber.log.Timber

interface CloakedCnameDetector {
fun detectCnameCloakedHost(documentUrl: String?, url: Uri): String?
fun detectCnameCloakedHost(documentUrl: Uri?, url: Uri): Uri?
}

@ContributesBinding(AppScope::class)
Expand All @@ -39,24 +39,23 @@ class CloakedCnameDetectorImpl @Inject constructor(
private val userAllowListRepository: UserAllowListRepository,
) : CloakedCnameDetector {

override fun detectCnameCloakedHost(documentUrl: String?, url: Uri): String? {
if (documentUrl != null && trackerAllowlist.isAnException(documentUrl, url.toString()) ||
override fun detectCnameCloakedHost(documentUrl: Uri?, url: Uri): Uri? {
if (documentUrl != null && trackerAllowlist.isAnException(documentUrl.toString(), url.toString()) ||
userAllowListRepository.isUriInUserAllowList(url)
) { return null }

url.host?.let { host ->
tdsCnameEntityDao.get(host)?.let { cnameEntity ->
var uncloakedHostName = cnameEntity.uncloakedHostName
Timber.v("$host is a CNAME cloaked host. Uncloaked host name: $uncloakedHostName")
url.path?.let { path ->
uncloakedHostName += path
val builder = Uri.Builder()
cnameEntity.uncloakedHostName.let {
builder.authority(it)
Timber.v("$host is a CNAME cloaked host. Uncloaked host name: $it")
}
uncloakedHostName = if (url.scheme != null) {
"${url.scheme}://$uncloakedHostName"
} else {
"${UrlScheme.http}://$uncloakedHostName"

url.pathSegments?.forEach { path ->
builder.appendPath(path)
}
return uncloakedHostName
return builder.scheme(url.scheme ?: UrlScheme.http).build()
}
}
return null
Expand Down
22 changes: 7 additions & 15 deletions app/src/main/java/com/duckduckgo/app/trackerdetection/TdsClient.kt
Original file line number Diff line number Diff line change
Expand Up @@ -28,22 +28,17 @@ class TdsClient(
override val name: Client.ClientName,
private val trackers: List<TdsTracker>,
private val urlToTypeMapper: UrlToTypeMapper,
private val optimizeTrackerEvaluation: Boolean,
) : Client {

override fun matches(
url: String,
documentUrl: Uri,
requestHeaders: Map<String, String>,
): Client.Result {
val tracker = if (optimizeTrackerEvaluation) {
val domain = host(url)?.let { Domain(it) }
trackers.firstOrNull {
domain?.let { domain -> sameOrSubdomain(domain, it.domain) } ?: false
} ?: return Client.Result(matches = false, isATracker = false)
} else {
trackers.firstOrNull { sameOrSubdomain(url, it.domain.value) } ?: return Client.Result(matches = false, isATracker = false)
}
val domain = host(url)?.let { Domain(it) }
val tracker = trackers.firstOrNull {
domain?.let { domain -> sameOrSubdomain(domain, it.domain) } ?: false
} ?: return Client.Result(matches = false, isATracker = false)
val matches = matchesTrackerEntry(tracker, url, documentUrl, requestHeaders)
return Client.Result(
matches = matches.shouldBlock,
Expand All @@ -59,12 +54,9 @@ class TdsClient(
documentUrl: Uri,
requestHeaders: Map<String, String>,
): Client.Result {
val tracker = if (optimizeTrackerEvaluation) {
val domain = url.host?.let { Domain(it) }
trackers.firstOrNull { sameOrSubdomain(domain, it.domain) } ?: return Client.Result(matches = false, isATracker = false)
} else {
trackers.firstOrNull { sameOrSubdomain(url, it.domain.value) } ?: return Client.Result(matches = false, isATracker = false)
}
val domain = url.host?.let { Domain(it) }
val tracker = trackers.firstOrNull { sameOrSubdomain(domain, it.domain) } ?: return Client.Result(matches = false, isATracker = false)

val matches = matchesTrackerEntry(tracker, url.toString(), documentUrl, requestHeaders)
return Client.Result(
matches = matches.shouldBlock,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import com.duckduckgo.app.browser.R
import com.duckduckgo.app.di.AppCoroutineScope
import com.duckduckgo.app.global.db.AppDatabase
import com.duckduckgo.app.lifecycle.MainProcessLifecycleObserver
import com.duckduckgo.app.pixels.remoteconfig.OptimizeTrackerEvaluationRCWrapper
import com.duckduckgo.app.trackerdetection.api.TdsJson
import com.duckduckgo.app.trackerdetection.db.TdsCnameEntityDao
import com.duckduckgo.app.trackerdetection.db.TdsDomainEntityDao
Expand Down Expand Up @@ -60,7 +59,6 @@ class TrackerDataLoader @Inject constructor(
private val moshi: Moshi,
private val urlToTypeMapper: UrlToTypeMapper,
private val dispatcherProvider: DispatcherProvider,
private val optimizeTrackerEvaluationRCWrapper: OptimizeTrackerEvaluationRCWrapper,
) : MainProcessLifecycleObserver {

override fun onCreate(owner: LifecycleOwner) {
Expand Down Expand Up @@ -108,7 +106,7 @@ class TrackerDataLoader @Inject constructor(
fun loadTrackers() {
val trackers = tdsTrackerDao.getAll()
Timber.d("Loaded ${trackers.size} tds trackers from DB")
val client = TdsClient(Client.ClientName.TDS, trackers, urlToTypeMapper, optimizeTrackerEvaluationRCWrapper.enabled)
val client = TdsClient(Client.ClientName.TDS, trackers, urlToTypeMapper)
trackerDetector.addClient(client)
}

Expand Down
Loading
Loading