Uplift of #25983 (squashed) to release

Author: brave-builds

browser/ui/tabs/brave_tab_strip_model.cc

@@ -10,17 +10,14 @@
 #include <iterator>
 #include <vector>
 
-#include "base/containers/adapters.h"
 #include "base/containers/span.h"
 #include "base/ranges/algorithm.h"
 #include "brave/browser/ui/brave_browser_window.h"
-#include "brave/browser/ui/views/tabs/vertical_tab_utils.h"
 #include "brave/components/constants/pref_names.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_window.h"
-#include "chrome/browser/ui/tabs/tab_group_model.h"
 #include "chrome/browser/ui/tabs/tab_strip_model_delegate.h"
 #include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
 #include "components/prefs/pref_service.h"
@@ -48,28 +45,6 @@ void BraveTabStripModel::SelectRelativeTab(TabRelativeDirection direction,
   }
 }
 
-void BraveTabStripModel::ExecuteContextMenuCommand(
-    int context_index,
-    ContextMenuCommand command_id) {
-  Browser* browser = chrome::FindBrowserWithTab(GetWebContentsAt(0));
-  if (tabs::utils::ShouldShowVerticalTabs(browser) &&
-      command_id == CommandTogglePinned && WillContextMenuPin(context_index)) {
-    // For vertical tabs, we need to ungroup the tabs before pinning tabs in
-    // order to avoid NOTREACHED.
-    // https://github.com/brave/brave-browser/issues/40201
-    auto indices = GetIndicesForCommand(context_index);
-    for (auto index : base::Reversed(indices)) {
-      if (!GetTabGroupForTab(index).has_value()) {
-        continue;
-      }
-
-      RemoveFromGroup({index});
-    }
-  }
-
-  TabStripModel::ExecuteContextMenuCommand(context_index, command_id);
-}
-
 void BraveTabStripModel::SelectMRUTab(TabRelativeDirection direction,
                                       TabStripUserGestureDetails detail) {
   if (mru_cycle_list_.empty()) {

browser/ui/tabs/brave_tab_strip_model.h

@@ -45,8 +45,6 @@ class BraveTabStripModel : public TabStripModel {
   // TabStripModel:
   void SelectRelativeTab(TabRelativeDirection direction,
                          TabStripUserGestureDetails detail) override;
-  void ExecuteContextMenuCommand(int context_index,
-                                 ContextMenuCommand command_id) override;
 
  private:
   // List of tab indexes sorted by most recently used

browser/ui/views/tabs/brave_tab.cc

@@ -7,6 +7,7 @@
 
 #include <algorithm>
 #include <optional>
+#include <utility>
 
 #include "brave/browser/ui/tabs/brave_tab_layout_constants.h"
 #include "brave/browser/ui/tabs/brave_tab_prefs.h"
@@ -193,3 +194,20 @@ void BraveTab::UpdateShadowForActiveTab() {
     DestroyLayer();
   }
 }
+
+void BraveTab::SetData(TabRendererData data) {
+  const bool data_changed = data != data_;
+  Tab::SetData(std::move(data));
+
+  // Our vertical tab uses CompoundTabContainer.
+  // When tab is moved from the group by pinning, it's moved to
+  // pinned TabContainerImpl before its tab group id is cleared.
+  // And it causes runtime crash as using this tab from pinned TabContainerImpl
+  // has assumption that it's not included in any group.
+  // So, clear in-advance when tab enters to pinned TabContainerImpl.
+  if (data_changed &&
+      tabs::utils::ShouldShowVerticalTabs(controller()->GetBrowser()) &&
+      data_.pinned) {
+    set_group(std::nullopt);
+  }
+}

browser/ui/views/tabs/brave_tab.h

@@ -40,6 +40,7 @@ class BraveTab : public Tab {
   void MaybeAdjustLeftForPinnedTab(gfx::Rect* bounds,
                                    int visual_width) const override;
   gfx::Insets GetInsets() const override;
+  void SetData(TabRendererData data) override;
 
  private:
   friend class BraveTabTest;

browser/ui/views/tabs/vertical_tab_strip_browsertest.cc

@@ -138,9 +138,23 @@ class VerticalTabStripBrowserTest : public InProcessBrowserTest {
     browser_non_client_frame_view()->DeprecatedLayoutImmediately();
   }
 
+  void AppendTab(Browser* browser) {
+    chrome::AddTabAt(browser, GURL(), -1, true);
+  }
+
+  tab_groups::TabGroupId AddTabToNewGroup(Browser* browser, int tab_index) {
+    return browser->tab_strip_model()->AddToNewGroup({tab_index});
+  }
+
+  void AddTabToExistingGroup(Browser* browser,
+                             int tab_index,
+                             tab_groups::TabGroupId group) {
+    ASSERT_TRUE(browser->tab_strip_model()->SupportsTabGroups());
+    browser->tab_strip_model()->AddToExistingGroup({tab_index}, group);
+  }
+
   TabStrip* GetTabStrip(Browser* browser) {
-    BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser);
-    return browser_view->tabstrip();
+    return BrowserView::GetBrowserViewForBrowser(browser)->tabstrip();
   }
 
   Tab* GetTabAt(Browser* browser, int index) {
@@ -301,7 +315,7 @@ IN_PROC_BROWSER_TEST_F(VerticalTabStripBrowserTest, MinHeight) {
   ToggleVerticalTabStrip();
 
   // Add a tab to flush cached min size.
-  chrome::AddTabAt(browser(), {}, -1, true);
+  AppendTab(browser());
 
   const auto browser_view_min_size = browser_view()->GetMinimumSize();
   const auto browser_non_client_frame_view_min_size =
@@ -311,7 +325,7 @@ IN_PROC_BROWSER_TEST_F(VerticalTabStripBrowserTest, MinHeight) {
   auto tab_strip_min_height =
       browser_view()->tab_strip_region_view()->GetMinimumSize().height();
   for (int i = 0; i < 10; i++) {
-    chrome::AddTabAt(browser(), {}, -1, true);
+    AppendTab(browser());
   }
   ASSERT_LE(tab_strip_min_height,
             browser_view()->tab_strip_region_view()->GetMinimumSize().height());
@@ -463,7 +477,7 @@ IN_PROC_BROWSER_TEST_F(VerticalTabStripBrowserTest, LayoutSanity) {
 
   ToggleVerticalTabStrip();
 
-  chrome::AddTabAt(browser(), {}, -1, true);
+  AppendTab(browser());
 
   auto* widget_delegate_view =
       browser_view()->vertical_tab_strip_widget_delegate_view();
@@ -741,13 +755,23 @@ IN_PROC_BROWSER_TEST_F(VerticalTabStripBrowserTest, PinningGroupedTab) {
   // Regression check for https://github.com/brave/brave-browser/issues/40201
   ToggleVerticalTabStrip();
 
-  // Given that we have a grouped tab
-  browser()->tab_strip_model()->ExecuteContextMenuCommand(
-      0, TabStripModel::CommandToggleGrouped);
+  AppendTab(browser());
+  AppendTab(browser());
+  AppendTab(browser());
+
+  tab_groups::TabGroupId group = AddTabToNewGroup(browser(), 0);
+  AddTabToExistingGroup(browser(), 1, group);
+  AddTabToExistingGroup(browser(), 2, group);
+  AddTabToExistingGroup(browser(), 3, group);
+
+  browser()->tab_strip_model()->SetTabPinned(1, true);
+  EXPECT_EQ(GetTabStrip(browser())->tab_at(0)->group(), std::nullopt);
+
+  browser()->tab_strip_model()->SetTabPinned(2, true);
+  EXPECT_EQ(GetTabStrip(browser())->tab_at(1)->group(), std::nullopt);
 
-  // When pinning the tab shouldn't cause crash.
-  browser()->tab_strip_model()->ExecuteContextMenuCommand(
-      0, TabStripModel::CommandTogglePinned);
+  EXPECT_EQ(GetTabStrip(browser())->tab_at(2)->group().value(), group);
+  EXPECT_EQ(GetTabStrip(browser())->tab_at(3)->group().value(), group);
 }
 
 class VerticalTabStripDragAndDropBrowserTest
@@ -840,7 +864,7 @@ class VerticalTabStripDragAndDropBrowserTest
 IN_PROC_BROWSER_TEST_F(VerticalTabStripDragAndDropBrowserTest,
                        MAYBE_DragTabToReorder) {
   // Pre-conditions ------------------------------------------------------------
-  chrome::AddTabAt(browser(), {}, -1, true);
+  AppendTab(browser());
 
   auto* widget_delegate_view =
       browser_view()->vertical_tab_strip_widget_delegate_view_.get();
@@ -895,7 +919,7 @@ IN_PROC_BROWSER_TEST_F(VerticalTabStripDragAndDropBrowserTest,
 IN_PROC_BROWSER_TEST_F(VerticalTabStripDragAndDropBrowserTest,
                        MAYBE_DragTabToDetach) {
   // Pre-conditions ------------------------------------------------------------
-  chrome::AddTabAt(browser(), {}, -1, true);
+  AppendTab(browser());
 
   // Drag a tab out of tab strip to create browser -----------------------------
   GetTabStrip(browser())->StopAnimating(

chromium_src/chrome/browser/ui/tabs/tab_strip_model.h

@@ -10,10 +10,8 @@
 #define TAB_STRIP_MODEL_H_ friend class BraveTabStripModel;
 #define IsReadLaterSupportedForAny virtual IsReadLaterSupportedForAny
 #define TabDragController TabDragControllerChromium
-#define ExecuteContextMenuCommand virtual ExecuteContextMenuCommand
 
 #include "src/chrome/browser/ui/tabs/tab_strip_model.h"  // IWYU pragma: export
-#undef ExecuteContextMenuCommand
 #undef IsReadLaterSupportedForAny
 #undef SelectRelativeTab
 #undef TAB_STRIP_MODEL_H_

chromium_src/chrome/browser/ui/views/tabs/tab.h

@@ -18,6 +18,7 @@ class BraveTab;
 #define GetWidthOfLargestSelectableRegion \
   virtual GetWidthOfLargestSelectableRegion
 
+#define SetData virtual SetData
 #define ActiveStateChanged virtual ActiveStateChanged
 #define GetGroupColor virtual GetGroupColor
 #define UpdateIconVisibility virtual UpdateIconVisibility
@@ -33,5 +34,6 @@ class BraveTab;
 #undef ActiveStateChanged
 #undef GetWidthOfLargestSelectableRegion
 #undef kMinimumContentsWidthForCloseButtons
+#undef SetData
 
 #endif  // BRAVE_CHROMIUM_SRC_CHROME_BROWSER_UI_VIEWS_TABS_TAB_H_