Remove TabStrip::PrepareForCloseAt()

When removing tabs with mouse button, we'd like to know
the next active tab so that we can tailor available_width_for_tabs_.
But as PrepareForCloseAt() is called before the next tab is decided,
we don't know that. So move this logic to RemoveTabAt() where we
know it.
 And move other logic in PrepareCloseTabAt() is moved to CloseTab(),
 which triggered PrepareForCloseAt() via TabStripController::CloseTab().
As TabStripController::CloseTab() was invoked only by
TabStrip::CloseTab(), it's okay to move them to TabStrip::CloseTab().

Bug: 856289
Change-Id: I5eb7036da4214e92a218f20e8c09891ab9a11955
Reviewed-on: https://chromium-review.googlesource.com/1119616
Commit-Queue: Sang Woo Ko <sangwoo108@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572566}

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

@@ -267,7 +267,6 @@ void BrowserTabStripController::CloseTab(int model_index,
   // Cancel any pending tab transition.
   hover_tab_selector_.CancelTabTransition();
 
-  tabstrip_->PrepareForCloseAt(model_index, source);
   model_->CloseWebContentsAt(model_index,
                              TabStripModel::CLOSE_USER_GESTURE |
                              TabStripModel::CLOSE_CREATE_HISTORICAL_TAB);
@@ -427,7 +426,7 @@ void BrowserTabStripController::TabDetachedAt(WebContents* contents,
   // Cancel any pending tab transition.
   hover_tab_selector_.CancelTabTransition();
 
-  tabstrip_->RemoveTabAt(contents, model_index);
+  tabstrip_->RemoveTabAt(contents, model_index, was_active);
 }
 
 void BrowserTabStripController::ActiveTabChanged(

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

@@ -33,12 +33,14 @@ void FakeBaseTabStripController::AddPinnedTab(int index, bool is_active) {
 
 void FakeBaseTabStripController::RemoveTab(int index) {
   num_tabs_--;
-  tab_strip_->RemoveTabAt(nullptr, index);
+  // RemoveTabAt() expects the controller state to have been updated already.
+  const bool was_active = index == active_index_;
   if (active_index_ > index) {
     --active_index_;
   } else if (active_index_ == index) {
     SetActiveIndex(std::min(active_index_, num_tabs_ - 1));
   }
+  tab_strip_->RemoveTabAt(nullptr, index, was_active);
 }
 
 const ui::ListSelectionModel&
@@ -89,7 +91,6 @@ void FakeBaseTabStripController::AddSelectionFromAnchorTo(int index) {
 }
 
 void FakeBaseTabStripController::CloseTab(int index, CloseTabSource source) {
-  tab_strip_->PrepareForCloseAt(index, source);
   RemoveTab(index);
 }
 

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

@@ -545,7 +545,44 @@ void TabStrip::MoveTab(int from_model_index,
     observer.OnTabMoved(from_model_index, to_model_index);
 }
 
-void TabStrip::RemoveTabAt(content::WebContents* contents, int model_index) {
+void TabStrip::RemoveTabAt(content::WebContents* contents,
+                           int model_index,
+                           bool was_active) {
+  const int model_count = GetModelCount();
+  if (in_tab_close_ && model_count > 0 && model_index != model_count) {
+    // The user closed a tab other than the last tab. Set
+    // available_width_for_tabs_ so that as the user closes tabs with the mouse
+    // a tab continues to fall under the mouse.
+    int next_active_index = controller_->GetActiveIndex();
+    DCHECK(IsValidModelIndex(next_active_index));
+    if (model_index <= next_active_index) {
+      // At this point, model's internal state has already been updated.
+      // |contents| has been detached from model and the active index has been
+      // updated. But the tab for |contents| isn't removed yet. Thus, we need to
+      // fix up next_active_index based on it.
+      next_active_index++;
+    }
+    Tab* next_active_tab = tab_at(next_active_index);
+    Tab* tab_being_removed = tab_at(model_index);
+
+    int size_delta = tab_being_removed->width();
+    if (!tab_being_removed->data().pinned && was_active &&
+        current_active_width_ > current_inactive_width_) {
+      // When removing an active, non-pinned tab, an inactive tab will be made
+      // active and thus given the active width. Thus the width being removed
+      // from the strip is really the current width of whichever inactive tab
+      // will be made active.
+      size_delta = next_active_tab->width();
+    }
+
+    available_width_for_tabs_ = ideal_bounds(model_count).right() -
+                                TabStartX() - size_delta + Tab::GetOverlap();
+    if (model_index == 0 && tab_being_removed->data().pinned &&
+        !tab_at(1)->data().pinned) {
+      available_width_for_tabs_ -= GetPinnedToNonPinnedOffset();
+    }
+  }
+
   const bool first_unpinned_tab = model_index == GetPinnedTabCount();
 
   if (!touch_layout_)
@@ -696,54 +733,6 @@ bool TabStrip::ShouldTabBeVisible(const Tab* tab) const {
          tabstrip_right;
 }
 
-void TabStrip::PrepareForCloseAt(int model_index, CloseTabSource source) {
-  if (!in_tab_close_ && IsAnimating()) {
-    // Cancel any current animations. We do this as remove uses the current
-    // ideal bounds and we need to know ideal bounds is in a good state.
-    StopAnimating(true);
-  }
-
-  if (!GetWidget())
-    return;
-
-  int model_count = GetModelCount();
-  if (model_count > 1 && model_index != model_count - 1) {
-    // The user is about to close a tab other than the last tab. Set
-    // available_width_for_tabs_ so that as the user closes tabs with the mouse
-    // a tab continues to fall under the mouse.
-    Tab* tab_being_removed = tab_at(model_index);
-    int size_delta = tab_being_removed->width();
-    if (!tab_being_removed->data().pinned && tab_being_removed->IsActive()) {
-      // When removing an active, non-pinned tab, an inactive tab will be made
-      // active and thus given the active width. Thus the width being removed
-      // from the strip is really the current width of whichever inactive tab
-      // will be made active. This could be either |current_inactive_width_| or
-      // (current_inactive_width_ + 1) (if layout has apportioned extra space to
-      // the initial tabs in the strip). Unfortunately, the next active tab has
-      // not yet been chosen, so it's impossible to know which of these is
-      // correct; using the narrower value is more conservative.
-      // TODO(sky): https://crbug.com/856289 Refactor so this happens after
-      // another tab has been activated.
-      size_delta = current_inactive_width_;
-    }
-
-    available_width_for_tabs_ = ideal_bounds(model_count - 1).right() -
-                                TabStartX() - size_delta + Tab::GetOverlap();
-    if (model_index == 0 && tab_being_removed->data().pinned &&
-        !tab_at(1)->data().pinned) {
-      available_width_for_tabs_ -= GetPinnedToNonPinnedOffset();
-    }
-  }
-
-  in_tab_close_ = true;
-  resize_layout_timer_.Stop();
-  if (source == CLOSE_TAB_FROM_TOUCH) {
-    StartResizeLayoutTabsFromTouchTimer();
-  } else {
-    AddMessageLoopObserver();
-  }
-}
-
 void TabStrip::SetSelection(const ui::ListSelectionModel& new_selection) {
   if (selected_tabs_.active() != new_selection.active()) {
     if (selected_tabs_.active() >= 0)
@@ -923,18 +912,33 @@ void TabStrip::AddSelectionFromAnchorTo(Tab* tab) {
 }
 
 void TabStrip::CloseTab(Tab* tab, CloseTabSource source) {
+  int model_index = GetModelIndexOfTab(tab);
   if (tab->closing()) {
     // If the tab is already closing, close the next tab. We do this so that the
     // user can rapidly close tabs by clicking the close button and not have
     // the animations interfere with that.
-    const int closed_tab_index = FindClosingTab(tab).first->first;
-    if (closed_tab_index < GetModelCount())
-      controller_->CloseTab(closed_tab_index, source);
+    model_index = FindClosingTab(tab).first->first;
+  }
+
+  if (!IsValidModelIndex(model_index))
     return;
+
+  if (!in_tab_close_ && IsAnimating()) {
+    // Cancel any current animations. We do this as remove uses the current
+    // ideal bounds and we need to know ideal bounds is in a good state.
+    StopAnimating(true);
   }
-  int model_index = GetModelIndexOfTab(tab);
-  if (IsValidModelIndex(model_index))
-    controller_->CloseTab(model_index, source);
+
+  if (GetWidget()) {
+    in_tab_close_ = true;
+    resize_layout_timer_.Stop();
+    if (source == CLOSE_TAB_FROM_TOUCH)
+      StartResizeLayoutTabsFromTouchTimer();
+    else
+      AddMessageLoopObserver();
+  }
+
+  controller_->CloseTab(model_index, source);
 }
 
 void TabStrip::ToggleTabAudioMute(Tab* tab) {

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

@@ -155,7 +155,9 @@ class TabStrip : public views::View,
 
   // Removes a tab at the specified index. If the tab with |contents| is being
   // dragged then the drag is completed.
-  void RemoveTabAt(content::WebContents* contents, int model_index);
+  void RemoveTabAt(content::WebContents* contents,
+                   int model_index,
+                   bool was_active);
 
   // Sets the tab data at the specified model index.
   void SetTabData(int model_index, TabRendererData data);
@@ -167,12 +169,6 @@ class TabStrip : public views::View,
   // to clip).
   bool ShouldTabBeVisible(const Tab* tab) const;
 
-  // Invoked from the controller when the close initiates from the TabController
-  // (the user clicked the tab close button or middle clicked the tab). This is
-  // invoked from Close. Because of unload handlers Close is not always
-  // immediately followed by RemoveTabAt.
-  void PrepareForCloseAt(int model_index, CloseTabSource source);
-
   // Invoked when the selection is updated.
   void SetSelection(const ui::ListSelectionModel& new_selection);
 

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

@@ -252,7 +252,7 @@ TEST_P(TabStripTest, GetModelCount) {
 }
 
 TEST_P(TabStripTest, AccessibilityEvents) {
-  // When adding tabs, SetSelection() is called after RemoveTabAt(), as
+  // When adding tabs, SetSelection() is called after AddTabAt(), as
   // otherwise the index would not be meaningful.
   tab_strip_->AddTabAt(0, TabRendererData(), false);
   tab_strip_->AddTabAt(1, TabRendererData(), true);
@@ -266,7 +266,7 @@ TEST_P(TabStripTest, AccessibilityEvents) {
   // otherwise the index would not be meaningful.
   selection.SetSelectedIndex(0);
   tab_strip_->SetSelection(selection);
-  tab_strip_->RemoveTabAt(nullptr, 1);
+  tab_strip_->RemoveTabAt(nullptr, 1, true);
   EXPECT_EQ(2, test_views_delegate_->add_count());
   EXPECT_EQ(1, test_views_delegate_->remove_count());
 }