Track closing tabs in layout_helper_ instead of tabs_closing_map_.

This requires notifying layout_helper_ when a tab is being destroyed,
and TabStripLayoutHelper to step carefully around closing tabs in a few
places, but saves more complexity by not duplicating state between
TabStrip and TabStripLayoutHelper.

Bug: 958173
Change-Id: Iaeafc2391ecac0397947ccc34ddaecf853feb787
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1725271
Commit-Queue: Taylor Bergquist <tbergquist@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683380}

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

@@ -298,11 +298,6 @@ class TabStrip : public views::AccessiblePaneView,
   void HandleDragExited() override;
 
  private:
-  using Tabs = std::vector<Tab*>;
-  using TabsClosingMap = std::map<int, Tabs>;
-  using FindClosingTabResult =
-      std::pair<TabsClosingMap::iterator, Tabs::iterator>;
-
   class RemoveTabDelegate;
   class TabDragContextImpl;
 
@@ -403,8 +398,7 @@ class TabStrip : public views::AccessiblePaneView,
   // the actual last tab unless the strip is in the overflow node_data.
   const Tab* GetLastVisibleTab() const;
 
-  // Adds the tab at |index| to |tabs_closing_map_| and removes the tab from
-  // |tabs_|.
+  // Removes the tab at |index| from |tabs_|.
   void RemoveTabFromViewModel(int index);
 
   // Cleans up the Tab from the TabStrip. This is called from the tab animation
@@ -415,18 +409,10 @@ class TabStrip : public views::AccessiblePaneView,
   // from the tab animation code and is not a general-purpose method.
   void OnGroupCloseAnimationCompleted(TabGroupId group);
 
-  // Adjusts the indices of all tabs in |tabs_closing_map_| whose index is
-  // >= |index| to have a new index of |index + delta|.
-  void UpdateTabsClosingMap(int index, int delta);
-
   // Invoked from StoppedDraggingTabs to cleanup |tab|. If |tab| is known
   // |is_first_tab| is set to true.
   void StoppedDraggingTab(Tab* tab, bool* is_first_tab);
 
