Revert "Reland "[Tab Scrolling] Remove scroll button flag and put behind scrolling flag.""

This reverts commit 8c2b070486.

Reason for revert: https://bugs.chromium.org/p/chromium/issues/detail?id=1203261

Original change's description:
> Reland "[Tab Scrolling] Remove scroll button flag and put behind scrolling flag."
>
> This reverts commit 2f815348c7.
>
> Reason for revert: Attempt to reland
>
> Original change's description:
> > Revert "[Tab Scrolling] Remove scroll button flag and put behind scrolling flag."
> >
> > This reverts commit fbbc174c7e.
> >
> > Reason for revert:
> >
> > Note: It is reported that sheriffs cannot submit CL created by Findit
> > (crbug.com/1187426). A workaround in the mean time is to abandon this
> > CL and create another revert CL.
> >
> > Findit (https://goo.gl/kROfz5) identified CL at revision 874365 as the
> > culprit for failures in the build cycles as shown on:
> > https://analysis.chromium.org/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtL2ZiYmMxNzRjN2U3NDY0MzU1ZmExODdmNjQ3ZWE4NTJiNDZjZTg0YzMM
> >
> > Sample Failed Build: https://ci.chromium.org/b/8849403113370884336
> >
> > Sample Failed Step: unit_tests
> >
> > Original change's description:
> > > [Tab Scrolling] Remove scroll button flag and put behind scrolling flag.
> > > 
> > > Bug: 1116118
> > > Change-Id: Ib2a933f776e176d417b3385bf0ac2be09f1f228b
> > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2796130
> > > Commit-Queue: Charlene Yan <cyan@chromium.org>
> > > Reviewed-by: Taylor Bergquist <tbergquist@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#874365}
> >
> >
> > Change-Id: Icd75dd66bcab4320417ba60d9b1b0bc713375064
> > No-Presubmit: true
> > No-Tree-Checks: true
> > No-Try: true
> > Bug: 1116118
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2842065
> > Reviewed-by: Lei Zhang <thestig@chromium.org>
> > Commit-Queue: Lei Zhang <thestig@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#874473}
>
> Bug: 1116118
> Change-Id: I9a876d90343d39a6ac91f265b05d99c84c0e33ec
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2846283
> Reviewed-by: Taylor Bergquist <tbergquist@chromium.org>
> Commit-Queue: Charlene Yan <cyan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#876338}

Bug: 1116118
Change-Id: Ib5e42a16a04b6261ec9a96fedeae8653168f7373
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2855535
Auto-Submit: Taylor Bergquist <tbergquist@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#876777}

chrome/browser/about_flags.cc

@@ -4616,6 +4616,11 @@ const FeatureEntry kFeatureEntries[] = {
                                     kTabScrollingVariations,
                                     "TabScrolling")},
 
+    {"scrollable-tabstrip-buttons",
+     flag_descriptions::kScrollableTabStripButtonsName,
+     flag_descriptions::kScrollableTabStripButtonsDescription, kOsDesktop,
+     FEATURE_VALUE_TYPE(features::kScrollableTabStripButtons)},
+
     {"side-panel", flag_descriptions::kSidePanelName,
      flag_descriptions::kSidePanelDescription, kOsDesktop,
      FEATURE_VALUE_TYPE(features::kSidePanel)},

chrome/browser/flag-metadata.json

@@ -4630,6 +4630,11 @@
     "owners": [ "chrome-desktop-ui-sea@google.com", "tbergquist" ],
     "expiry_milestone": 93
   },
