[Tab Groups] Only create a tab group if it is the second link.

This is merely a heuristic for whether or not to create a tab group
based on the opener of the tab next to the current one.

Bug: 1128703
Change-Id: Iae0ae8db78be0bbe9be2fdacd143c70e1a0d49bc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2567818
Commit-Queue: Charlene Yan <cyan@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Connie Wan <connily@chromium.org>
Cr-Commit-Position: refs/heads/master@{#834915}

chrome/browser/ui/browser_browsertest.cc

@@ -870,30 +870,32 @@ class BrowserTestWithTabGroupsAutoCreateEnabled : public BrowserTest {
 };
 
 IN_PROC_BROWSER_TEST_F(BrowserTestWithTabGroupsAutoCreateEnabled,
-                       NewTabFromLinkInGroupedTabOpensInGroup) {
+                       FirstNewTabFromLinkWithSameDomainDoesNotCreateGroup) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
-  // Add a grouped tab.
+  // Open a tab not in a group.
   TabStripModel* const model = browser()->tab_strip_model();
-  ui_test_utils::NavigateToURL(browser(),
-                               embedded_test_server()->GetURL("/empty.html"));
-  const tab_groups::TabGroupId group_id = model->AddToNewGroup({0});
+  GURL url1("http://www.example.com/empty.html");
+  ui_test_utils::NavigateToURL(browser(), url1);
+  ASSERT_FALSE(model->GetTabGroupForTab(0).has_value());
 
-  // Open a new background tab.
+  // Open a new background tab with the same domain as the active tab.
   WebContents* const contents =
       browser()->tab_strip_model()->GetActiveWebContents();
+  GURL url2("http://www.example.com/");
   OpenURLFromTab(
       contents,
-      OpenURLParams(embedded_test_server()->GetURL("/empty.html"), Referrer(),
-                    WindowOpenDisposition::NEW_BACKGROUND_TAB,
+      OpenURLParams(url2, Referrer(), WindowOpenDisposition::NEW_BACKGROUND_TAB,
                     ui::PAGE_TRANSITION_TYPED, false));
 
-  // It should have inherited the tab group from the first tab.
-  EXPECT_EQ(group_id, model->GetTabGroupForTab(1));
+  // The new tab which has the same domain as the tab it originated from should
+  // not create a new group.
+  EXPECT_FALSE(model->GetTabGroupForTab(0).has_value());
+  EXPECT_FALSE(model->GetTabGroupForTab(1).has_value());
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserTestWithTabGroupsAutoCreateEnabled,
-                       NewTabFromLinkWithSameDomainCreatesGroup) {
+                       SecondNewTabFromLinkWithSameDomainCreatesGroup) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
   // Open a tab not in a group.
@@ -902,7 +904,7 @@ IN_PROC_BROWSER_TEST_F(BrowserTestWithTabGroupsAutoCreateEnabled,
   ui_test_utils::NavigateToURL(browser(), url1);
   ASSERT_FALSE(model->GetTabGroupForTab(0).has_value());
 
-  // Open a new background tab with the same domain as the active tab.
+  // Open two new background tabs with the same domain as the active tab.
   WebContents* const contents =
       browser()->tab_strip_model()->GetActiveWebContents();
   GURL url2("http://www.example.com/");
@@ -910,15 +912,24 @@ IN_PROC_BROWSER_TEST_F(BrowserTestWithTabGroupsAutoCreateEnabled,
       contents,
       OpenURLParams(url2, Referrer(), WindowOpenDisposition::NEW_BACKGROUND_TAB,
                     ui::PAGE_TRANSITION_TYPED, false));
+  OpenURLFromTab(
+      contents,
+      OpenURLParams(url2, Referrer(), WindowOpenDisposition::NEW_BACKGROUND_TAB,
+                    ui::PAGE_TRANSITION_TYPED, false));
 
-  // The new tab which has the same domain as the tab it originated from should
-  // be grouped with its parent tab.
+  // Opening the second tab from the source will result in the source tab and
+  // the two tabs opened from the source to be added in a new group.
+  ASSERT_EQ(model->count(), 3);
   EXPECT_TRUE(model->GetTabGroupForTab(0).has_value());
   EXPECT_TRUE(model->GetTabGroupForTab(1).has_value());
+  EXPECT_TRUE(model->GetTabGroupForTab(2).has_value());
   EXPECT_EQ(model->GetTabGroupForTab(0).value(),
             model->GetTabGroupForTab(1).value());
+  EXPECT_EQ(model->GetTabGroupForTab(0).value(),
+            model->GetTabGroupForTab(2).value());
 }
 
+// TODO(crbug.com/997344): Test this with implicitly-created links.
 IN_PROC_BROWSER_TEST_F(BrowserTestWithTabGroupsAutoCreateEnabled,
                        NewTabFromLinkWithDifferentDomainDoesNotCreateGroup) {
   ASSERT_TRUE(embedded_test_server()->Start());
@@ -944,6 +955,7 @@ IN_PROC_BROWSER_TEST_F(BrowserTestWithTabGroupsAutoCreateEnabled,
   EXPECT_FALSE(model->GetTabGroupForTab(1).has_value());
 }
 
+// TODO(crbug.com/997344): Test this with implicitly-created links.
 IN_PROC_BROWSER_TEST_F(BrowserTest, TargetBlankLinkOpensInGroup) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
@@ -964,8 +976,27 @@ IN_PROC_BROWSER_TEST_F(BrowserTest, TargetBlankLinkOpensInGroup) {
   EXPECT_EQ(group_id, browser()->tab_strip_model()->GetTabGroupForTab(1));
 }
 
-// TODO(crbug.com/997344): Test the above two scenarios with implicitly-created
-// links, if still applicable.
+IN_PROC_BROWSER_TEST_F(BrowserTest, NewTabFromLinkInGroupedTabOpensInGroup) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  // Add a grouped tab.
+  TabStripModel* const model = browser()->tab_strip_model();
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/empty.html"));
+  const tab_groups::TabGroupId group_id = model->AddToNewGroup({0});
+
+  // Open a new background tab.
+  WebContents* const contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  OpenURLFromTab(
+      contents,
+      OpenURLParams(embedded_test_server()->GetURL("/empty.html"), Referrer(),
+                    WindowOpenDisposition::NEW_BACKGROUND_TAB,
+                    ui::PAGE_TRANSITION_TYPED, false));
+
+  // It should have inherited the tab group from the first tab.
+  EXPECT_EQ(group_id, model->GetTabGroupForTab(1));
+}
 
 // BeforeUnloadAtQuitWithTwoWindows is a regression test for
 // http://crbug.com/11842. It opens two windows, one of which has a

chrome/browser/ui/browser_tabstrip.cc

@@ -117,15 +117,41 @@ void ConfigureTabGroupForNavigation(NavigateParams* nav_params) {
   if (nav_params->disposition == WindowOpenDisposition::NEW_FOREGROUND_TAB ||
       nav_params->disposition == WindowOpenDisposition::NEW_BACKGROUND_TAB) {
     nav_params->group = model->GetTabGroupForTab(source_index);
-    if (base::FeatureList::IsEnabled(features::kTabGroupsAutoCreate) &&
-        !nav_params->group.has_value() && !model->IsTabPinned(source_index)) {
-      const GURL& source_url =
-          nav_params->source_contents->GetLastCommittedURL();
-      const GURL& target_url = nav_params->url;
-      if (target_url.DomainIs(source_url.host_piece()))
-        nav_params->group = model->AddToNewGroup({source_index});
+
+    // Because the target tab has not opened yet, adding the source tab, and the
+    // tab immediately to the right of the source tab will also result in the
+    // target tab getting added to this group.
+    if (ShouldAutoCreateGroupForNavigation(nav_params)) {
+      nav_params->group =
+          model->AddToNewGroup({source_index, source_index + 1});
+    }
+  }
+}
+
+bool ShouldAutoCreateGroupForNavigation(NavigateParams* nav_params) {
+  TabStripModel* model = nav_params->browser->tab_strip_model();
+  const int source_index =
+      model->GetIndexOfWebContents(nav_params->source_contents);
+  if (!base::FeatureList::IsEnabled(features::kTabGroupsAutoCreate) ||
+      nav_params->group.has_value() || model->IsTabPinned(source_index)) {
+    return false;
+  }
+  const GURL& source_url = nav_params->source_contents->GetLastCommittedURL();
+  const GURL& target_url = nav_params->url;
+
+  // If the opener of the tab to the right has the same domain as the
+  // souce URL, create a new group.
+  if (target_url.DomainIs(source_url.host_piece()) &&
+      model->ContainsIndex(source_index + 1)) {
+    content::WebContents* neighbor_opener_contents =
+        model->GetOpenerOfWebContentsAt(source_index + 1);
+    if (neighbor_opener_contents &&
+        source_url.DomainIs(
+            neighbor_opener_contents->GetLastCommittedURL().host_piece())) {
+      return true;
     }
   }
+  return false;
 }
 
 }  // namespace chrome

chrome/browser/ui/browser_tabstrip.h

@@ -58,6 +58,9 @@ void CloseWebContents(Browser* browser,
 // applicable.
 void ConfigureTabGroupForNavigation(NavigateParams* nav_params);
 
+// Decides whether or not to create a new tab group.
+bool ShouldAutoCreateGroupForNavigation(NavigateParams* nav_params);
+
 }  // namespace chrome
 
 #endif  // CHROME_BROWSER_UI_BROWSER_TABSTRIP_H_