ishan
/
fenix
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Close #24613: Remove history improvement feature flag

This commit is contained in:
Roger Yang 2022-10-25 16:50:37 -04:00 committed by mergify[bot]
parent e8ef439469
commit 35e0afe329
8 changed files with 23 additions and 59 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
app/src/main/java/org/mozilla/fenix/FeatureFlags.kt
View File

@ -92,11 +92,6 @@ object FeatureFlags {
*/
const val showHomeOnboarding = true
/**
* Enables history improvement features.
*/
const val historyImprovementFeatures = true
/**
* Enables the Task Continuity enhancements.
*/

19
app/src/main/java/org/mozilla/fenix/components/history/PagedHistoryProvider.kt
View File

@ -11,7 +11,6 @@ import mozilla.components.concept.storage.HistoryMetadataKey
import mozilla.components.concept.storage.VisitInfo
import mozilla.components.concept.storage.VisitType
import mozilla.components.support.ktx.kotlin.tryGetHostFromUrl
import org.mozilla.fenix.FeatureFlags
import org.mozilla.fenix.library.history.History
import org.mozilla.fenix.library.history.HistoryItemTimeGroup
import org.mozilla.fenix.utils.Settings.Companion.SEARCH_GROUP_MINIMUM_SITES
@ -86,7 +85,6 @@ interface PagedHistoryProvider {
*/
class DefaultPagedHistoryProvider(
private val historyStorage: PlacesHistoryStorage,
private val historyImprovementFeatures: Boolean = FeatureFlags.historyImprovementFeatures,
) : PagedHistoryProvider {
/**
@ -132,11 +130,7 @@ class DefaultPagedHistoryProvider(
)
}
.filter {
if (historyImprovementFeatures) {
it.items.size >= SEARCH_GROUP_MINIMUM_SITES
} else {
true
}
it.items.size >= SEARCH_GROUP_MINIMUM_SITES
}
.toList()
}
@ -207,10 +201,7 @@ class DefaultPagedHistoryProvider(
emptyList()
}
val historyMetadata = historyGroupsInOffset.flatMap { it.items }
if (historyImprovementFeatures) {
history = history.distinctBy { Pair(it.historyTimeGroup, it.url) }
}
history = history.distinctBy { Pair(it.historyTimeGroup, it.url) }
// Add all history items that are not in a group filtering out any matches with a history
// metadata item.
@ -227,11 +218,7 @@ class DefaultPagedHistoryProvider(
},
)
return if (historyImprovementFeatures) {
result.sortedByDescending { it.visitedAt }
} else {
result.removeConsecutiveDuplicates().sortedByDescending { it.visitedAt }
}
return result.sortedByDescending { it.visitedAt }
}
private fun transformVisitInfoToHistoryItem(visit: VisitInfo): HistoryDB.Regular {

8
app/src/main/java/org/mozilla/fenix/home/recentvisits/RecentVisitsFeature.kt
View File

@ -17,7 +17,6 @@ import mozilla.components.concept.storage.HistoryHighlightWeights
import mozilla.components.concept.storage.HistoryMetadata
import mozilla.components.concept.storage.HistoryMetadataStorage
import mozilla.components.support.base.feature.LifecycleAwareFeature
import org.mozilla.fenix.FeatureFlags
import org.mozilla.fenix.components.AppStore
import org.mozilla.fenix.components.appstate.AppAction
import org.mozilla.fenix.home.HomeFragment
@ -52,7 +51,6 @@ class RecentVisitsFeature(
private val historyHighlightsStorage: Lazy<PlacesHistoryStorage>,
private val scope: CoroutineScope,
private val ioDispatcher: CoroutineDispatcher = Dispatchers.IO,
private val historyImprovementFeatures: Boolean = FeatureFlags.historyImprovementFeatures,
) : LifecycleAwareFeature {
private var job: Job? = null
@ -214,11 +212,7 @@ class RecentVisitsFeature(
)
}
.filter {
if (historyImprovementFeatures) {
it.groupItems.size >= SEARCH_GROUP_MINIMUM_SITES
} else {
true
}
it.groupItems.size >= SEARCH_GROUP_MINIMUM_SITES
}
}

5
app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt
View File

@ -40,7 +40,6 @@ import mozilla.components.concept.storage.BookmarkNodeType
import mozilla.components.lib.state.ext.consumeFrom
import mozilla.components.support.base.feature.UserInteractionHandler
import mozilla.telemetry.glean.private.NoExtras
import org.mozilla.fenix.FeatureFlags
import org.mozilla.fenix.GleanMetrics.BookmarksManagement
import org.mozilla.fenix.HomeActivity
import org.mozilla.fenix.NavHostActivity
@ -183,10 +182,6 @@ class BookmarkFragment : LibraryPageFragment<BookmarkNode>(), UserInteractionHan
if (mode.showMenu) {
inflater.inflate(R.menu.bookmarks_menu, menu)
}
if (!FeatureFlags.historyImprovementFeatures) {
menu.findItem(R.id.bookmark_search)?.isVisible = false
}
}
is BookmarkFragmentState.Mode.Selecting -> {
if (mode.selectedItems.any { it.type != BookmarkNodeType.ITEM }) {

5
app/src/main/java/org/mozilla/fenix/library/history/HistoryFragment.kt
View File

@ -38,7 +38,6 @@ import mozilla.components.service.fxa.sync.SyncReason
import mozilla.components.support.base.feature.UserInteractionHandler
import mozilla.telemetry.glean.private.NoExtras
import org.mozilla.fenix.BrowserDirection
import org.mozilla.fenix.FeatureFlags
import org.mozilla.fenix.HomeActivity
import org.mozilla.fenix.NavHostActivity
import org.mozilla.fenix.R
@ -224,10 +223,6 @@ class HistoryFragment : LibraryPageFragment<History>(), UserInteractionHandler,
} else {
inflater.inflate(R.menu.history_menu, menu)
}
if (!FeatureFlags.historyImprovementFeatures) {
menu.findItem(R.id.history_search)?.isVisible = false
}
}
override fun onMenuItemSelected(item: MenuItem): Boolean = when (item.itemId) {

5
app/src/main/java/org/mozilla/fenix/utils/Settings.kt
View File

@ -31,7 +31,6 @@ import mozilla.components.support.locale.LocaleManager
import org.mozilla.fenix.BuildConfig
import org.mozilla.fenix.Config
import org.mozilla.fenix.FeatureFlags
import org.mozilla.fenix.FeatureFlags.historyImprovementFeatures
import org.mozilla.fenix.R
import org.mozilla.fenix.browser.browsingmode.BrowsingMode
import org.mozilla.fenix.components.metrics.MozillaProductDetector
@ -82,9 +81,9 @@ class Settings(private val appContext: Context) : PreferencesHolder {
/**
* The minimum number a search groups should contain.
* Filtering is applied depending on the [historyImprovementFeatures] flag value.
*/
const val SEARCH_GROUP_MINIMUM_SITES: Int = 2
@VisibleForTesting
internal var SEARCH_GROUP_MINIMUM_SITES: Int = 2
// The maximum number of top sites to display.
const val TOP_SITES_MAX_COUNT = 16

6
app/src/test/java/org/mozilla/fenix/components/history/PagedHistoryProviderTest.kt
View File

@ -17,6 +17,7 @@ import mozilla.components.concept.storage.VisitType
import org.junit.Assert.assertEquals
import org.junit.Before
import org.junit.Test
import org.mozilla.fenix.utils.Settings
class PagedHistoryProviderTest {
@ -25,13 +26,13 @@ class PagedHistoryProviderTest {
@Before
fun setup() {
storage = mockk()
Settings.SEARCH_GROUP_MINIMUM_SITES = 1
}
@Test
fun `getHistory uses getVisitsPaginated`() = runTest {
val provider = DefaultPagedHistoryProvider(
historyStorage = storage,
historyImprovementFeatures = false,
)
val visitInfo1 = VisitInfo(
@ -148,7 +149,6 @@ class PagedHistoryProviderTest {
fun `history metadata matching lower bound`() = runTest {
val provider = DefaultPagedHistoryProvider(
historyStorage = storage,
historyImprovementFeatures = false,
)
// Oldest history visit on the page is 15 seconds (buffer time) newer than matching
// metadata record.
@ -218,7 +218,6 @@ class PagedHistoryProviderTest {
fun `history metadata matching upper bound`() = runTest {
val provider = DefaultPagedHistoryProvider(
historyStorage = storage,
historyImprovementFeatures = false,
)
// Newest history visit on the page is 15 seconds (buffer time) older than matching
// metadata record.
@ -288,7 +287,6 @@ class PagedHistoryProviderTest {
fun `redirects are filtered out from history metadata groups`() = runTest {
val provider = DefaultPagedHistoryProvider(
historyStorage = storage,
historyImprovementFeatures = false,
)
val visitInfo1 = VisitInfo(

29
app/src/test/java/org/mozilla/fenix/home/recentvisits/RecentVisitsFeatureTest.kt
View File

@ -35,6 +35,7 @@ import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryGrou
import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem.RecentHistoryHighlight
import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItemInternal.HistoryGroupInternal
import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItemInternal.HistoryHighlightInternal
import org.mozilla.fenix.utils.Settings
import kotlin.random.Random
@OptIn(ExperimentalCoroutinesApi::class)
@ -55,6 +56,7 @@ class RecentVisitsFeatureTest {
fun setup() {
historyHightlightsStorage = mockk(relaxed = true)
historyMetadataStorage = mockk(relaxed = true)
Settings.SEARCH_GROUP_MINIMUM_SITES = 1
}
@Test
@ -391,7 +393,7 @@ class RecentVisitsFeatureTest {
@Test
fun `GIVEN a list of history highlights and groups WHEN updateState is called THEN emit RecentHistoryChange`() {
val feature = spyk(RecentVisitsFeature(appStore, mockk(), mockk(), mockk(), mockk(), false))
val feature = spyk(RecentVisitsFeature(appStore, mockk(), mockk(), mockk(), mockk()))
val expected = List<RecentHistoryHighlight>(1) { mockk() }
every { feature.getCombinedHistory(any(), any()) } returns expected
@ -405,7 +407,7 @@ class RecentVisitsFeatureTest {
@Test
fun `GIVEN highlights visits exist in search groups WHEN getCombined is called THEN remove the highlights already in groups`() {
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk(), false)
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk())
val visitsFromSearch = getSearchFromHistoryMetadataItems(4)
val directVisits = getDirectVisitsHistoryMetadataItems(4)
val directDupeVisits = getSearchFromHistoryMetadataItems(2).map {
@ -429,7 +431,7 @@ class RecentVisitsFeatureTest {
@Test
fun `GIVEN fewer than needed highlights and search groups WHEN getCombined is called THEN the result is sorted by date`() {
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk(), false)
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk())
val visitsFromSearch = getSearchFromHistoryMetadataItems(4)
val directVisits = getDirectVisitsHistoryMetadataItems(4)
val expected = directVisits.reversed().toRecentHistoryHighlights()
@ -448,7 +450,7 @@ class RecentVisitsFeatureTest {
@Test
fun `GIVEN more highlights are newer than search groups WHEN getCombined is called THEN then return an even split then sorted by date`() {
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk(), false)
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk())
val visitsFromSearch = getSearchFromHistoryMetadataItems(5)
val directVisits = getDirectVisitsHistoryMetadataItems(14)
val expected = directVisits.takeLast(5).reversed().toRecentHistoryHighlights() +
@ -464,7 +466,7 @@ class RecentVisitsFeatureTest {
@Test
fun `GIVEN more search groups are newer than highlights WHEN getCombined is called THEN then return an even split then sorted by date`() {
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk(), false)
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk())
val visitsFromSearch = getSearchFromHistoryMetadataItems(14)
val directVisits = getDirectVisitsHistoryMetadataItems(5)
val expected = visitsFromSearch.takeLast(4).toIndividualRecentHistoryGroups() +
@ -480,7 +482,7 @@ class RecentVisitsFeatureTest {
@Test
fun `GIVEN all highlights have metadata WHEN getHistoryHighlights is called THEN return a list of highlights with an inferred last access time`() {
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk(), false)
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk())
val visitsFromSearch = getSearchFromHistoryMetadataItems(10)
val directVisits = getDirectVisitsHistoryMetadataItems(10)
@ -497,7 +499,7 @@ class RecentVisitsFeatureTest {
@Test
fun `GIVEN not all highlights have metadata WHEN getHistoryHighlights is called THEN set 0 for the highlights with not found last access time`() {
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk(), false)
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk())
val visitsFromSearch = getSearchFromHistoryMetadataItems(10)
val directVisits = getDirectVisitsHistoryMetadataItems(10)
val highlightsWithUnknownAccessTime = directVisits.toHistoryHighlightsInternal().take(5).map {
@ -518,7 +520,7 @@ class RecentVisitsFeatureTest {
@Test
fun `GIVEN multiple metadata records for the same highlight WHEN getHistoryHighlights is called THEN set the latest access time from multiple available`() {
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk(), false)
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk())
val visitsFromSearch = getSearchFromHistoryMetadataItems(10)
val directVisits = getDirectVisitsHistoryMetadataItems(10)
val newerDirectVisits = directVisits.mapIndexed { index, item ->
@ -540,7 +542,7 @@ class RecentVisitsFeatureTest {
@Test
fun `GIVEN multiple metadata entries only for direct accessed pages WHEN getHistorySearchGroups is called THEN return an empty list`() {
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk(), false)
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk())
val directVisits = getDirectVisitsHistoryMetadataItems(10)
val result = feature.getHistorySearchGroups(directVisits)
@ -550,7 +552,7 @@ class RecentVisitsFeatureTest {
@Test
fun `GIVEN multiple metadata entries WHEN getHistorySearchGroups is called THEN group all entries by their search term`() {
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk(), false)
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk())
val visitsFromSearch = getSearchFromHistoryMetadataItems(10)
val directVisits = getDirectVisitsHistoryMetadataItems(10)
@ -563,7 +565,7 @@ class RecentVisitsFeatureTest {
@Test
fun `GIVEN multiple metadata entries for the same url WHEN getHistorySearchGroups is called THEN entries are deduped`() {
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk(), false)
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk())
val visitsFromSearch = getSearchFromHistoryMetadataItems(10)
val newerVisitsFromSearch = visitsFromSearch.map { it.copy(updatedAt = it.updatedAt * 2) }
val directVisits = getDirectVisitsHistoryMetadataItems(10)
@ -582,7 +584,7 @@ class RecentVisitsFeatureTest {
@Test
fun `GIVEN highlights and search groups WHEN getSortedHistory is called THEN sort descending all items based on the last access time`() {
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk(), false)
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk())
val visitsFromSearch = getSearchFromHistoryMetadataItems(10)
val directVisits = getDirectVisitsHistoryMetadataItems(10)
val expected = directVisits.reversed().toRecentHistoryHighlights()
@ -601,7 +603,7 @@ class RecentVisitsFeatureTest {
@Test
fun `GIVEN highlights don't have a valid title WHEN getSortedHistory is called THEN the url is set as title`() {
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk(), false)
val feature = RecentVisitsFeature(mockk(), mockk(), mockk(), mockk(), mockk())
val visitsFromSearch = getSearchFromHistoryMetadataItems(10)
val directVisits = getDirectVisitsHistoryMetadataItems(10).mapIndexed { index, item ->
when (index % 3) {
@ -653,7 +655,6 @@ class RecentVisitsFeatureTest {
lazy { historyHightlightsStorage },
scope,
testDispatcher,
false,
)
assertEquals(emptyList<RecentHistoryGroup>(), appStore.state.recentHistory)