2
0
mirror of https://github.com/fork-maintainers/iceraven-browser synced 2024-11-19 09:25:34 +00:00

[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.
This commit is contained in:
Grigory Kruglov 2022-01-11 16:01:32 -08:00 committed by Grisha Kruglov
parent 64a9715198
commit 364f558857
9 changed files with 11 additions and 48 deletions

View File

@ -16,9 +16,7 @@ import kotlinx.coroutines.withContext
import mozilla.components.concept.fetch.Client import mozilla.components.concept.fetch.Client
import mozilla.components.concept.fetch.Request import mozilla.components.concept.fetch.Request
import mozilla.components.lib.publicsuffixlist.PublicSuffixList import mozilla.components.lib.publicsuffixlist.PublicSuffixList
import mozilla.components.lib.publicsuffixlist.ext.urlToTrimmedHost
import mozilla.components.support.ktx.android.net.hostWithoutCommonPrefixes import mozilla.components.support.ktx.android.net.hostWithoutCommonPrefixes
import org.mozilla.fenix.perf.runBlockingIncrement
import java.io.IOException import java.io.IOException
import java.net.IDN import java.net.IDN
import java.util.Locale import java.util.Locale
@ -91,14 +89,6 @@ private fun Uri.isIpv6(): Boolean {
return host.isNotEmpty() && host.contains(":") 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. * Trims a URL string of its scheme and common prefixes.
* *

View File

@ -12,7 +12,6 @@ import mozilla.components.concept.menu.candidate.TextMenuCandidate
import mozilla.components.concept.menu.candidate.TextStyle import mozilla.components.concept.menu.candidate.TextStyle
import mozilla.components.support.ktx.android.content.getColorFromAttr import mozilla.components.support.ktx.android.content.getColorFromAttr
import org.mozilla.fenix.R import org.mozilla.fenix.R
import org.mozilla.fenix.ext.components
import org.mozilla.fenix.settings.logins.interactor.SavedLoginsInteractor import org.mozilla.fenix.settings.logins.interactor.SavedLoginsInteractor
class SavedLoginsSortingStrategyMenu( class SavedLoginsSortingStrategyMenu(
@ -51,7 +50,7 @@ class SavedLoginsSortingStrategyMenu(
effect = if (itemToHighlight == Item.AlphabeticallySort) highlight else null effect = if (itemToHighlight == Item.AlphabeticallySort) highlight else null
) { ) {
savedLoginsInteractor.onSortingStrategyChanged( savedLoginsInteractor.onSortingStrategyChanged(
SortingStrategy.Alphabetically(context.components.publicSuffixList) SortingStrategy.Alphabetically
) )
}, },
TextMenuCandidate( TextMenuCandidate(

View File

@ -4,15 +4,12 @@
package org.mozilla.fenix.settings.logins package org.mozilla.fenix.settings.logins
import mozilla.components.lib.publicsuffixlist.PublicSuffixList
import org.mozilla.fenix.ext.urlToTrimmedHost
sealed class SortingStrategy { sealed class SortingStrategy {
abstract operator fun invoke(logins: List<SavedLogin>): List<SavedLogin> abstract operator fun invoke(logins: List<SavedLogin>): List<SavedLogin>
data class Alphabetically(private val publicSuffixList: PublicSuffixList) : SortingStrategy() { object Alphabetically : SortingStrategy() {
override fun invoke(logins: List<SavedLogin>): List<SavedLogin> { override fun invoke(logins: List<SavedLogin>): List<SavedLogin> {
return logins.sortedBy { it.origin.urlToTrimmedHost(publicSuffixList) } return logins.sortedBy { it.origin }
} }
} }

View File

@ -1153,8 +1153,7 @@ class Settings(private val appContext: Context) : PreferencesHolder {
var savedLoginsSortingStrategy: SortingStrategy var savedLoginsSortingStrategy: SortingStrategy
get() { get() {
return when (savedLoginsMenuHighlightedItem) { return when (savedLoginsMenuHighlightedItem) {
SavedLoginsSortingStrategyMenu.Item.AlphabeticallySort -> SavedLoginsSortingStrategyMenu.Item.AlphabeticallySort -> SortingStrategy.Alphabetically
SortingStrategy.Alphabetically(appContext.components.publicSuffixList)
SavedLoginsSortingStrategyMenu.Item.LastUsedSort -> SortingStrategy.LastUsed SavedLoginsSortingStrategyMenu.Item.LastUsedSort -> SortingStrategy.LastUsed
} }
} }

View File

@ -22,13 +22,6 @@ class StringTest {
private val publicSuffixList = PublicSuffixList(testContext) 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 @Test
fun `Simplified Url`() { fun `Simplified Url`() {
val urlTest = "https://www.amazon.com" val urlTest = "https://www.amazon.com"

View File

@ -117,7 +117,7 @@ class LoginsFragmentStoreTest {
baseState.copy( baseState.copy(
isLoading = true, isLoading = true,
searchedForText = null, searchedForText = null,
sortingStrategy = SortingStrategy.Alphabetically(mockk()), sortingStrategy = SortingStrategy.Alphabetically,
highlightedItem = SavedLoginsSortingStrategyMenu.Item.AlphabeticallySort, highlightedItem = SavedLoginsSortingStrategyMenu.Item.AlphabeticallySort,
loginList = loginList loginList = loginList
) )
@ -139,7 +139,7 @@ class LoginsFragmentStoreTest {
baseState.copy( baseState.copy(
isLoading = true, isLoading = true,
searchedForText = "example", searchedForText = "example",
sortingStrategy = SortingStrategy.Alphabetically(mockk()), sortingStrategy = SortingStrategy.Alphabetically,
highlightedItem = SavedLoginsSortingStrategyMenu.Item.AlphabeticallySort, highlightedItem = SavedLoginsSortingStrategyMenu.Item.AlphabeticallySort,
loginList = loginList loginList = loginList
) )

View File

@ -7,8 +7,6 @@ package org.mozilla.fenix.settings.logins
import androidx.navigation.NavController import androidx.navigation.NavController
import io.mockk.mockk import io.mockk.mockk
import io.mockk.verifyAll import io.mockk.verifyAll
import mozilla.components.lib.publicsuffixlist.PublicSuffixList
import mozilla.components.support.test.robolectric.testContext
import org.junit.Test import org.junit.Test
import org.junit.runner.RunWith import org.junit.runner.RunWith
import org.mozilla.fenix.BrowserDirection import org.mozilla.fenix.BrowserDirection
@ -24,8 +22,7 @@ import org.mozilla.fenix.utils.Settings
class LoginsListControllerTest { class LoginsListControllerTest {
private val store: LoginsFragmentStore = mockk(relaxed = true) private val store: LoginsFragmentStore = mockk(relaxed = true)
private val settings: Settings = mockk(relaxed = true) private val settings: Settings = mockk(relaxed = true)
private val publicSuffixList = PublicSuffixList(testContext) private val sortingStrategy: SortingStrategy = SortingStrategy.Alphabetically
private val sortingStrategy: SortingStrategy = SortingStrategy.Alphabetically(publicSuffixList)
private val navController: NavController = mockk(relaxed = true) private val navController: NavController = mockk(relaxed = true)
private val browserNavigator: (String, Boolean, BrowserDirection) -> Unit = mockk(relaxed = true) private val browserNavigator: (String, Boolean, BrowserDirection) -> Unit = mockk(relaxed = true)
private val metrics: MetricController = mockk(relaxed = true) private val metrics: MetricController = mockk(relaxed = true)
@ -43,11 +40,7 @@ class LoginsListControllerTest {
controller.handleSort(sortingStrategy) controller.handleSort(sortingStrategy)
verifyAll { verifyAll {
store.dispatch( store.dispatch(LoginsAction.SortLogins(SortingStrategy.Alphabetically))
LoginsAction.SortLogins(
SortingStrategy.Alphabetically(publicSuffixList)
)
)
settings.savedLoginsSortingStrategy = sortingStrategy settings.savedLoginsSortingStrategy = sortingStrategy
} }
} }

View File

@ -43,12 +43,10 @@ class SavedLoginsInteractorTest {
@Test @Test
fun `GIVEN a change in sorting strategy, WHEN the interactor is called for it, THEN it should just delegate the controller`() { 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) every { testContext.components.publicSuffixList } returns PublicSuffixList(testContext)
val sortingStrategy = SortingStrategy.Alphabetically(testContext.components.publicSuffixList) interactor.onSortingStrategyChanged(SortingStrategy.Alphabetically)
interactor.onSortingStrategyChanged(sortingStrategy)
verifyAll { verifyAll {
listController.handleSort(sortingStrategy) listController.handleSort(SortingStrategy.Alphabetically)
} }
} }

View File

@ -12,7 +12,6 @@ import io.mockk.just
import io.mockk.mockk import io.mockk.mockk
import io.mockk.verify import io.mockk.verify
import mozilla.components.concept.menu.candidate.HighPriorityHighlightEffect 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.ktx.android.content.getColorFromAttr
import mozilla.components.support.test.robolectric.testContext import mozilla.components.support.test.robolectric.testContext
import org.junit.Assert.assertEquals import org.junit.Assert.assertEquals
@ -21,7 +20,6 @@ import org.junit.Before
import org.junit.Test import org.junit.Test
import org.junit.runner.RunWith import org.junit.runner.RunWith
import org.mozilla.fenix.R import org.mozilla.fenix.R
import org.mozilla.fenix.ext.components
import org.mozilla.fenix.helpers.FenixRobolectricTestRunner import org.mozilla.fenix.helpers.FenixRobolectricTestRunner
import org.mozilla.fenix.settings.logins.SavedLoginsSortingStrategyMenu.Item import org.mozilla.fenix.settings.logins.SavedLoginsSortingStrategyMenu.Item
import org.mozilla.fenix.settings.logins.interactor.SavedLoginsInteractor import org.mozilla.fenix.settings.logins.interactor.SavedLoginsInteractor
@ -69,16 +67,12 @@ class SavedLoginsSortingStrategyMenuTest {
@Test @Test
fun `candidates call interactor on click`() { fun `candidates call interactor on click`() {
val publicSuffixList = PublicSuffixList(testContext)
every { testContext.components.publicSuffixList } returns publicSuffixList
val (name, lastUsed) = menu.menuItems(Item.AlphabeticallySort) val (name, lastUsed) = menu.menuItems(Item.AlphabeticallySort)
every { interactor.onSortingStrategyChanged(any()) } just Runs every { interactor.onSortingStrategyChanged(any()) } just Runs
name.onClick() name.onClick()
verify { verify {
interactor.onSortingStrategyChanged( interactor.onSortingStrategyChanged(SortingStrategy.Alphabetically)
SortingStrategy.Alphabetically(publicSuffixList)
)
} }
lastUsed.onClick() lastUsed.onClick()