From 364f5588571f5551bec424171ee22dea3d37a70a Mon Sep 17 00:00:00 2001 From: Grigory Kruglov Date: Tue, 11 Jan 2022 16:01:32 -0800 Subject: [PATCH] [fenix] For https://github.com/mozilla-mobile/fenix/issues/22795: Stop trimming login origins on sorting Due to the async nature (??) of the trimming code, this is causing severe performance issues during search. Looking back through commits, doesn't seem like there's a particularly good reason we were trimming here. All I could find is https://github.com/mozilla-mobile/fenix/pull/9986 (comment) which is lacking explanation of why this is actually useful. And currently, we're dealing with an origin (not a full url when this was initially written, I think), i.e. https://accounts.firefox.com vs https://accounts.firefox.com/signin. So, the suffix stripping isn't even doing much beyond removing com in vast majority of cases. So, seems like all of this trimming stuff can be cleaned up. --- app/src/main/java/org/mozilla/fenix/ext/String.kt | 10 ---------- .../settings/logins/SavedLoginsSortingStrategyMenu.kt | 3 +-- .../mozilla/fenix/settings/logins/SortingStrategy.kt | 7 ++----- app/src/main/java/org/mozilla/fenix/utils/Settings.kt | 3 +-- app/src/test/java/org/mozilla/fenix/ext/StringTest.kt | 7 ------- .../fenix/settings/logins/LoginsFragmentStoreTest.kt | 4 ++-- .../fenix/settings/logins/LoginsListControllerTest.kt | 11 ++--------- .../settings/logins/SavedLoginsInteractorTest.kt | 6 ++---- .../logins/SavedLoginsSortingStrategyMenuTest.kt | 8 +------- 9 files changed, 11 insertions(+), 48 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/ext/String.kt b/app/src/main/java/org/mozilla/fenix/ext/String.kt index 04bf08c3ef..c9afbd8b31 100644 --- a/app/src/main/java/org/mozilla/fenix/ext/String.kt +++ b/app/src/main/java/org/mozilla/fenix/ext/String.kt @@ -16,9 +16,7 @@ import kotlinx.coroutines.withContext import mozilla.components.concept.fetch.Client import mozilla.components.concept.fetch.Request import mozilla.components.lib.publicsuffixlist.PublicSuffixList -import mozilla.components.lib.publicsuffixlist.ext.urlToTrimmedHost import mozilla.components.support.ktx.android.net.hostWithoutCommonPrefixes -import org.mozilla.fenix.perf.runBlockingIncrement import java.io.IOException import java.net.IDN import java.util.Locale @@ -91,14 +89,6 @@ private fun Uri.isIpv6(): Boolean { return host.isNotEmpty() && host.contains(":") } -/** - * Trim a host's prefix and suffix - */ -fun String.urlToTrimmedHost(publicSuffixList: PublicSuffixList): String = - runBlockingIncrement { - urlToTrimmedHost(publicSuffixList).await() - } - /** * Trims a URL string of its scheme and common prefixes. * diff --git a/app/src/main/java/org/mozilla/fenix/settings/logins/SavedLoginsSortingStrategyMenu.kt b/app/src/main/java/org/mozilla/fenix/settings/logins/SavedLoginsSortingStrategyMenu.kt index 8e5fbe21da..f34a7d00f0 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/logins/SavedLoginsSortingStrategyMenu.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/logins/SavedLoginsSortingStrategyMenu.kt @@ -12,7 +12,6 @@ import mozilla.components.concept.menu.candidate.TextMenuCandidate import mozilla.components.concept.menu.candidate.TextStyle import mozilla.components.support.ktx.android.content.getColorFromAttr import org.mozilla.fenix.R -import org.mozilla.fenix.ext.components import org.mozilla.fenix.settings.logins.interactor.SavedLoginsInteractor class SavedLoginsSortingStrategyMenu( @@ -51,7 +50,7 @@ class SavedLoginsSortingStrategyMenu( effect = if (itemToHighlight == Item.AlphabeticallySort) highlight else null ) { savedLoginsInteractor.onSortingStrategyChanged( - SortingStrategy.Alphabetically(context.components.publicSuffixList) + SortingStrategy.Alphabetically ) }, TextMenuCandidate( diff --git a/app/src/main/java/org/mozilla/fenix/settings/logins/SortingStrategy.kt b/app/src/main/java/org/mozilla/fenix/settings/logins/SortingStrategy.kt index eae2ff2cef..2e684eee96 100644 --- a/app/src/main/java/org/mozilla/fenix/settings/logins/SortingStrategy.kt +++ b/app/src/main/java/org/mozilla/fenix/settings/logins/SortingStrategy.kt @@ -4,15 +4,12 @@ package org.mozilla.fenix.settings.logins -import mozilla.components.lib.publicsuffixlist.PublicSuffixList -import org.mozilla.fenix.ext.urlToTrimmedHost - sealed class SortingStrategy { abstract operator fun invoke(logins: List): List - data class Alphabetically(private val publicSuffixList: PublicSuffixList) : SortingStrategy() { + object Alphabetically : SortingStrategy() { override fun invoke(logins: List): List { - return logins.sortedBy { it.origin.urlToTrimmedHost(publicSuffixList) } + return logins.sortedBy { it.origin } } } diff --git a/app/src/main/java/org/mozilla/fenix/utils/Settings.kt b/app/src/main/java/org/mozilla/fenix/utils/Settings.kt index 0c87a43558..e828cabc9a 100644 --- a/app/src/main/java/org/mozilla/fenix/utils/Settings.kt +++ b/app/src/main/java/org/mozilla/fenix/utils/Settings.kt @@ -1153,8 +1153,7 @@ class Settings(private val appContext: Context) : PreferencesHolder { var savedLoginsSortingStrategy: SortingStrategy get() { return when (savedLoginsMenuHighlightedItem) { - SavedLoginsSortingStrategyMenu.Item.AlphabeticallySort -> - SortingStrategy.Alphabetically(appContext.components.publicSuffixList) + SavedLoginsSortingStrategyMenu.Item.AlphabeticallySort -> SortingStrategy.Alphabetically SavedLoginsSortingStrategyMenu.Item.LastUsedSort -> SortingStrategy.LastUsed } } diff --git a/app/src/test/java/org/mozilla/fenix/ext/StringTest.kt b/app/src/test/java/org/mozilla/fenix/ext/StringTest.kt index 0450ee0309..751f23491f 100644 --- a/app/src/test/java/org/mozilla/fenix/ext/StringTest.kt +++ b/app/src/test/java/org/mozilla/fenix/ext/StringTest.kt @@ -22,13 +22,6 @@ class StringTest { private val publicSuffixList = PublicSuffixList(testContext) - @Test - fun `Url To Trimmed Host`() { - val urlTest = "http://www.example.com:1080/docs/resource1.html" - val new = urlTest.urlToTrimmedHost(publicSuffixList) - assertEquals(new, "example") - } - @Test fun `Simplified Url`() { val urlTest = "https://www.amazon.com" diff --git a/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsFragmentStoreTest.kt b/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsFragmentStoreTest.kt index c1a8ef40c9..cac0be6c27 100644 --- a/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsFragmentStoreTest.kt +++ b/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsFragmentStoreTest.kt @@ -117,7 +117,7 @@ class LoginsFragmentStoreTest { baseState.copy( isLoading = true, searchedForText = null, - sortingStrategy = SortingStrategy.Alphabetically(mockk()), + sortingStrategy = SortingStrategy.Alphabetically, highlightedItem = SavedLoginsSortingStrategyMenu.Item.AlphabeticallySort, loginList = loginList ) @@ -139,7 +139,7 @@ class LoginsFragmentStoreTest { baseState.copy( isLoading = true, searchedForText = "example", - sortingStrategy = SortingStrategy.Alphabetically(mockk()), + sortingStrategy = SortingStrategy.Alphabetically, highlightedItem = SavedLoginsSortingStrategyMenu.Item.AlphabeticallySort, loginList = loginList ) diff --git a/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsListControllerTest.kt b/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsListControllerTest.kt index f8e2f5eda6..2ca207d3cd 100644 --- a/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsListControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/settings/logins/LoginsListControllerTest.kt @@ -7,8 +7,6 @@ package org.mozilla.fenix.settings.logins import androidx.navigation.NavController import io.mockk.mockk import io.mockk.verifyAll -import mozilla.components.lib.publicsuffixlist.PublicSuffixList -import mozilla.components.support.test.robolectric.testContext import org.junit.Test import org.junit.runner.RunWith import org.mozilla.fenix.BrowserDirection @@ -24,8 +22,7 @@ import org.mozilla.fenix.utils.Settings class LoginsListControllerTest { private val store: LoginsFragmentStore = mockk(relaxed = true) private val settings: Settings = mockk(relaxed = true) - private val publicSuffixList = PublicSuffixList(testContext) - private val sortingStrategy: SortingStrategy = SortingStrategy.Alphabetically(publicSuffixList) + private val sortingStrategy: SortingStrategy = SortingStrategy.Alphabetically private val navController: NavController = mockk(relaxed = true) private val browserNavigator: (String, Boolean, BrowserDirection) -> Unit = mockk(relaxed = true) private val metrics: MetricController = mockk(relaxed = true) @@ -43,11 +40,7 @@ class LoginsListControllerTest { controller.handleSort(sortingStrategy) verifyAll { - store.dispatch( - LoginsAction.SortLogins( - SortingStrategy.Alphabetically(publicSuffixList) - ) - ) + store.dispatch(LoginsAction.SortLogins(SortingStrategy.Alphabetically)) settings.savedLoginsSortingStrategy = sortingStrategy } } diff --git a/app/src/test/java/org/mozilla/fenix/settings/logins/SavedLoginsInteractorTest.kt b/app/src/test/java/org/mozilla/fenix/settings/logins/SavedLoginsInteractorTest.kt index 06f8ba2e55..347663f94a 100644 --- a/app/src/test/java/org/mozilla/fenix/settings/logins/SavedLoginsInteractorTest.kt +++ b/app/src/test/java/org/mozilla/fenix/settings/logins/SavedLoginsInteractorTest.kt @@ -43,12 +43,10 @@ class SavedLoginsInteractorTest { @Test fun `GIVEN a change in sorting strategy, WHEN the interactor is called for it, THEN it should just delegate the controller`() { every { testContext.components.publicSuffixList } returns PublicSuffixList(testContext) - val sortingStrategy = SortingStrategy.Alphabetically(testContext.components.publicSuffixList) - - interactor.onSortingStrategyChanged(sortingStrategy) + interactor.onSortingStrategyChanged(SortingStrategy.Alphabetically) verifyAll { - listController.handleSort(sortingStrategy) + listController.handleSort(SortingStrategy.Alphabetically) } } diff --git a/app/src/test/java/org/mozilla/fenix/settings/logins/SavedLoginsSortingStrategyMenuTest.kt b/app/src/test/java/org/mozilla/fenix/settings/logins/SavedLoginsSortingStrategyMenuTest.kt index 3bf89f5330..b850e307ad 100644 --- a/app/src/test/java/org/mozilla/fenix/settings/logins/SavedLoginsSortingStrategyMenuTest.kt +++ b/app/src/test/java/org/mozilla/fenix/settings/logins/SavedLoginsSortingStrategyMenuTest.kt @@ -12,7 +12,6 @@ import io.mockk.just import io.mockk.mockk import io.mockk.verify import mozilla.components.concept.menu.candidate.HighPriorityHighlightEffect -import mozilla.components.lib.publicsuffixlist.PublicSuffixList import mozilla.components.support.ktx.android.content.getColorFromAttr import mozilla.components.support.test.robolectric.testContext import org.junit.Assert.assertEquals @@ -21,7 +20,6 @@ import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.mozilla.fenix.R -import org.mozilla.fenix.ext.components import org.mozilla.fenix.helpers.FenixRobolectricTestRunner import org.mozilla.fenix.settings.logins.SavedLoginsSortingStrategyMenu.Item import org.mozilla.fenix.settings.logins.interactor.SavedLoginsInteractor @@ -69,16 +67,12 @@ class SavedLoginsSortingStrategyMenuTest { @Test fun `candidates call interactor on click`() { - val publicSuffixList = PublicSuffixList(testContext) - every { testContext.components.publicSuffixList } returns publicSuffixList val (name, lastUsed) = menu.menuItems(Item.AlphabeticallySort) every { interactor.onSortingStrategyChanged(any()) } just Runs name.onClick() verify { - interactor.onSortingStrategyChanged( - SortingStrategy.Alphabetically(publicSuffixList) - ) + interactor.onSortingStrategyChanged(SortingStrategy.Alphabetically) } lastUsed.onClick()