Archives
/
iceraven-browser
mirror of https://github.com/fork-maintainers/iceraven-browser
2
0
Fork 0
Code Issues Releases Wiki Activity

[fenix] Make search term grouping tolerant to (parent tab) navigation

Browse Source 
Co-authored-by: Grisha Kruglov <gkruglov@mozilla.com>
pull/600/head
Christian Sadilek 3 years ago committed by mergify[bot]
parent d70d0610d0
commit c1f2c33a76
3 changed files with 198 additions and 41 deletions
Show Stats Download Patch File Download Diff File

13
app/src/main/java/org/mozilla/fenix/components/history/PagedHistoryProvider.kt
Unescape Escape View File

@ -12,6 +12,7 @@ import org.mozilla.fenix.FeatureFlags
import org.mozilla.fenix.library.history.History
import org.mozilla.fenix.library.history.toHistoryMetadata
import org.mozilla.fenix.perf.runBlockingIncrement
import kotlin.math.abs
private const val BUFFER_TIME = 15000 /* 15 seconds in ms */
@ -100,7 +101,7 @@ class DefaultPagedHistoryProvider(
suspend fun getMatchingHistory(historyMetadata: History.Metadata): VisitInfo? {
val history = historyStorage.getDetailedVisits(
start = historyMetadata.visitedAt - BUFFER_TIME,
end = historyMetadata.visitedAt,
end = historyMetadata.visitedAt + BUFFER_TIME,
excludeTypes = listOf(
VisitType.NOT_A_VISIT,
VisitType.DOWNLOAD,
@ -111,7 +112,9 @@ class DefaultPagedHistoryProvider(
VisitType.REDIRECT_PERMANENT
)
)
return history.lastOrNull { it.url == historyMetadata.url }
return history
.filter { it.url == historyMetadata.url }
.minByOrNull { abs(historyMetadata.visitedAt - it.visitTime) }
}
/**
@ -152,8 +155,8 @@ class DefaultPagedHistoryProvider(
// items.
val historyGroupsInOffset = if (history.isNotEmpty()) {
historyGroups?.filter {
history.last().visitedAt <= it.visitedAt &&
it.visitedAt <= (history.first().visitedAt + visitedAtBuffer)
history.last().visitedAt <= it.visitedAt - visitedAtBuffer &&
it.visitedAt - visitedAtBuffer <= (history.first().visitedAt + visitedAtBuffer)
} ?: emptyList()
} else {
emptyList()
@ -171,7 +174,7 @@ class DefaultPagedHistoryProvider(
// url, but we don't have a use case for this currently in the history view.
result.addAll(
historyGroupsInOffset.map { group ->
group.copy(items = group.items.filter { it.totalViewTime > 0 }.distinctBy { it.url })
group.copy(items = group.items.distinctBy { it.url })
}
)

71
app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddleware.kt
Unescape Escape View File

@ -102,19 +102,17 @@ class HistoryMetadataMiddleware(
// changes introduced by the action. These handlers rely on up-to-date tab state, which
// is why they're in the "post" section.
when (action) {
is TabListAction.AddTabAction -> {
if (!action.tab.content.private) {
createHistoryMetadataIfNeeded(context, action.tab)
}
}
// NB: sometimes this fires multiple times after the page finished loading.
is ContentAction.UpdateHistoryStateAction -> {
context.state.findNormalTab(action.sessionId)?.let { tab ->
// When history state is ready, we can record metadata for this page.
val knownHistoryMetadata = tab.historyMetadata
val metadataPresentForUrl = knownHistoryMetadata != null &&
knownHistoryMetadata.url == tab.content.url
// Record metadata for tab if there is no metadata present, or if url of the
// tab changes since we last recorded metadata.
if (!metadataPresentForUrl) {
createHistoryMetadata(context, tab)
}
createHistoryMetadataIfNeeded(context, tab)
}
// Once we get a history update let's reset the flag for future loads.
directLoadTriggered = false
}
@ -127,13 +125,41 @@ class HistoryMetadataMiddleware(
}
}
private fun createHistoryMetadataIfNeeded(
context: MiddlewareContext<BrowserState, BrowserAction>,
tab: TabSessionState
) {
// When history state is ready, we can record metadata for this page.
val knownHistoryMetadata = tab.historyMetadata
val metadataPresentForUrl = knownHistoryMetadata != null &&
knownHistoryMetadata.url == tab.content.url
// Record metadata for tab if there is no metadata present, or if url of the
// tab changes since we last recorded metadata.
if (!metadataPresentForUrl) {
createHistoryMetadata(context, tab)
}
}
private fun createHistoryMetadata(context: MiddlewareContext<BrowserState, BrowserAction>, tab: TabSessionState) {
val tabParent = tab.getParent(context.store)
val previousUrlIndex = tab.content.history.currentIndex - 1
val tabMetadataHasSearchTerms = !tab.historyMetadata?.searchTerm.isNullOrBlank()
// Obtain search terms and referrer url either from tab parent, or from the history stack.
// Obtain search terms and referrer url either from tab parent, from the history stack, or
// from the tab itself.
// At a high level, there are two main cases here - 1) either the tab was opened as a 'new tab'
// via the search results page, or 2) a page was opened in the same tab as the search results page.
// Details about the New Tab case:
// - we obtain search terms via tab's parent (the search results page)
// - however, it's possible that parent changed (e.g. user navigated away from the search
// results page).
// - our approach below is to capture search terms from the parent within the tab.historyMetadata
// state on the first load of the tab, and then rely on this data for subsequent page loads on that tab.
// - this way, once a tab becomes part of the search group, it won't leave this search group
// unless a direct navigation event happens.
val (searchTerm, referrerUrl) = when {
tabParent != null -> {
// Loading page opened in a New Tab for the first time.
tabParent != null && !tabMetadataHasSearchTerms -> {
val searchTerms = tabParent.content.searchTerms.takeUnless { it.isEmpty() }
?: context.state.search.parseSearchTerms(tabParent.content.url)
searchTerms to tabParent.content.url
@ -142,14 +168,31 @@ class HistoryMetadataMiddleware(
// web content i.e., they followed a link, not if the user navigated directly via
// toolbar.
!directLoadTriggered && previousUrlIndex >= 0 -> {
val previousUrl = tab.content.history.items[previousUrlIndex].uri
val searchTerms = context.state.search.parseSearchTerms(previousUrl)
// Once a tab is within the search group, only direct navigation event can change that.
val (searchTerms, referrerUrl) = if (tabMetadataHasSearchTerms) {
tab.historyMetadata?.searchTerm to tab.historyMetadata?.referrerUrl
} else {
val previousUrl = tab.content.history.items[previousUrlIndex].uri
context.state.search.parseSearchTerms(previousUrl) to previousUrl
}
if (searchTerms != null) {
searchTerms to previousUrl
searchTerms to referrerUrl
} else {
null to null
}
}
// In certain redirect cases, we won't have a previous url in the history stack of the tab,
// but will have the search terms already set on the tab from having gone through this logic
// for the redirecting url.
// In that case, we leave this tab within the search group it's already in.
tabMetadataHasSearchTerms -> {
tab.historyMetadata?.searchTerm to tab.historyMetadata?.referrerUrl
}
// We had no search terms, no history stack, and no parent.
// For example, this would be a search results page itself.
// For now, the original search results page is not part of the search group.
// See https://github.com/mozilla-mobile/fenix/issues/21659.
else -> null to null
}

155
app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddlewareTest.kt
Unescape Escape View File

@ -27,6 +27,7 @@ import mozilla.components.concept.storage.HistoryMetadataKey
import mozilla.components.support.test.ext.joinBlocking
import mozilla.components.support.test.mock
import org.junit.Assert.assertEquals
import org.junit.Assert.assertNull
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
@ -58,7 +59,7 @@ class HistoryMetadataMiddlewareTest {
every { service.createMetadata(any(), any()) } returns expectedKey
store.dispatch(TabListAction.AddTabAction(tab)).joinBlocking()
verify { service wasNot Called }
verify(exactly = 1) { service.createMetadata(tab) }
store.dispatch(ContentAction.UpdateHistoryStateAction(tab.id, emptyList(), currentIndex = 0)).joinBlocking()
val capturedTab = slot<TabSessionState>()
@ -88,7 +89,7 @@ class HistoryMetadataMiddlewareTest {
}
@Test
fun `GIVEN normal tab has parent WHEN history metadata is recorded THEN search terms and referrer url are provided`() {
fun `GIVEN normal tab has parent with session search terms WHEN history metadata is recorded THEN search terms and referrer url are provided`() {
val parentTab = createTab("https://google.com?q=mozilla+website", searchTerms = "mozilla website")
val tab = createTab("https://mozilla.org", parent = parentTab)
store.dispatch(TabListAction.AddTabAction(parentTab)).joinBlocking()
@ -101,7 +102,7 @@ class HistoryMetadataMiddlewareTest {
}
@Test
fun `GIVEN normal tab has search results parent without search terms WHEN history metadata is recorded THEN search terms and referrer url are provided`() {
fun `GIVEN normal tab has search results parent without session search terms WHEN history metadata is recorded THEN search terms and referrer url are provided`() {
setupGoogleSearchEngine()
val parentTab = createTab("https://google.com?q=mozilla+website")
@ -115,6 +116,76 @@ class HistoryMetadataMiddlewareTest {
}
}
@Test
fun `GIVEN tab opened as new tab from a search page WHEN search page navigates away THEN redirecting or navigating in tab retains original search terms`() {
service = TestingMetadataService()
middleware = HistoryMetadataMiddleware(service)
store = BrowserStore(
middleware = listOf(middleware) + EngineMiddleware.create(engine = mockk()),
initialState = BrowserState()
)
setupGoogleSearchEngine()
val parentTab = createTab("https://google.com?q=mozilla+website", searchTerms = "mozilla website")
val tab = createTab("https://google.com?url=https://mozilla.org", parent = parentTab)
store.dispatch(TabListAction.AddTabAction(parentTab, select = true)).joinBlocking()
store.dispatch(TabListAction.AddTabAction(tab)).joinBlocking()
with((service as TestingMetadataService).createdMetadata) {
assertEquals(2, this.count())
assertEquals("https://google.com?q=mozilla+website", this[0].url)
assertNull(this[0].searchTerm)
assertNull(this[0].referrerUrl)
assertEquals("https://google.com?url=https://mozilla.org", this[1].url)
assertEquals("mozilla website", this[1].searchTerm)
assertEquals("https://google.com?q=mozilla+website", this[1].referrerUrl)
}
// Both tabs load.
store.dispatch(ContentAction.UpdateHistoryStateAction(parentTab.id, listOf(HistoryItem("Google - mozilla website", "https://google.com?q=mozilla+website")), 0)).joinBlocking()
store.dispatch(ContentAction.UpdateHistoryStateAction(tab.id, listOf(HistoryItem("", "https://google.com?url=mozilla+website")), currentIndex = 0)).joinBlocking()
with((service as TestingMetadataService).createdMetadata) {
assertEquals(2, this.count())
}
// Parent navigates away. Search terms are reset.
store.dispatch(ContentAction.UpdateUrlAction(parentTab.id, "https://firefox.com")).joinBlocking()
store.dispatch(ContentAction.UpdateSearchTermsAction(parentTab.id, "")).joinBlocking()
store.dispatch(ContentAction.UpdateHistoryStateAction(parentTab.id, listOf(HistoryItem("Google - mozilla website", "https://google.com?q=mozilla+website"), HistoryItem("Firefox", "https://firefox.com")), 1)).joinBlocking()
with((service as TestingMetadataService).createdMetadata) {
assertEquals(3, this.count())
assertEquals("https://firefox.com", this[2].url)
assertEquals("mozilla website", this[2].searchTerm)
assertEquals("https://google.com?q=mozilla+website", this[2].referrerUrl)
}
// Redirect the child tab (url changed, history stack has single item).
store.dispatch(ContentAction.UpdateUrlAction(tab.id, "https://mozilla.org")).joinBlocking()
store.dispatch(ContentAction.UpdateHistoryStateAction(tab.id, listOf(HistoryItem("Mozilla", "https://mozilla.org")), currentIndex = 0)).joinBlocking()
val tab2 = store.state.findTab(tab.id)!!
assertEquals("https://mozilla.org", tab2.content.url)
with((service as TestingMetadataService).createdMetadata) {
assertEquals(4, this.count())
assertEquals("https://mozilla.org", this[3].url)
assertEquals("mozilla website", this[3].searchTerm)
assertEquals("https://google.com?q=mozilla+website", this[3].referrerUrl)
}
// Navigate the child tab.
store.dispatch(ContentAction.UpdateUrlAction(tab.id, "https://mozilla.org/manifesto")).joinBlocking()
store.dispatch(ContentAction.UpdateHistoryStateAction(tab.id, listOf(HistoryItem("Mozilla", "https://mozilla.org"), HistoryItem("Mozilla Manifesto", "https://mozilla.org/manifesto")), currentIndex = 1)).joinBlocking()
val tab3 = store.state.findTab(tab.id)!!
assertEquals("https://mozilla.org/manifesto", tab3.content.url)
with((service as TestingMetadataService).createdMetadata) {
assertEquals(5, this.count())
assertEquals("https://mozilla.org/manifesto", this[4].url)
assertEquals("mozilla website", this[4].searchTerm)
assertEquals("https://google.com?q=mozilla+website", this[4].referrerUrl)
}
}
@Test
fun `GIVEN normal tab has parent WHEN url is the same THEN nothing happens`() {
val parentTab = createTab("https://mozilla.org", searchTerms = "mozilla website")
@ -122,8 +193,11 @@ class HistoryMetadataMiddlewareTest {
store.dispatch(TabListAction.AddTabAction(parentTab)).joinBlocking()
store.dispatch(TabListAction.AddTabAction(tab)).joinBlocking()
verify(exactly = 1) { service.createMetadata(parentTab, null, null) }
// Without our referrer url check, we would have recorded this metadata.
verify(exactly = 0) { service.createMetadata(tab, "mozilla website", "https://mozilla.org") }
store.dispatch(ContentAction.UpdateHistoryStateAction(tab.id, emptyList(), currentIndex = 0)).joinBlocking()
verify { service wasNot Called }
verify(exactly = 1) { service.createMetadata(any(), any(), any()) }
}
@Test
@ -226,14 +300,15 @@ class HistoryMetadataMiddlewareTest {
}
@Test
fun `GIVEN tab without meta data WHEN user navigates and new page starts loading THEN nothing happens`() {
fun `GIVEN tab without metadata WHEN user navigates and new page starts loading THEN nothing happens`() {
val tab = createTab(url = "https://mozilla.org")
store.dispatch(TabListAction.AddTabAction(tab)).joinBlocking()
verify { service wasNot Called }
verify(exactly = 1) { service.createMetadata(tab) }
store.dispatch(ContentAction.UpdateLoadingStateAction(tab.id, true)).joinBlocking()
verify { service wasNot Called }
verify(exactly = 1) { service.createMetadata(any()) }
verify(exactly = 0) { service.updateMetadata(any(), any()) }
}
@Test
@ -260,13 +335,22 @@ class HistoryMetadataMiddlewareTest {
every { service.createMetadata(any(), any()) } returns expectedKey
store.dispatch(TabListAction.AddTabAction(tab)).joinBlocking()
verify { service wasNot Called }
val capturedTabs = mutableListOf<TabSessionState>()
verify {
service.createMetadata(capture(capturedTabs), null, null)
}
assertEquals(tab.id, capturedTabs[0].id)
assertNull(capturedTabs[0].historyMetadata)
assertEquals(expectedKey, store.state.findTab(tab.id)?.historyMetadata)
store.dispatch(MediaSessionAction.UpdateMediaMetadataAction(tab.id, mockk())).joinBlocking()
val capturedTab = slot<TabSessionState>()
verify { service.createMetadata(capture(capturedTab)) }
verify {
service.createMetadata(capture(capturedTabs), null, null)
}
assertEquals(tab.id, capturedTab.captured.id)
// Ugh, why are there three captured tabs when only two invocations of createMetadata happened?
assertEquals(tab.id, capturedTabs[2].id)
assertEquals(expectedKey, capturedTabs[2].historyMetadata)
assertEquals(expectedKey, store.state.findTab(tab.id)?.historyMetadata)
}
@ -293,27 +377,30 @@ class HistoryMetadataMiddlewareTest {
store.dispatch(TabListAction.AddTabAction(tab)).joinBlocking()
store.dispatch(TabListAction.AddTabAction(otherTab)).joinBlocking()
verify { service wasNot Called }
verify(exactly = 1) { service.createMetadata(any()) }
verify(exactly = 0) { service.updateMetadata(any(), any()) }
store.dispatch(TabListAction.AddTabAction(yetAnotherTab, select = true)).joinBlocking()
val capturedTab = slot<TabSessionState>()
verify(exactly = 2) { service.createMetadata(any()) }
verify(exactly = 1) { service.updateMetadata(existingKey, capture(capturedTab)) }
assertEquals(tab.id, capturedTab.captured.id)
}
@Test
fun `GIVEN private tab is selected WHEN new tab will be added and selected THEN nothing happens`() {
fun `GIVEN private tab is selected WHEN new tab will be added and selected THEN metadata not updated for private tab`() {
val existingKey = HistoryMetadataKey(url = "https://mozilla.org")
val tab = createTab(url = "https://mozilla.org", historyMetadata = existingKey, private = true)
val otherTab = createTab(url = "https://blog.mozilla.org")
val yetAnotherTab = createTab(url = "https://media.mozilla.org")
store.dispatch(TabListAction.AddTabAction(tab)).joinBlocking()
store.dispatch(TabListAction.AddTabAction(otherTab)).joinBlocking()
verify { service wasNot Called }
store.dispatch(TabListAction.AddTabAction(otherTab)).joinBlocking()
verify { service.createMetadata(otherTab) }
store.dispatch(TabListAction.AddTabAction(yetAnotherTab, select = true)).joinBlocking()
verify { service wasNot Called }
verify { service.createMetadata(yetAnotherTab) }
}
@Test
@ -378,10 +465,11 @@ class HistoryMetadataMiddlewareTest {
store.dispatch(TabListAction.AddTabAction(tab)).joinBlocking()
store.dispatch(TabListAction.AddTabAction(otherTab)).joinBlocking()
verify { service wasNot Called }
// 1 because 'tab' already has a metadata key set.
verify(exactly = 1) { service.createMetadata(any()) }
store.dispatch(TabListAction.RemoveTabAction(otherTab.id)).joinBlocking()
verify { service wasNot Called }
verify(exactly = 1) { service.createMetadata(any()) }
}
@Test
@ -394,7 +482,8 @@ class HistoryMetadataMiddlewareTest {
store.dispatch(TabListAction.AddTabAction(tab)).joinBlocking()
store.dispatch(TabListAction.AddTabAction(otherTab)).joinBlocking()
store.dispatch(TabListAction.AddTabAction(yetAnotherTab)).joinBlocking()
verify { service wasNot Called }
// 'tab' already has an existing key, so metadata isn't created for it.
verify(exactly = 2) { service.createMetadata(any()) }
store.dispatch(TabListAction.RemoveTabsAction(listOf(tab.id, otherTab.id))).joinBlocking()
val capturedTab = slot<TabSessionState>()
@ -410,12 +499,14 @@ class HistoryMetadataMiddlewareTest {
val yetAnotherTab = createTab(url = "https://media.mozilla.org")
store.dispatch(TabListAction.AddTabAction(tab)).joinBlocking()
verify { service wasNot Called }
store.dispatch(TabListAction.AddTabAction(otherTab)).joinBlocking()
store.dispatch(TabListAction.AddTabAction(yetAnotherTab)).joinBlocking()
verify { service wasNot Called }
verify(exactly = 2) { service.createMetadata(any()) }
store.dispatch(TabListAction.RemoveTabsAction(listOf(tab.id, otherTab.id))).joinBlocking()
verify { service wasNot Called }
verify(exactly = 2) { service.createMetadata(any()) }
verify(exactly = 0) { service.updateMetadata(any(), any()) }
}
@Test
@ -428,10 +519,12 @@ class HistoryMetadataMiddlewareTest {
store.dispatch(TabListAction.AddTabAction(tab)).joinBlocking()
store.dispatch(TabListAction.AddTabAction(otherTab)).joinBlocking()
store.dispatch(TabListAction.AddTabAction(yetAnotherTab)).joinBlocking()
verify { service wasNot Called }
verify(exactly = 2) { service.createMetadata(any()) }
verify(exactly = 0) { service.updateMetadata(any(), any()) }
store.dispatch(TabListAction.RemoveTabsAction(listOf(otherTab.id, yetAnotherTab.id))).joinBlocking()
verify { service wasNot Called }
verify(exactly = 2) { service.createMetadata(any()) }
verify(exactly = 0) { service.updateMetadata(any(), any()) }
}
private fun setupGoogleSearchEngine() {
@ -457,4 +550,22 @@ class HistoryMetadataMiddlewareTest {
)
).joinBlocking()
}
// Provides a more convenient way of capturing arguments for the functions we care about.
// I.e. capturing arguments in mockk was driving me mad and this is easy to understand and works.
class TestingMetadataService : HistoryMetadataService {
val createdMetadata = mutableListOf<HistoryMetadataKey>()
override fun createMetadata(
tab: TabSessionState,
searchTerms: String?,
referrerUrl: String?
): HistoryMetadataKey {
createdMetadata.add(HistoryMetadataKey(tab.content.url, searchTerms, referrerUrl))
return HistoryMetadataKey(tab.content.url, searchTerms, referrerUrl)
}
override fun updateMetadata(key: HistoryMetadataKey, tab: TabSessionState) {}
override fun cleanup(olderThan: Long) {}
}
}

Write Preview
Loading…
Cancel
Save