From 1b305c139804a4828f1c3cd920d43321a749353c Mon Sep 17 00:00:00 2001 From: Grigory Kruglov Date: Tue, 11 Jan 2022 16:01:32 -0800 Subject: [PATCH] For #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 #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 04bf08c3e..c9afbd8b3 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 8e5fbe21d..f34a7d00f 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 eae2ff2ce..2e684eee9 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 0c87a4355..e828cabc9 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 0450ee030..751f23491 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 c1a8ef40c..cac0be6c2 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 f8e2f5eda..2ca207d3c 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 06f8ba2e5..347663f94 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 3bf89f533..b850e307a 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()