-  // Finds |tab| in the |tab_closing_map_| and returns a pair of iterators
-  // indicating precisely where it is.
-  FindClosingTabResult FindClosingTab(const Tab* tab);
-
   // Invoked when a mouse event occurs over |source|. Potentially switches the
   // |stacked_layout_|.
   void UpdateStackedLayoutFromMouseEvent(views::View* source,
@@ -563,12 +549,9 @@ class TabStrip : public views::AccessiblePaneView,
   // There is a one-to-one mapping between each of the tabs in the
   // TabStripController (TabStripModel) and |tabs_|. Because we animate tab
   // removal there exists a period of time where a tab is displayed but not in
-  // the model. When this occurs the tab is removed from |tabs_| and placed in
-  // |tabs_closing_map_|. When the animation completes the tab is removed from
-  // |tabs_closing_map_|. The painting code ensures both sets of tabs are
-  // painted, and the event handling code ensures only tabs in |tabs_| are used.
+  // the model. When this occurs the tab is removed from |tabs_|, but remains
+  // in |layout_helper_| until the remove animation completes.
   views::ViewModelT<Tab> tabs_;
-  TabsClosingMap tabs_closing_map_;
 
   // Map associating each group to its TabGroupHeader instance.
   std::map<TabGroupId, std::unique_ptr<TabGroupHeader>> group_headers_;

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

@@ -54,11 +54,13 @@ enum class ViewType {
 
 struct TabStripLayoutHelper::TabSlot {
   static TabStripLayoutHelper::TabSlot CreateForTab(
+      Tab* tab,
       TabAnimationState::TabOpenness openness,
       TabAnimationState::TabPinnedness pinned,
       base::OnceClosure removed_callback) {
     TabStripLayoutHelper::TabSlot slot;
     slot.type = ViewType::kTab;
+    slot.tab = tab;
     TabAnimationState initial_state = TabAnimationState::ForIdealTabState(
         openness, pinned, TabAnimationState::TabActiveness::kInactive, 0);
     slot.animation = std::make_unique<TabAnimation>(
@@ -86,9 +88,15 @@ struct TabStripLayoutHelper::TabSlot {
     return group.value();
   }
 
+  Tab* GetTab() const {
+    DCHECK(tab.has_value());
+    return tab.value();
+  }
+
   ViewType type;
   std::unique_ptr<TabAnimation> animation;
   base::Optional<TabGroupId> group;
+  base::Optional<Tab*> tab;
 };
 
 TabStripLayoutHelper::TabStripLayoutHelper(
@@ -107,6 +115,17 @@ TabStripLayoutHelper::TabStripLayoutHelper(
 
 TabStripLayoutHelper::~TabStripLayoutHelper() = default;
 
+std::vector<Tab*> TabStripLayoutHelper::GetTabs() {
+  std::vector<Tab*> tabs;
+  for (const TabSlot& slot : slots_) {
+    if (slot.type == ViewType::kTab) {
+      tabs.push_back(slot.GetTab());
+    }
+  }
+
+  return tabs;
+}
+
 bool TabStripLayoutHelper::IsAnimating() const {
   return animation_timer_.IsRunning();
 }
@@ -123,32 +142,48 @@ int TabStripLayoutHelper::GetPinnedTabCount() const {
 
 void TabStripLayoutHelper::InsertTabAtNoAnimation(
     int model_index,
+    Tab* tab,
     base::OnceClosure tab_removed_callback,
     TabAnimationState::TabActiveness active,
     TabAnimationState::TabPinnedness pinned) {
   const int slot_index = GetSlotIndexForTabModelIndex(model_index);
-  slots_.insert(slots_.begin() + slot_index,
-                TabSlot::CreateForTab(TabAnimationState::TabOpenness::kOpen,
-                                      pinned, std::move(tab_removed_callback)));
+  slots_.insert(
+      slots_.begin() + slot_index,
+      TabSlot::CreateForTab(tab, TabAnimationState::TabOpenness::kOpen, pinned,
+                            std::move(tab_removed_callback)));
 }
 
 void TabStripLayoutHelper::InsertTabAt(
     int model_index,
+    Tab* tab,
     base::OnceClosure tab_removed_callback,
     TabAnimationState::TabActiveness active,
     TabAnimationState::TabPinnedness pinned) {
   const int slot_index = GetSlotIndexForTabModelIndex(model_index);
-  slots_.insert(slots_.begin() + slot_index,
-                TabSlot::CreateForTab(TabAnimationState::TabOpenness::kClosed,
-                                      pinned, std::move(tab_removed_callback)));
+  slots_.insert(
+      slots_.begin() + slot_index,
+      TabSlot::CreateForTab(tab, TabAnimationState::TabOpenness::kClosed,
+                            pinned, std::move(tab_removed_callback)));
   AnimateSlot(slot_index,
               slots_[slot_index].animation->target_state().WithOpenness(
                   TabAnimationState::TabOpenness::kOpen));
 }
 
-void TabStripLayoutHelper::RemoveTabAt(int model_index) {
+void TabStripLayoutHelper::CloseTabAt(int model_index) {
   // TODO(958173): Animate closed.
-  slots_.erase(slots_.begin() + GetSlotIndexForTabModelIndex(model_index));
+  TabAnimation* animation =
+      slots_[GetSlotIndexForTabModelIndex(model_index)].animation.get();
+  animation->AnimateTo(animation->target_state().WithOpenness(
+      TabAnimationState::TabOpenness::kClosed));
+}
+
+void TabStripLayoutHelper::OnTabDestroyed(Tab* tab) {
+  auto it =
+      std::find_if(slots_.begin(), slots_.end(), [tab](const TabSlot& slot) {
+        return slot.type == ViewType::kTab && slot.GetTab() == tab;
+      });
+  DCHECK(it != slots_.end());
+  slots_.erase(it);
 }
 
 void TabStripLayoutHelper::MoveTab(
@@ -254,8 +289,11 @@ void TabStripLayoutHelper::UpdateIdealBounds(int available_width) {
     auto pinned = i <= last_pinned_tab_slot_index
                       ? TabAnimationState::TabPinnedness::kPinned
                       : TabAnimationState::TabPinnedness::kUnpinned;
-    ideal_animation_states.push_back(TabAnimationState::ForIdealTabState(
-        TabAnimationState::TabOpenness::kOpen, pinned, active, 0));
+    auto open = slots_[i].animation->target_state().IsFullyClosed()
+                    ? TabAnimationState::TabOpenness::kClosed
+                    : TabAnimationState::TabOpenness::kOpen;
+    ideal_animation_states.push_back(
+        TabAnimationState::ForIdealTabState(open, pinned, active, 0));
   }
 
   const std::vector<gfx::Rect> bounds = CalculateTabBounds(
@@ -271,9 +309,12 @@ void TabStripLayoutHelper::UpdateIdealBounds(int available_width) {
     const TabSlot& slot = slots_[i];
     switch (slot.type) {
       case ViewType::kTab:
-        tabs->set_ideal_bounds(current_tab_model_index, bounds[i]);
-        UpdateCachedTabWidth(i, bounds[i].width(), i == active_tab_slot_index);
-        ++current_tab_model_index;
+        if (!slot.animation->target_state().IsFullyClosed()) {
+          tabs->set_ideal_bounds(current_tab_model_index, bounds[i]);
+          UpdateCachedTabWidth(i, bounds[i].width(),
+                               i == active_tab_slot_index);
+          ++current_tab_model_index;
+        }
         break;
       case ViewType::kGroupHeader:
         group_headers[slot.GetGroup()]->SetBoundsRect(bounds[i]);
@@ -314,8 +355,15 @@ int TabStripLayoutHelper::LayoutTabs(int available_width) {
   std::vector<gfx::Rect> bounds = CalculateTabBounds(
       GetTabSizeInfo(), GetCurrentTabStates(), available_width);
 
-  // TODO(958173): Assume for now that there are no closing tabs or headers.
-  DCHECK_EQ(bounds.size(), tabs->view_size() + group_headers.size());
+  if (DCHECK_IS_ON()) {
+    int num_closing_tabs = 0;
+    for (Tab* tab : GetTabs()) {
+      if (tab->closing())
+        ++num_closing_tabs;
+    }
+    DCHECK_EQ(bounds.size(),
+              tabs->view_size() + num_closing_tabs + group_headers.size());
+  }
 
   int trailing_x = 0;
 
@@ -325,8 +373,9 @@ int TabStripLayoutHelper::LayoutTabs(int available_width) {
   for (size_t i = 0; i < bounds.size(); i++) {
     switch (slots_[i].type) {
       case ViewType::kTab: {
-        if (!tabs->view_at(current_tab_model_index)->dragging()) {
-          tabs->view_at(current_tab_model_index)->SetBoundsRect(bounds[i]);
+        Tab* tab = slots_[i].GetTab();
+        if (!tab->dragging()) {
+          tab->SetBoundsRect(bounds[i]);
           trailing_x = std::max(trailing_x, bounds[i].right());
           // TODO(958173): We shouldn't need to update the cached widths here,
           // since they're also updated in UpdateIdealBounds. However, tests
@@ -335,7 +384,8 @@ int TabStripLayoutHelper::LayoutTabs(int available_width) {
               current_tab_model_index, bounds[i].width(),
               current_tab_model_index == active_tab_model_index);
         }
-        ++current_tab_model_index;
+        if (!slots_[i].animation->target_state().IsFullyClosed())
+          ++current_tab_model_index;
         break;
       }
       case ViewType::kGroupHeader: {
@@ -353,7 +403,8 @@ int TabStripLayoutHelper::LayoutTabs(int available_width) {
 int TabStripLayoutHelper::GetSlotIndexForTabModelIndex(int model_index) const {
   int current_model_index = 0;
   for (size_t i = 0; i < slots_.size(); i++) {
-    if (slots_[i].type == ViewType::kTab) {
+    if (slots_[i].type == ViewType::kTab &&
+        !slots_[i].animation->target_state().IsFullyClosed()) {
       if (current_model_index == model_index)
         return i;
       ++current_model_index;

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

@@ -36,6 +36,10 @@ class TabStripLayoutHelper {
                        base::RepeatingClosure on_animation_progressed);
   ~TabStripLayoutHelper();
 
+  // Returns a vector of all tabs in the strip, including both closing tabs
+  // and tabs still in the model.
+  std::vector<Tab*> GetTabs();
+
   // Returns whether any animations for tabs or group headers are in progress.
   bool IsAnimating() const;
 
@@ -50,6 +54,7 @@ class TabStripLayoutHelper {
   // Inserts a new tab at |index|, without animation. |tab_removed_callback|
   // will be invoked if the tab is removed at the end of a remove animation.
   void InsertTabAtNoAnimation(int model_index,
+                              Tab* tab,
                               base::OnceClosure tab_removed_callback,
                               TabAnimationState::TabActiveness active,
                               TabAnimationState::TabPinnedness pinned);
@@ -57,14 +62,18 @@ class TabStripLayoutHelper {
   // Inserts a new tab at |index|, with animation. |tab_removed_callback| will
   // be invoked if the tab is removed at the end of a remove animation.
   void InsertTabAt(int model_index,
+                   Tab* tab,
                    base::OnceClosure tab_removed_callback,
                    TabAnimationState::TabActiveness active,
                    TabAnimationState::TabPinnedness pinned);
 
-  // Removes the tab at |index|. TODO(958173): This should invoke the associated
-  // |tab_removed_callback| but currently does not, as it would duplicate
-  // TabStrip::RemoveTabDelegate::AnimationEnded.
-  void RemoveTabAt(int model_index);
+  // Marks the tab at |model_index| as closing. Invoked when the remove
+  // animation begins and the tab is removed from the model.
+  void CloseTabAt(int model_index);
+
+  // Invoked when |tab| has been destroyed by TabStrip (i.e. the remove
+  // animation has completed).
+  void OnTabDestroyed(Tab* tab);
 
   // Moves the tab at |prev_index| with group |group_at_prev_index| to
   // |new_index|. Also updates the group header's location if necessary.
@@ -90,9 +99,11 @@ class TabStripLayoutHelper {
   // Finishes all in-progress animations.
   void CompleteAnimations();
 
-  // TODO(958173): Temporary method. Like CompleteAnimations, but does not call
-  // any associated |tab_removed_callback| or |header_removed_callback|. See
-  // comment on TabStripAnimator::CompleteAnimationsWithoutDestroyingTabs.
+  // TODO(958173): Temporary method that completes running animations
+  // without invoking the callback to destroy removed tabs. Use to hand
+  // off animation (and removed tab destruction) responsibilities from
+  // this animator to elsewhere without teleporting tabs or destroying
+  // the same tab more than once.
   void CompleteAnimationsWithoutDestroyingTabs();
 
   // Generates and sets the ideal bounds for the views in |tabs| and