From 2198298df50df7dbe87256b023cc4805454fe233 Mon Sep 17 00:00:00 2001 From: Grisha Kruglov Date: Wed, 16 Jun 2021 01:42:41 -0700 Subject: [PATCH] Obtain searchTerms from previous page in tab's history Co-authored-by: Christian Sadilek --- .../HistoryMetadataMiddleware.kt | 47 ++++++++--- .../historymetadata/HistoryMetadataService.kt | 19 +++-- .../HistoryMetadataMiddlewareTest.kt | 79 +++++++++++++++++-- .../HistoryMetadataServiceTest.kt | 4 +- buildSrc/src/main/java/AndroidComponents.kt | 2 +- 5 files changed, 125 insertions(+), 26 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddleware.kt b/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddleware.kt index 35d106f915..09376083d7 100644 --- a/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddleware.kt +++ b/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddleware.kt @@ -14,6 +14,7 @@ import mozilla.components.browser.state.selector.findTab import mozilla.components.browser.state.selector.selectedNormalTab import mozilla.components.browser.state.state.BrowserState import mozilla.components.browser.state.state.TabSessionState +import mozilla.components.feature.search.ext.parseSearchTerms import mozilla.components.lib.state.Middleware import mozilla.components.lib.state.MiddlewareContext import mozilla.components.lib.state.Store @@ -66,12 +67,10 @@ class HistoryMetadataMiddleware( is ContentAction.UpdateLoadingStateAction -> { context.state.findNormalTab(action.sessionId)?.let { tab -> val selectedTab = tab.id == context.state.selectedTabId - if (tab.content.loading && !action.loading) { - // When a page stops loading we record its metadata - createHistoryMetadata(context, tab) - } else if (!tab.content.loading && action.loading && selectedTab) { + if (!tab.content.loading && action.loading && selectedTab) { // When a page starts loading (e.g. user navigated away by - // clicking on a link) we update metadata + // clicking on a link) we update metadata for the selected + // (i.e. previous) url of this tab. updateHistoryMetadata(tab) } } @@ -80,10 +79,25 @@ class HistoryMetadataMiddleware( next(action) - // Post process actions + // Post process actions. At this point, tab state will be up-to-date and will possess any + // 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) { - // We're handling this after processing the action because we want the tab - // state to contain the updated media session state. + // 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) + } + } + } + // NB: this could be called bunch of times in quick succession. is MediaSessionAction.UpdateMediaMetadataAction -> { context.state.findNormalTab(action.tabId)?.let { tab -> createHistoryMetadata(context, tab) @@ -93,7 +107,22 @@ class HistoryMetadataMiddleware( } private fun createHistoryMetadata(context: MiddlewareContext, tab: TabSessionState) { - val key = historyMetadataService.createMetadata(tab, tab.getParent(context.store)) + val tabParent = tab.getParent(context.store) + val previousUrlIndex = tab.content.history.currentIndex - 1 + + // Obtain search terms and referrer url either from tab parent, or from the history stack. + val (searchTerm, referrerUrl) = when { + tabParent != null -> { + tabParent.content.searchTerms.takeUnless { it.isEmpty() } to tabParent.content.url + } + previousUrlIndex >= 0 -> { + val previousUrl = tab.content.history.items[previousUrlIndex].uri + context.state.search.parseSearchTerms(previousUrl) to previousUrl + } + else -> null to null + } + + val key = historyMetadataService.createMetadata(tab, searchTerm, referrerUrl) context.dispatch(HistoryMetadataAction.SetHistoryMetadataKeyAction(tab.id, key)) } diff --git a/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataService.kt b/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataService.kt index b898baddae..6f9df959ae 100644 --- a/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataService.kt +++ b/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataService.kt @@ -23,9 +23,14 @@ interface HistoryMetadataService { * Creates a history metadata record for the provided tab. * * @param tab the [TabSessionState] to record metadata for. - * @param parent the parent [TabSessionState] for search and domain grouping purposes. + * @param searchTerms Search terms associated with this metadata. + * @param referrerUrl Referrer url associated with this metadata. */ - fun createMetadata(tab: TabSessionState, parent: TabSessionState? = null): HistoryMetadataKey + fun createMetadata( + tab: TabSessionState, + searchTerms: String? = null, + referrerUrl: String? = null + ): HistoryMetadataKey /** * Updates the history metadata corresponding to the provided tab. @@ -51,14 +56,14 @@ class DefaultHistoryMetadataService( private val logger = Logger("DefaultHistoryMetadataService") - override fun createMetadata(tab: TabSessionState, parent: TabSessionState?): HistoryMetadataKey { + override fun createMetadata(tab: TabSessionState, searchTerms: String?, referrerUrl: String?): HistoryMetadataKey { logger.debug("Creating metadata for tab ${tab.id}") val existingMetadata = tab.historyMetadata val metadataKey = if (existingMetadata != null && existingMetadata.url == tab.content.url) { existingMetadata } else { - tab.toHistoryMetadataKey(parent) + tab.toHistoryMetadataKey(searchTerms, referrerUrl) } val documentTypeObservation = HistoryMetadataObservation.DocumentTypeObservation( @@ -95,9 +100,9 @@ class DefaultHistoryMetadataService( } } -fun TabSessionState.toHistoryMetadataKey(parent: TabSessionState? = null): HistoryMetadataKey = +fun TabSessionState.toHistoryMetadataKey(searchTerms: String?, referrerUrl: String?): HistoryMetadataKey = HistoryMetadataKey( url = content.url, - referrerUrl = parent?.content?.url, - searchTerm = parent?.content?.searchTerms.takeUnless { it.isNullOrEmpty() } + referrerUrl = referrerUrl, + searchTerm = searchTerms ) diff --git a/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddlewareTest.kt b/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddlewareTest.kt index e96f309a6e..bb844a026c 100644 --- a/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddlewareTest.kt +++ b/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddlewareTest.kt @@ -12,20 +12,27 @@ import io.mockk.verify import kotlinx.coroutines.ExperimentalCoroutinesApi import mozilla.components.browser.state.action.ContentAction import mozilla.components.browser.state.action.MediaSessionAction +import mozilla.components.browser.state.action.SearchAction import mozilla.components.browser.state.action.TabListAction import mozilla.components.browser.state.engine.EngineMiddleware +import mozilla.components.browser.state.search.SearchEngine import mozilla.components.browser.state.selector.findTab import mozilla.components.browser.state.state.BrowserState import mozilla.components.browser.state.state.TabSessionState import mozilla.components.browser.state.state.createTab import mozilla.components.browser.state.store.BrowserStore +import mozilla.components.concept.engine.history.HistoryItem 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.Before import org.junit.Test +import org.junit.runner.RunWith +import org.mozilla.fenix.helpers.FenixRobolectricTestRunner @ExperimentalCoroutinesApi +@RunWith(FenixRobolectricTestRunner::class) class HistoryMetadataMiddlewareTest { private lateinit var store: BrowserStore @@ -43,22 +50,81 @@ class HistoryMetadataMiddlewareTest { } @Test - fun `GIVEN normal tab WHEN loading completed THEN meta data is recorded`() { + fun `GIVEN normal tab WHEN history is updated THEN meta data is also recorded`() { val tab = createTab("https://mozilla.org") val expectedKey = HistoryMetadataKey(url = tab.content.url) every { service.createMetadata(any(), any()) } returns expectedKey store.dispatch(TabListAction.AddTabAction(tab)).joinBlocking() - store.dispatch(ContentAction.UpdateLoadingStateAction(tab.id, true)).joinBlocking() verify { service wasNot Called } - store.dispatch(ContentAction.UpdateLoadingStateAction(tab.id, false)).joinBlocking() + store.dispatch(ContentAction.UpdateHistoryStateAction(tab.id, emptyList(), currentIndex = 0)).joinBlocking() val capturedTab = slot() - verify { service.createMetadata(capture(capturedTab), null) } + verify(exactly = 1) { service.createMetadata(capture(capturedTab)) } + + // Not recording if url didn't change. + store.dispatch(ContentAction.UpdateHistoryStateAction(tab.id, emptyList(), currentIndex = 0)).joinBlocking() + verify(exactly = 1) { service.createMetadata(capture(capturedTab)) } assertEquals(tab.id, capturedTab.captured.id) assertEquals(expectedKey, store.state.findTab(tab.id)?.historyMetadata) + + // Now, test that we'll record metadata for the same tab after url is changed. + store.dispatch(ContentAction.UpdateUrlAction(tab.id, "https://firefox.com")).joinBlocking() + store.dispatch(ContentAction.UpdateHistoryStateAction(tab.id, emptyList(), currentIndex = 0)).joinBlocking() + verify(exactly = 2) { service.createMetadata(capture(capturedTab)) } + + assertEquals(tab.id, capturedTab.captured.id) + assertEquals(expectedKey, store.state.findTab(tab.id)?.historyMetadata) + } + + @Test + fun `GIVEN normal tab has parent 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() + store.dispatch(TabListAction.AddTabAction(tab)).joinBlocking() + + store.dispatch(ContentAction.UpdateHistoryStateAction(tab.id, emptyList(), currentIndex = 0)).joinBlocking() + verify { + service.createMetadata(any(), eq("mozilla website"), eq("https://google.com?q=mozilla+website")) + } + } + + @Test + fun `GIVEN normal tab has no parent WHEN history metadata is recorded THEN search terms and referrer url are provided`() { + val tab = createTab("https://mozilla.org") + store.dispatch(TabListAction.AddTabAction(tab)).joinBlocking() + store.dispatch(SearchAction.SetSearchEnginesAction( + regionSearchEngines = listOf( + SearchEngine( + id = "google", + name = "Google", + icon = mock(), + type = SearchEngine.Type.BUNDLED, + resultUrls = listOf("https://google.com?q={searchTerms}") + ) + ), + userSelectedSearchEngineId = null, + userSelectedSearchEngineName = null, + regionDefaultSearchEngineId = "google", + customSearchEngines = emptyList(), + hiddenSearchEngines = emptyList(), + additionalAvailableSearchEngines = emptyList(), + additionalSearchEngines = emptyList(), + regionSearchEnginesOrder = listOf("google") + )).joinBlocking() + + val historyState = listOf( + HistoryItem("firefox", "https://google.com?q=mozilla+website"), + HistoryItem("mozilla", "https://mozilla.org") + ) + store.dispatch(ContentAction.UpdateHistoryStateAction(tab.id, historyState, currentIndex = 1)).joinBlocking() + + verify { + service.createMetadata(any(), eq("mozilla website"), eq("https://google.com?q=mozilla+website")) + } } @Test @@ -69,10 +135,9 @@ class HistoryMetadataMiddlewareTest { every { service.createMetadata(any(), any()) } returns expectedKey store.dispatch(TabListAction.AddTabAction(tab)).joinBlocking() - store.dispatch(ContentAction.UpdateLoadingStateAction(tab.id, true)).joinBlocking() verify { service wasNot Called } - store.dispatch(ContentAction.UpdateLoadingStateAction(tab.id, false)).joinBlocking() + store.dispatch(ContentAction.UpdateHistoryStateAction(tab.id, emptyList(), currentIndex = 0)).joinBlocking() verify { service wasNot Called } } @@ -130,7 +195,7 @@ class HistoryMetadataMiddlewareTest { store.dispatch(MediaSessionAction.UpdateMediaMetadataAction(tab.id, mockk())).joinBlocking() val capturedTab = slot() - verify { service.createMetadata(capture(capturedTab), null) } + verify { service.createMetadata(capture(capturedTab)) } assertEquals(tab.id, capturedTab.captured.id) assertEquals(expectedKey, store.state.findTab(tab.id)?.historyMetadata) diff --git a/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataServiceTest.kt b/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataServiceTest.kt index 588b2252f8..b1f80bb7d5 100644 --- a/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataServiceTest.kt +++ b/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataServiceTest.kt @@ -42,10 +42,10 @@ class HistoryMetadataServiceTest { fun `GIVEN a regular page WHEN metadata is created THEN a regular document type observation is recorded`() { val parent = createTab("https://mozilla.org") val tab = createTab("https://blog.mozilla.org", parent = parent) - service.createMetadata(tab, parent) + service.createMetadata(tab, searchTerms = "hello", referrerUrl = parent.content.url) testDispatcher.advanceUntilIdle() - val expectedKey = HistoryMetadataKey(url = tab.content.url, referrerUrl = parent.content.url) + val expectedKey = HistoryMetadataKey(url = tab.content.url, searchTerm = "hello", referrerUrl = parent.content.url) val expectedObservation = HistoryMetadataObservation.DocumentTypeObservation(documentType = DocumentType.Regular) coVerify { storage.noteHistoryMetadataObservation(expectedKey, expectedObservation) } } diff --git a/buildSrc/src/main/java/AndroidComponents.kt b/buildSrc/src/main/java/AndroidComponents.kt index 9d150168a9..ebd22c08d0 100644 --- a/buildSrc/src/main/java/AndroidComponents.kt +++ b/buildSrc/src/main/java/AndroidComponents.kt @@ -3,5 +3,5 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ object AndroidComponents { - const val VERSION = "91.0.20210617143333" + const val VERSION = "91.0.20210617193910" }