+  {
+    "name": "scrollable-tabstrip-buttons",
+    "owners": [ "chrome-desktop-ui-sea@google.com", "tbergquist" ],
+    "expiry_milestone": 93
+  },
   {
     "name": "sct-auditing",
     "owners": [ "cthomp", "estark", "jdeblasio" ],

chrome/browser/flag_descriptions.cc

@@ -2166,6 +2166,11 @@ const char kScrollableTabStripName[] = "Tab Scrolling";
 const char kScrollableTabStripDescription[] =
     "Enables tab strip to scroll left and right when full.";
 
+const char kScrollableTabStripButtonsName[] = "Tab Scrolling Buttons";
+const char kScrollableTabStripButtonsDescription[] =
+    "When the scrollable-tabstrip flag is enabled, this enables buttons to "
+    "permanently appear on the tabstrip.";
+
 const char kScrollUnificationName[] = "Scroll Unification";
 const char kScrollUnificationDescription[] =
     "Refactoring project that eliminates scroll handling code from Blink. "

chrome/browser/flag_descriptions.h

@@ -1252,6 +1252,9 @@ extern const char kScrollableTabStripFlagId[];
 extern const char kScrollableTabStripName[];
 extern const char kScrollableTabStripDescription[];
 
+extern const char kScrollableTabStripButtonsName[];
+extern const char kScrollableTabStripButtonsDescription[];
+
 extern const char kScrollUnificationName[];
 extern const char kScrollUnificationDescription[];
 

chrome/browser/ui/ui_features.cc

@@ -59,6 +59,11 @@ const base::Feature kScrollableTabStrip{"ScrollableTabStrip",
                                         base::FEATURE_DISABLED_BY_DEFAULT};
 const char kMinimumTabWidthFeatureParameterName[] = "minTabWidth";
 
+// Enables buttons to permanently appear on the tabstrip when
+// scrollable-tabstrip is enabled. https://crbug.com/1116118
+const base::Feature kScrollableTabStripButtons{
+    "ScrollableTabStripButtons", base::FEATURE_DISABLED_BY_DEFAULT};
+
 // Hosts some content in a side panel. https://crbug.com/1149995
 const base::Feature kSidePanel{"SidePanel", base::FEATURE_DISABLED_BY_DEFAULT};
 

chrome/browser/ui/ui_features.h

@@ -46,6 +46,8 @@ extern const base::Feature kReadLaterNewBadgePromo;
 extern const base::Feature kScrollableTabStrip;
 extern const char kMinimumTabWidthFeatureParameterName[];
 
+extern const base::Feature kScrollableTabStripButtons;
+
 extern const base::Feature kSidePanel;
 
 extern const base::Feature kSidePanelPrototype;

chrome/browser/ui/views/frame/tab_strip_region_view.cc

@@ -194,18 +194,6 @@ TabStripRegionView::TabStripRegionView(std::unique_ptr<TabStrip> tab_strip) {
         views::kFlexBehaviorKey,
         views::FlexSpecification(base::BindRepeating(
             &TabScrollContainerFlexRule, base::Unretained(tab_strip_))));
-
-    leading_scroll_button_ = AddChildView(CreateScrollButton(
-        base::BindRepeating(&TabStripRegionView::ScrollTowardsLeadingTab,
-                            base::Unretained(this))));
-    trailing_scroll_button_ = AddChildView(CreateScrollButton(
-        base::BindRepeating(&TabStripRegionView::ScrollTowardsTrailingTab,
-                            base::Unretained(this))));
-
-    // The space in dips between the scroll buttons and the NTB.
-    constexpr int kScrollButtonsTrailingMargin = 8;
-    trailing_scroll_button_->SetProperty(
-        views::kMarginsKey, gfx::Insets(0, 0, 0, kScrollButtonsTrailingMargin));
   } else {
     tab_strip_container_ = AddChildView(std::move(tab_strip));
 
@@ -219,6 +207,20 @@ TabStripRegionView::TabStripRegionView(std::unique_ptr<TabStrip> tab_strip) {
                                       tab_strip_container_flex_spec);
   }
 
+  if (base::FeatureList::IsEnabled(features::kScrollableTabStripButtons)) {
+    leading_scroll_button_ = AddChildView(CreateScrollButton(
+        base::BindRepeating(&TabStripRegionView::ScrollTowardsLeadingTab,
+                            base::Unretained(this))));
+    trailing_scroll_button_ = AddChildView(CreateScrollButton(
+        base::BindRepeating(&TabStripRegionView::ScrollTowardsTrailingTab,
+                            base::Unretained(this))));
+
+    // The space in dips between the scroll buttons and the NTB.
+    constexpr int kScrollButtonsTrailingMargin = 8;
+    trailing_scroll_button_->SetProperty(
+        views::kMarginsKey, gfx::Insets(0, 0, 0, kScrollButtonsTrailingMargin));
+  }
+
   new_tab_button_ = AddChildView(std::make_unique<NewTabButton>(
       tab_strip_, base::BindRepeating(&TabStrip::NewTabButtonPressed,
                                       base::Unretained(tab_strip_))));
