Closes #21659: Add SERP to history search groups

app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddleware.kt

@@ -14,6 +14,7 @@ import mozilla.components.browser.state.selector.findNormalTab
 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.SearchState
 import mozilla.components.browser.state.state.TabSessionState
 import mozilla.components.feature.search.ext.parseSearchTerms
 import mozilla.components.lib.state.Middleware
@@ -141,40 +142,48 @@ class HistoryMetadataMiddleware(
         }
     }
 
-    private fun createHistoryMetadata(context: MiddlewareContext<BrowserState, BrowserAction>, tab: TabSessionState) {
+    @Suppress("ComplexMethod")
+    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, 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
+        //
+        // At a high level, there are two main cases here:
+        // 1) The tab was opened as a 'new tab' via the search results page. In this case we obtain
+        // search terms via the tab's parent (the search results page). However, it's possible that
+        // the 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 group
         // unless a direct navigation event happens.
+        //
+        // 2) A page was opened in the same tab as the search results page (navigated to via content).
         val (searchTerm, referrerUrl) = when {
-            // Loading page opened in a New Tab for the first time.
+            // Page was opened in a new tab. Look for search terms in the parent tab.
             tabParent != null && !tabMetadataHasSearchTerms -> {
-                val searchTerms = tabParent.content.searchTerms.takeUnless { it.isEmpty() }
-                    ?: context.state.search.parseSearchTerms(tabParent.content.url)
+                val searchTerms = findSearchTerms(tabParent, context.state.search)
                 searchTerms to tabParent.content.url
             }
-            // We only want to inspect the previous url in history if the user navigated via
-            // web content i.e., they followed a link, not if the user navigated directly via
-            // toolbar.
+            // Page was navigated to via content i.e., the user followed a link. Look for search terms in tab history.
             !directLoadTriggered && previousUrlIndex >= 0 -> {
                 // Once a tab is within the search group, only a direct load event (via the toolbar) can change that.
+                val previousUrl = tab.content.history.items[previousUrlIndex].uri
                 val (searchTerms, referrerUrl) = if (tabMetadataHasSearchTerms) {
-                    tab.historyMetadata?.searchTerm to tab.historyMetadata?.referrerUrl
+                    tab.historyMetadata?.searchTerm to previousUrl
                 } else {
-                    val previousUrl = tab.content.history.items[previousUrlIndex].uri
-                    context.state.search.parseSearchTerms(previousUrl) to previousUrl
+                    // Find search terms by checking if page is a SERP or a result opened from a SERP
+                    val searchTerms = findSearchTerms(tab, context.state.search)
+                    if (searchTerms != null) {
+                        searchTerms to null
+                    } else {
+                        context.state.search.parseSearchTerms(previousUrl) to previousUrl
+                    }
                 }
 
                 if (searchTerms != null) {
@@ -190,10 +199,11 @@ class HistoryMetadataMiddleware(
             tabMetadataHasSearchTerms && !(directLoadTriggered && previousUrlIndex >= 0) -> {
                 tab.historyMetadata?.searchTerm to tab.historyMetadata?.referrerUrl
             }
-            // We had no search terms, no history stack, and no parent.
-            // This would be the case for any page loaded directly via the toolbar including
-            // a search results page itself. For now, the original search results page is not
-            // part of the search group: https://github.com/mozilla-mobile/fenix/issues/21659.
+            // A page was loaded directly or via content. Check if it is a search result page.
+            !tabMetadataHasSearchTerms -> {
+                findSearchTerms(tab, context.state.search) to null
+            }
+            // No search terms or referrer were detected.
             else -> null to null
         }
 
@@ -218,4 +228,8 @@ class HistoryMetadataMiddleware(
             store.state.findTab(it)
         }
     }
+
+    private fun findSearchTerms(tab: TabSessionState, searchState: SearchState): String? {
+        return tab.content.searchTerms.takeUnless { it.isEmpty() } ?: searchState.parseSearchTerms(tab.content.url)
+    }
 }

app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataMiddlewareTest.kt

@@ -114,6 +114,55 @@ class HistoryMetadataMiddlewareTest {
         }
     }
 
+    @Test
+    fun `GIVEN normal tab is a search engine result page WHEN history metadata is recorded THEN search terms are provided`() {
+        service = TestingMetadataService()
+        middleware = HistoryMetadataMiddleware(service)
+        val tab = createTab("about:blank")
+        store = BrowserStore(
+            middleware = listOf(middleware) + EngineMiddleware.create(engine = mockk()),
+            initialState = BrowserState(
+                tabs = listOf(tab)
+            )
+        )
+        setupGoogleSearchEngine()
+
+        val serpUrl = "https://google.com?q=mozilla+website"
+        store.dispatch(EngineAction.LoadUrlAction(tab.id, serpUrl)).joinBlocking()
+        store.dispatch(ContentAction.UpdateUrlAction(tab.id, serpUrl)).joinBlocking()
+        store.dispatch(ContentAction.UpdateHistoryStateAction(tab.id, listOf(HistoryItem("Google Search", serpUrl)), currentIndex = 0)).joinBlocking()
+        with((service as TestingMetadataService).createdMetadata) {
+            assertEquals(1, this.count())
+            assertEquals("https://google.com?q=mozilla+website", this[0].url)
+            assertEquals("mozilla website", this[0].searchTerm)
+            assertNull(this[0].referrerUrl)
+        }
+    }
+
+    @Test
+    fun `GIVEN normal tab navigates to search engine result page WHEN history metadata is recorded THEN search terms are provided`() {
+        service = TestingMetadataService()
+        middleware = HistoryMetadataMiddleware(service)
+        val tab = createTab("https://google.com")
+        store = BrowserStore(
+            middleware = listOf(middleware) + EngineMiddleware.create(engine = mockk()),
+            initialState = BrowserState(
+                tabs = listOf(tab)
+            )
+        )
+        setupGoogleSearchEngine()
+
+        val serpUrl = "https://google.com?q=mozilla+website"
+        store.dispatch(ContentAction.UpdateUrlAction(tab.id, serpUrl)).joinBlocking()
+        store.dispatch(ContentAction.UpdateHistoryStateAction(tab.id, listOf(HistoryItem("Google Search", "https://google.com"), HistoryItem("Google Search", serpUrl)), currentIndex = 1)).joinBlocking()
+        with((service as TestingMetadataService).createdMetadata) {
+            assertEquals(1, this.count())
+            assertEquals("https://google.com?q=mozilla+website", this[0].url)
+            assertEquals("mozilla website", this[0].searchTerm)
+            assertNull(this[0].referrerUrl)
+        }
+    }
+
     @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()
@@ -132,7 +181,7 @@ class HistoryMetadataMiddlewareTest {
         with((service as TestingMetadataService).createdMetadata) {
             assertEquals(2, this.count())
             assertEquals("https://google.com?q=mozilla+website", this[0].url)
-            assertNull(this[0].searchTerm)
+            assertEquals("mozilla website", this[0].searchTerm)
             assertNull(this[0].referrerUrl)
 
             assertEquals("https://google.com?url=https://mozilla.org", this[1].url)
@@ -180,7 +229,7 @@ class HistoryMetadataMiddlewareTest {
             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)
+            assertEquals("https://mozilla.org", this[4].referrerUrl)
         }
     }
 
@@ -202,7 +251,7 @@ class HistoryMetadataMiddlewareTest {
         with((service as TestingMetadataService).createdMetadata) {
             assertEquals(2, this.count())
             assertEquals("https://google.com?q=mozilla+website", this[0].url)
-            assertNull(this[0].searchTerm)
+            assertEquals("mozilla website", this[0].searchTerm)
             assertNull(this[0].referrerUrl)
 
             assertEquals("https://google.com?url=https://mozilla.org", this[1].url)
@@ -258,7 +307,7 @@ class HistoryMetadataMiddlewareTest {
         with((service as TestingMetadataService).createdMetadata) {
             assertEquals(2, this.count())
             assertEquals("https://google.com?q=mozilla+website", this[0].url)
-            assertNull(this[0].searchTerm)
+            assertEquals("mozilla website", this[0].searchTerm)
             assertNull(this[0].referrerUrl)
 
             assertEquals("https://google.com?url=https://mozilla.org", this[1].url)
@@ -298,7 +347,7 @@ class HistoryMetadataMiddlewareTest {
 
     @Test
     fun `GIVEN normal tab has parent WHEN url is the same THEN nothing happens`() {
-        val parentTab = createTab("https://mozilla.org", searchTerms = "mozilla website")
+        val parentTab = createTab("https://mozilla.org")
         val tab = createTab("https://mozilla.org", parent = parentTab)
         store.dispatch(TabListAction.AddTabAction(parentTab)).joinBlocking()
         store.dispatch(TabListAction.AddTabAction(tab)).joinBlocking()