From 78e85949cfbc3a28c410dd92151cff0bfb2c6e73 Mon Sep 17 00:00:00 2001 From: Grisha Kruglov Date: Wed, 17 Nov 2021 20:27:24 -0800 Subject: [PATCH] [fenix] Closes https://github.com/mozilla-mobile/fenix/issues/22484 - Track direct loads per tab; fix search term lookup This patch fixes two problems: 1) We were treating "direct tab load" as an event which applies uniformally to all tabs, even though it's actually an event which happens for a specific tab. This lead to background tabs (pages opened as new tab) setting the direct load flag, and then a simultaneously loading parent tab would incorrectly interpret that flag for itself. The patch switches this tracking from a simple boolean (are we direct loading?) to a set of tab IDs that are currently direct loading. 2) In a case when a background tab was loading with a parent who's search terms were cleared by a direct load, we were not trying to lookup search terms on the background tab's historyMetadata key, which exists to capture search terms for this exact scenario. The patch adds an additional fallback lookup for that path. --- .../HistoryMetadataMiddleware.kt | 33 +++++++--- .../HistoryMetadataMiddlewareTest.kt | 63 +++++++++++++++++++ 2 files changed, 88 insertions(+), 8 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 ee5e791f10..39d0ccdfab 100644 --- a/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddleware.kt +++ b/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddleware.kt @@ -31,9 +31,9 @@ class HistoryMetadataMiddleware( private val logger = Logger("HistoryMetadataMiddleware") - // Tracks whether a page load is in progress that was triggered directly by the app + // Tracks whether a load is in progress for a tab/session ID that was triggered directly by the app // e.g. via the toolbar as opposed to via web content. - private var directLoadTriggered: Boolean = false + private var directLoadTriggeredSet = mutableSetOf() @Suppress("ComplexMethod") override fun invoke( @@ -90,11 +90,13 @@ class HistoryMetadataMiddleware( } } } - is EngineAction.LoadUrlAction, - is EngineAction.OptimizedLoadUrlTriggeredAction -> { + is EngineAction.LoadUrlAction -> { // This isn't an ideal fix as we shouldn't have to hold any state in the middleware: // https://github.com/mozilla-mobile/android-components/issues/11034 - directLoadTriggered = true + directLoadTriggeredSet.add(action.tabId) + } + is EngineAction.OptimizedLoadUrlTriggeredAction -> { + directLoadTriggeredSet.add(action.tabId) } } @@ -116,7 +118,7 @@ class HistoryMetadataMiddleware( } // Once we get a history update let's reset the flag for future loads. - directLoadTriggered = false + directLoadTriggeredSet.remove(action.sessionId) } // NB: this could be called bunch of times in quick succession. is MediaSessionAction.UpdateMediaMetadataAction -> { @@ -150,6 +152,7 @@ class HistoryMetadataMiddleware( val tabParent = tab.getParent(context.store) val previousUrlIndex = tab.content.history.currentIndex - 1 val tabMetadataHasSearchTerms = !tab.historyMetadata?.searchTerm.isNullOrBlank() + val directLoadTriggered = directLoadTriggeredSet.contains(tab.id) // Obtain search terms and referrer url either from tab parent, from the history stack, or // from the tab itself. @@ -200,7 +203,9 @@ class HistoryMetadataMiddleware( tab.historyMetadata?.searchTerm to tab.historyMetadata?.referrerUrl } // In all other cases (e.g. direct load) find search terms by checking if page is a SERP - else -> findSearchTerms(tab, context.state.search) to null + else -> { + findSearchTerms(tab, context.state.search) to null + } } // Sanity check to make sure we don't record a metadata record referring to itself. @@ -226,6 +231,18 @@ class HistoryMetadataMiddleware( } private fun findSearchTerms(tab: TabSessionState, searchState: SearchState): String? { - return tab.content.searchTerms.takeUnless { it.isEmpty() } ?: searchState.parseSearchTerms(tab.content.url) + // Only check for search terms in metadata if we're not direct loading this tab. If we are, + // we don't retain previous search terms. + // `tab.content.searchTerms` are cleared as a side-effect of performing a direct load. + val metadataSearchTerms: () -> String? = { + if (!directLoadTriggeredSet.contains(tab.id)) { + tab.historyMetadata?.searchTerm.takeUnless { it.isNullOrEmpty() } + } else { + null + } + } + return tab.content.searchTerms.takeUnless { it.isEmpty() } + ?: metadataSearchTerms() + ?: searchState.parseSearchTerms(tab.content.url) } } 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 99123b5450..51e20f2bd3 100644 --- a/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddlewareTest.kt +++ b/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddlewareTest.kt @@ -14,6 +14,7 @@ import mozilla.components.browser.state.action.EngineAction 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.action.HistoryMetadataAction import mozilla.components.browser.state.engine.EngineMiddleware import mozilla.components.browser.state.search.SearchEngine import mozilla.components.browser.state.selector.findTab @@ -233,6 +234,68 @@ class HistoryMetadataMiddlewareTest { } } + @Test + fun `GIVEN tab opened as new tab from a search page WHEN it loads while parent navigates to a result THEN parent will retain its 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) + assertEquals("mozilla website", 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) + } + + // Parent tab loads. + store.dispatch(ContentAction.UpdateHistoryStateAction(parentTab.id, listOf(HistoryItem("Google - mozilla website", "https://google.com?q=mozilla+website")), 0)).joinBlocking() + with((service as TestingMetadataService).createdMetadata) { + assertEquals(2, this.count()) + } + + // Simulate a state where search metadata is missing for the child tab. + store.dispatch(HistoryMetadataAction.SetHistoryMetadataKeyAction(tab.id, HistoryMetadataKey("https://google.com?url=https://mozilla.org", null, null))).joinBlocking() + + // Parent navigates away, while the child starts loading. A mostly realistic sequence of events... + store.dispatch(ContentAction.UpdateUrlAction(parentTab.id, "https://firefox.com")).joinBlocking() + store.dispatch(ContentAction.UpdateSearchTermsAction(parentTab.id, "")).joinBlocking() + store.dispatch(EngineAction.LoadUrlAction(tab.id, "https://google.com?url=https://mozilla.org")).joinBlocking() + store.dispatch(ContentAction.UpdateHistoryStateAction(tab.id, listOf(HistoryItem("", "https://google.com?url=https://mozilla.org")), currentIndex = 0)).joinBlocking() + 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() + 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(4, this.count()) + assertEquals("https://firefox.com", this[3].url) + assertEquals("mozilla website", this[3].searchTerm) + assertEquals("https://google.com?q=mozilla+website", this[3].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) + + assertEquals("https://mozilla.org", this[2].url) + assertEquals("mozilla website", this[2].searchTerm) + // This is suspect. The parent tab switched away right before the child loaded, so the + // referrer here is potentially bogus. + assertEquals("https://firefox.com", this[2].referrerUrl) + } + } + @Test fun `GIVEN tab with search terms WHEN subsequent direct load occurs THEN search terms are not retained`() { service = TestingMetadataService()