@@ -310,7 +312,7 @@ void TabStripRegionView::FrameColorsChanged() {
   new_tab_button_->FrameColorsChanged();
   if (tab_search_button_)
     tab_search_button_->FrameColorsChanged();
-  if (base::FeatureList::IsEnabled(features::kScrollableTabStrip)) {
+  if (base::FeatureList::IsEnabled(features::kScrollableTabStripButtons)) {
     const SkColor background_color = tab_strip_->GetTabBackgroundColor(
         TabActive::kInactive, BrowserFrameActiveState::kUseCurrent);
     SkColor foreground_color = tab_strip_->GetTabForegroundColor(
@@ -362,7 +364,7 @@ void TabStripRegionView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
 void TabStripRegionView::OnViewPreferredSizeChanged(View* view) {
   DCHECK_EQ(view, tab_strip_);
 
-  if (base::FeatureList::IsEnabled(features::kScrollableTabStrip))
+  if (base::FeatureList::IsEnabled(features::kScrollableTabStripButtons))
     UpdateScrollButtonVisibility();
 
   // The |tab_strip_|'s preferred size changing can change our own preferred
@@ -378,24 +380,28 @@ int TabStripRegionView::GetTabStripAvailableWidth() const {
   // sibling views. First ask for the available size of the container.
   views::SizeBound width_bound = GetAvailableSize(tab_strip_container_).width();
 
-  int tabstrip_available_width = width_bound.min_of(width());
-  // The scroll buttons should never prevent the tabstrip from being entirely
-  // visible (i.e. non-scrollable). In that sense, their layout space is always
-  // available for the tabstrip's use.
-  if (base::FeatureList::IsEnabled(features::kScrollableTabStrip) &&
-      leading_scroll_button_->GetVisible()) {
-    // The scroll button span has to be manually calculated since this occurs
-    // during layout and we cannot use the layout positions.
-    const int scroll_buttons_span =
-        leading_scroll_button_->width() + trailing_scroll_button_->width() +
-        trailing_scroll_button_->GetProperty(views::kMarginsKey)->right();
-    tabstrip_available_width += scroll_buttons_span;
-  }
-
   // Because we can't return a null value, and we can't return zero, for cases
   // where we have never been laid out we will return something arbitrary (the
   // width of the region view is as good a choice as any, as it's strictly
   // larger than the tabstrip should be able to display).
+  int tabstrip_available_width = width_bound.min_of(width());
+
+  // The scroll buttons should never prevent the tabstrip from being entirely
+  // visible (i.e. non-scrollable). In that sense, their layout space is always
+  // available for the tabstrip's use.
+  if (base::FeatureList::IsEnabled(features::kScrollableTabStripButtons) &&
+      leading_scroll_button_->GetVisible()) {
+    const int scroll_buttons_span =
+        new_tab_button_->x() - leading_scroll_button_->x();
+    // The NTB must immediately follow the scroll buttons for this approach
+    // to make sense. If these DCHECKS fail, we will need to revisit this
+    // assumption.
+    DCHECK_GT(scroll_buttons_span, 0);
+    DCHECK_EQ(GetIndexOf(trailing_scroll_button_) + 1,
+              GetIndexOf(new_tab_button_));
+    tabstrip_available_width += scroll_buttons_span;
+  }
+
   return tabstrip_available_width;
 }
 
@@ -442,10 +448,11 @@ void TabStripRegionView::UpdateNewTabButtonBorder() {
 }
 
 void TabStripRegionView::UpdateScrollButtonVisibility() {
-  DCHECK(base::FeatureList::IsEnabled(features::kScrollableTabStrip));
+  DCHECK(base::FeatureList::IsEnabled(features::kScrollableTabStripButtons));
   // Make the scroll buttons visible only if the tabstrip can be scrolled.
   bool is_scrollable =
       tab_strip_->GetMinimumSize().width() > GetTabStripAvailableWidth();
+
   leading_scroll_button_->SetVisible(is_scrollable);
   trailing_scroll_button_->SetVisible(is_scrollable);
 }

chrome/browser/ui/views/frame/tab_strip_region_view.h

@@ -44,8 +44,6 @@ class TabStripRegionView final : public views::AccessiblePaneView,
 
   NewTabButton* new_tab_button() { return new_tab_button_; }
 
-  views::View* leading_scroll_button() { return leading_scroll_button_; }
-
   TabSearchButton* tab_search_button() { return tab_search_button_; }
 
   TipMarqueeView* tip_marquee_view() { return tip_marquee_view_; }

chrome/browser/ui/views/frame/tab_strip_region_view_unittest.cc

@@ -62,9 +62,6 @@ class TabStripRegionViewTestBase : public ChromeViewsTestBase {
   }
 
   void TearDown() override {
-    // TODO(crbug.com/1202946): Ensure the tabstrip is not animating during tear
-    // down which causes undefined behavior.
-    tab_strip_->StopAnimating(true);
     widget_.reset();
     ChromeViewsTestBase::TearDown();
   }
@@ -267,27 +264,6 @@ TEST_F(TabStripRegionViewTestWithScrollingEnabled,
       tab_strip_->tab_at(tab_strip_->GetModelCount() - 1)->GetVisible());
 }
 
-// When scrolling is enabled, adding enough tabs will cause the tabstrip to
-// enter a scroll state with visible scroll controls. When the last tab that
-// caused the tabstrip to enter the scroll state is removed, the tabstrip should
-// return to a non-scrolling state.
-TEST_F(TabStripRegionViewTestWithScrollingEnabled,
-       TabStripEntersAndExitsScrolling) {
-  // const int minimum_active_width = TabStyleViews::GetMinimumInactiveWidth();
-  controller_->AddTab(0, true);
-  CompleteAnimationAndLayout();
-
-  // Add tabs to the tabstrip until it is full and should start overflowing.
-  while (!tab_strip_region_view_->leading_scroll_button()->GetVisible()) {
-    controller_->AddTab(0, false);
-    CompleteAnimationAndLayout();
-  }
-  EXPECT_TRUE(tab_strip_region_view_->leading_scroll_button()->GetVisible());
-  controller_->CloseTab(0);
-  CompleteAnimationAndLayout();
-  EXPECT_FALSE(tab_strip_region_view_->leading_scroll_button()->GetVisible());
-}
-
 INSTANTIATE_TEST_SUITE_P(All,
                          TabStripRegionViewTest,
                          ::testing::Values(true, false));