Regenerate ideal bounds of tiny tabs

We have to regenerate ideal bounds when it's updated even when
it's animating.

Bug: 587688
Change-Id: I238808c6242734e1c3749a6dd7bd624f6a8a3bac
Test: TabStripBrowserTest.ActiveTabWidthWhenTabsAreTiny
Reviewed-on: https://chromium-review.googlesource.com/958382
Commit-Queue: SangWoo Ko <sangwoo108@gmail.com>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546007}

chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.cc

@@ -18,7 +18,7 @@ void FakeBaseTabStripController::AddTab(int index, bool is_active) {
   num_tabs_++;
   tab_strip_->AddTabAt(index, TabRendererData(), is_active);
   if (is_active)
-    active_index_ = index;
+    SelectTab(index);
 }
 
 void FakeBaseTabStripController::AddPinnedTab(int index, bool is_active) {
@@ -33,8 +33,11 @@ void FakeBaseTabStripController::AddPinnedTab(int index, bool is_active) {
 void FakeBaseTabStripController::RemoveTab(int index) {
   num_tabs_--;
   tab_strip_->RemoveTabAt(nullptr, index);
-  if (active_index_ == index)
-    active_index_ = -1;
+  if (active_index_ > index) {
+    --active_index_;
+  } else if (active_index_ == index) {
+    SetActiveIndex(std::min(active_index_, num_tabs_ - 1));
+  }
 }
 
 const ui::ListSelectionModel&
@@ -71,11 +74,8 @@ bool FakeBaseTabStripController::IsTabPinned(int index) const {
 void FakeBaseTabStripController::SelectTab(int index) {
   if (!IsValidIndex(index) || active_index_ == index)
     return;
-  ui::ListSelectionModel old_selection_model;
-  old_selection_model.SetSelectedIndex(active_index_);
-  active_index_ = index;
-  selection_model_.SetSelectedIndex(active_index_);
-  tab_strip_->SetSelection(old_selection_model, selection_model_);
+
+  SetActiveIndex(index);
 }
 
 void FakeBaseTabStripController::ExtendSelectionTo(int index) {
@@ -88,6 +88,8 @@ void FakeBaseTabStripController::AddSelectionFromAnchorTo(int index) {
 }
 
 void FakeBaseTabStripController::CloseTab(int index, CloseTabSource source) {
+  tab_strip_->PrepareForCloseAt(index, source);
+  RemoveTab(index);
 }
 
 void FakeBaseTabStripController::ToggleTabAudioMute(int index) {
@@ -117,6 +119,7 @@ bool FakeBaseTabStripController::IsCompatibleWith(TabStrip* other) const {
 }
 
 void FakeBaseTabStripController::CreateNewTab() {
+  AddTab(num_tabs_, true);
 }
 
 void FakeBaseTabStripController::CreateNewTabWithLocation(
@@ -152,3 +155,12 @@ base::string16 FakeBaseTabStripController::GetAccessibleTabName(
 Profile* FakeBaseTabStripController::GetProfile() const {
   return nullptr;
 }
+
+void FakeBaseTabStripController::SetActiveIndex(int new_index) {
+  ui::ListSelectionModel old_selection_model;
+  old_selection_model.SetSelectedIndex(active_index_);
+  active_index_ = new_index;
+  selection_model_.SetSelectedIndex(active_index_);
+  if (IsValidIndex(active_index_))
+    tab_strip_->SetSelection(old_selection_model, selection_model_);
+}

chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.h

@@ -56,6 +56,8 @@ class FakeBaseTabStripController : public TabStripController {
   Profile* GetProfile() const override;
 
  private:
+  void SetActiveIndex(int new_index);
+
   TabStrip* tab_strip_ = nullptr;
 
   int num_tabs_ = 0;

chrome/browser/ui/views/tabs/tab_strip.cc

@@ -115,6 +115,8 @@ const int kPinnedToNonPinnedOffset = 2;
 const int kPinnedToNonPinnedOffset = 3;
 #endif
 
+TabSizeInfo* g_tab_size_info = nullptr;
+
 // Returns the width needed for the new tab button (and padding).
 int GetNewTabButtonWidth(bool is_incognito) {
   return GetLayoutSize(NEW_TAB_BUTTON, is_incognito).width() +
@@ -213,18 +215,17 @@ TabDragController::EventSource EventSourceFromEvent(
 }
 
 const TabSizeInfo& GetTabSizeInfo() {
-  static TabSizeInfo* tab_size_info = nullptr;
-  if (tab_size_info)
-    return *tab_size_info;
+  if (g_tab_size_info)
+    return *g_tab_size_info;
 
-  tab_size_info = new TabSizeInfo;
-  tab_size_info->pinned_tab_width = Tab::GetPinnedWidth();
-  tab_size_info->min_active_width = Tab::GetMinimumActiveSize().width();
-  tab_size_info->min_inactive_width = Tab::GetMinimumInactiveSize().width();
-  tab_size_info->max_size = Tab::GetStandardSize();
-  tab_size_info->tab_overlap = Tab::GetOverlap();
-  tab_size_info->pinned_to_normal_offset = kPinnedToNonPinnedOffset;
-  return *tab_size_info;
+  g_tab_size_info = new TabSizeInfo;
+  g_tab_size_info->pinned_tab_width = Tab::GetPinnedWidth();
+  g_tab_size_info->min_active_width = Tab::GetMinimumActiveSize().width();
+  g_tab_size_info->min_inactive_width = Tab::GetMinimumInactiveSize().width();
+  g_tab_size_info->max_size = Tab::GetStandardSize();
+  g_tab_size_info->tab_overlap = Tab::GetOverlap();
+  g_tab_size_info->pinned_to_normal_offset = kPinnedToNonPinnedOffset;
+  return *g_tab_size_info;
 }
 
 }  // namespace
@@ -654,13 +655,24 @@ void TabStrip::SetSelection(const ui::ListSelectionModel& old_selection,
       AnimateToIdealBounds();
     SchedulePaint();
   } else {
-    // We have "tiny tabs" if the tabs are so tiny that the unselected ones are
-    // a different size to the selected ones.
-    bool tiny_tabs = current_inactive_width_ != current_active_width_;
-    if (!IsAnimating() && (!in_tab_close_ || tiny_tabs)) {
-      DoLayout();
-    } else {
+    if (current_inactive_width_ == current_active_width_) {
+      // When tabs are wide enough, selecting a new tab cannot change the
+      // ideal bounds, so only a repaint is necessary.
       SchedulePaint();
+    } else if (IsAnimating()) {
+      // The selection change will have modified the ideal bounds of the tabs
+      // in |old_selection| and |new_selection|.  We need to recompute.
+      // Note: This is safe even if we're in the midst of mouse-based tab
+      // closure--we won't expand the tabstrip back to the full window
+      // width--because PrepareForCloseAt() will have set
+      // |available_width_for_tabs_| already.
+      GenerateIdealBounds();
+      AnimateToIdealBounds();
+    } else {
+      // As in the animating case above, the selection change will have
+      // affected the desired bounds of the tabs, but since we're not animating
+      // we can just snap to the new bounds.
+      DoLayout();
     }
   }
 
@@ -2206,6 +2218,14 @@ int TabStrip::GetStartXForNormalTabs() const {
          kPinnedToNonPinnedOffset;
 }
 
+// static
+void TabStrip::ResetTabSizeInfoForTesting() {
+  if (g_tab_size_info) {
+    delete g_tab_size_info;
+    g_tab_size_info = nullptr;
+  }
+}
+
 Tab* TabStrip::FindTabForEvent(const gfx::Point& point) {
   DCHECK(touch_layout_);
   int active_tab_index = touch_layout_->active_index();

chrome/browser/ui/views/tabs/tab_strip.h

@@ -284,6 +284,7 @@ class TabStrip : public views::View,
   FRIEND_TEST_ALL_PREFIXES(TabDragControllerTest, GestureEndShouldEndDragTest);
   FRIEND_TEST_ALL_PREFIXES(TabStripTest, TabForEventWhenStacked);
   FRIEND_TEST_ALL_PREFIXES(TabStripTest, TabCloseButtonVisibilityWhenStacked);
+  FRIEND_TEST_ALL_PREFIXES(TabStripTest, ActiveTabWidthWhenTabsAreTiny);
 
   // Used during a drop session of a url. Tracks the position of the drop as
   // well as a window used to highlight where the drop occurs.
@@ -519,6 +520,10 @@ class TabStrip : public views::View,
   // hit-test region of the specified Tab.
   bool IsPointInTab(Tab* tab, const gfx::Point& point_in_tabstrip_coords);
 
+  // Reset cached tab size info. Because Tab size info can be different
+  // depending on touch ui optimization, we should be able to reset this.
+  static void ResetTabSizeInfoForTesting();
+
   // -- Touch Layout ----------------------------------------------------------
 
   // Returns the position normal tabs start at.

chrome/browser/ui/views/tabs/tab_strip_impl_unittest.cc → chrome/browser/ui/views/tabs/tab_strip_unittest.cc

@@ -13,7 +13,6 @@
 #include "chrome/browser/ui/views/tabs/tab.h"
 #include "chrome/browser/ui/views/tabs/tab_icon.h"
 #include "chrome/browser/ui/views/tabs/tab_renderer_data.h"
-#include "chrome/browser/ui/views/tabs/tab_strip.h"
 #include "chrome/browser/ui/views/tabs/tab_strip_controller.h"
 #include "chrome/browser/ui/views/tabs/tab_strip_observer.h"
 #include "chrome/test/base/testing_profile.h"
@@ -131,6 +130,7 @@ class TabStripTest : public views::ViewsTestBase,
   }
 
   void TearDown() override {
+    TabStrip::ResetTabSizeInfoForTesting();
     widget_.reset();
     views::ViewsTestBase::TearDown();
   }
@@ -565,6 +565,41 @@ TEST_P(TabStripTest, AttentionIndicatorHidesOnSelect) {
   EXPECT_FALSE(IsShowingAttentionIndicator(1));
 }
 
+// The active tab should always be at least as wide as its minimum width.
+// http://crbug.com/587688
+TEST_P(TabStripTest, ActiveTabWidthWhenTabsAreTiny) {
+  // The bug was caused when it's animating. Therefore we should make widget
+  // visible so that animation can be triggered.
+  tab_strip_->GetWidget()->Show();
+  tab_strip_->SetBounds(0, 0, 200, 20);
+
+  const int min_inactive_width = Tab::GetMinimumInactiveSize().width();
+  // Create a lot of tabs in order to make inactive tabs tiny.
+  while (tab_strip_->current_inactive_width_ != min_inactive_width)
+    controller_->CreateNewTab();
+
+  const int min_active_width = Tab::GetMinimumActiveSize().width();
+  int active_index = controller_->GetActiveIndex();
+  EXPECT_GT(tab_strip_->tab_count(), 1);
+  EXPECT_EQ(tab_strip_->tab_count() - 1, active_index);
+  EXPECT_LT(tab_strip_->ideal_bounds(0).width(),
+            tab_strip_->ideal_bounds(active_index).width());
+
+  // Even though other tabs are very tiny, the active tab should be at least
+  // as wide as it's minimum width.
+  EXPECT_GE(tab_strip_->ideal_bounds(active_index).width(), min_active_width);
+
+  // During mouse-based tab closure, the active tab should remain at least as
+  // wide as it's minium width.
+  controller_->SelectTab(0);
+  while (tab_strip_->tab_count()) {
+    const int active_index = controller_->GetActiveIndex();
+    EXPECT_GE(tab_strip_->ideal_bounds(active_index).width(), min_active_width);
+    tab_strip_->CloseTab(tab_strip_->tab_at(active_index),
+                         CLOSE_TAB_FROM_MOUSE);
+  }
+}
+
 // Defines an alias to be used for tests that are only relevant to the touch-
 // optimized UI mode.
 using TabStripTouchOptimizedUiOnlyTest = TabStripTest;

chrome/test/BUILD.gn

@@ -4259,8 +4259,8 @@ test("unit_tests") {
         "../browser/ui/views/tabs/fake_base_tab_strip_controller.cc",
         "../browser/ui/views/tabs/fake_base_tab_strip_controller.h",
         "../browser/ui/views/tabs/stacked_tab_strip_layout_unittest.cc",
-        "../browser/ui/views/tabs/tab_strip_impl_unittest.cc",
         "../browser/ui/views/tabs/tab_strip_layout_unittest.cc",
+        "../browser/ui/views/tabs/tab_strip_unittest.cc",
         "../browser/ui/views/tabs/tab_unittest.cc",
         "../browser/ui/views/toolbar/reload_button_unittest.cc",
         "../browser/ui/views/toolbar/toolbar_action_view_unittest.cc",