Refactor Tab Groups part 3

Created TabGroupViews, which manages most of the tab group's visual calculations and updates. TabStrip still paints the views, but it no longer keeps low-level maps to each individual view.

TabGroupUnderline can now use TabGroupViews for the bulk of its visual calculations. In the future, TabGroupHighlight will take a similar approach.

Change-Id: I894dc03375cb29b42b80bd1273587a41131f1b22
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1925336
Commit-Queue: Connie Wan <connily@chromium.org>
Reviewed-by: Taylor Bergquist <tbergquist@chromium.org>
Cr-Commit-Position: refs/heads/master@{#721202}

chrome/browser/ui/BUILD.gn

@@ -3193,6 +3193,8 @@ jumbo_static_library("ui") {
       "views/tabs/tab_group_header.h",
       "views/tabs/tab_group_underline.cc",
       "views/tabs/tab_group_underline.h",
+      "views/tabs/tab_group_views.cc",
+      "views/tabs/tab_group_views.h",
       "views/tabs/tab_hover_card_bubble_view.cc",
       "views/tabs/tab_hover_card_bubble_view.h",
       "views/tabs/tab_icon.cc",

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

@@ -21,7 +21,7 @@ class TabGroupEditorBubbleViewDialogBrowserTest : public DialogBrowserTest {
 
     BrowserView* browser_view = static_cast<BrowserView*>(browser()->window());
     TabGroupHeader* header =
-        browser_view->tabstrip()->group_headers_[group].get();
+        browser_view->tabstrip()->group_views_[group]->header();
     ASSERT_NE(nullptr, header);
 
     ui::MouseEvent pressed_event(ui::ET_MOUSE_PRESSED, gfx::PointF(),

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

@@ -7,12 +7,12 @@
 #include <memory>
 #include <utility>
 
+#include "chrome/browser/ui/layout_constants.h"
 #include "chrome/browser/ui/tabs/tab_group_visual_data.h"
 #include "chrome/browser/ui/tabs/tab_style.h"
 #include "chrome/browser/ui/views/tabs/tab.h"
 #include "chrome/browser/ui/views/tabs/tab_group_header.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_group_views.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/gfx/canvas.h"
 #include "ui/views/background.h"
@@ -20,12 +20,13 @@
 
 constexpr int TabGroupUnderline::kStrokeThickness;
 
-TabGroupUnderline::TabGroupUnderline(TabStrip* tab_strip, TabGroupId group)
-    : tab_strip_(tab_strip), group_(group) {
+TabGroupUnderline::TabGroupUnderline(TabGroupViews* tab_group_views,
+                                     TabGroupId group)
+    : tab_group_views_(tab_group_views), group_(group) {
   // Set non-zero bounds to start with, so that painting isn't pruned.
   // Needed because UpdateBounds() happens during OnPaint(), which is called
   // after painting is pruned.
-  const int y = tab_strip_->bounds().height() - 1;
+  const int y = GetLayoutConstant(TAB_HEIGHT) - 1;
   SetBounds(0, y - kStrokeThickness, kStrokeThickness * 2, kStrokeThickness);
 }
 
@@ -35,7 +36,7 @@ void TabGroupUnderline::OnPaint(gfx::Canvas* canvas) {
   SkPath path = GetPath();
   cc::PaintFlags flags;
   flags.setAntiAlias(true);
-  flags.setColor(GetColor());
+  flags.setColor(tab_group_views_->GetGroupColor());
   flags.setStyle(cc::PaintFlags::kFill_Style);
   canvas->DrawPath(path, flags);
 }
@@ -50,10 +51,8 @@ void TabGroupUnderline::UpdateBounds() {
   if (end_x <= start_x)
     return;
 
-  const int start_y = tab_strip_->bounds().height() - 1;
-
-  SetBounds(start_x, start_y - kStrokeThickness, end_x - start_x,
-            kStrokeThickness);
+  const int y = tab_group_views_->GetBounds().height() - 1;
+  SetBounds(start_x, y - kStrokeThickness, end_x - start_x, kStrokeThickness);
 }
 
 // static
@@ -62,29 +61,20 @@ int TabGroupUnderline::GetStrokeInset() {
 }
 
 int TabGroupUnderline::GetStart() const {
-  const TabGroupHeader* group_header = tab_strip_->group_header(group_);
+  const gfx::Rect group_bounds = tab_group_views_->GetBounds();
 
-  return group_header->bounds().x() + GetStrokeInset();
+  return group_bounds.x() + GetStrokeInset();
 }
 
 int TabGroupUnderline::GetEnd() const {
-  // Fall back to the group header end for any corner cases. This ensures
-  // that the underline always has a positive width.
-  const TabGroupHeader* group_header = tab_strip_->group_header(group_);
-  const int header_end = group_header->bounds().right() - GetStrokeInset();
-
-  const std::vector<int> tabs_in_group =
-      tab_strip_->controller()->ListTabsInGroup(group_);
-  if (tabs_in_group.size() <= 0)
-    return header_end;
-
-  const int last_tab_index = tabs_in_group[tabs_in_group.size() - 1];
-  const Tab* last_tab = tab_strip_->tab_at(last_tab_index);
-
-  const int tab_end =
-      last_tab->bounds().right() +
-      (last_tab->IsActive() ? kStrokeThickness : -GetStrokeInset());
-  return std::max(tab_end, header_end);
+  const gfx::Rect group_bounds = tab_group_views_->GetBounds();
+
+  const Tab* last_grouped_tab = tab_group_views_->GetLastTabInGroup();
+  if (!last_grouped_tab)
+    return group_bounds.right() - GetStrokeInset();
+
+  return group_bounds.right() +
+         (last_grouped_tab->IsActive() ? kStrokeThickness : -GetStrokeInset());
 }
 
 SkPath TabGroupUnderline::GetPath() const {
@@ -100,10 +90,3 @@ SkPath TabGroupUnderline::GetPath() const {
 
   return path;
 }
-
-SkColor TabGroupUnderline::GetColor() const {
-  const TabGroupVisualData* data =
-      tab_strip_->controller()->GetVisualDataForGroup(group_);
-
-  return data->color();
-}

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

@@ -8,7 +8,7 @@
 #include "chrome/browser/ui/tabs/tab_group_id.h"
 #include "ui/views/view.h"
 
-class TabStrip;
+class TabGroupViews;
 
 // View for tab group underlines in the tab strip, which are markers of group
 // members. There is one underline for each group, which is included in the tab
@@ -18,7 +18,7 @@ class TabGroupUnderline : public views::View {
   static constexpr int kStrokeThickness = 3;
   static int GetStrokeInset();
 
-  TabGroupUnderline(TabStrip* tab_strip, TabGroupId group);
+  TabGroupUnderline(TabGroupViews* tab_group_views, TabGroupId group);
 
   TabGroupId group() const { return group_; }
 
@@ -42,10 +42,7 @@ class TabGroupUnderline : public views::View {
   // represented using a fill path.
   SkPath GetPath() const;
 
-  // The underline color is the group color.
-  SkColor GetColor() const;
-
-  TabStrip* const tab_strip_;
+  TabGroupViews* const tab_group_views_;
   const TabGroupId group_;
 
   DISALLOW_COPY_AND_ASSIGN(TabGroupUnderline);

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

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/views/tabs/tab_group_views.h"
+
+#include <utility>
+
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/views/tabs/tab_group_header.h"
+#include "chrome/browser/ui/views/tabs/tab_group_underline.h"
+#include "chrome/browser/ui/views/tabs/tab_strip.h"
+#include "chrome/browser/ui/views/tabs/tab_strip_controller.h"
+#include "third_party/skia/include/core/SkColor.h"
+#include "ui/gfx/geometry/rect.h"
+
+TabGroupViews::TabGroupViews(TabStrip* tab_strip, TabGroupId group)
+    : tab_strip_(tab_strip), group_(group) {
+  header_ = std::make_unique<TabGroupHeader>(tab_strip_, group_);
+  header_->set_owned_by_client();
+
+  underline_ = std::make_unique<TabGroupUnderline>(this, group_);
+  underline_->set_owned_by_client();
+}
+
+TabGroupViews::~TabGroupViews() {}
+
+void TabGroupViews::UpdateVisuals() {
+  header_->VisualsChanged();
+  underline_->SchedulePaint();
+
+  const int active_index = tab_strip_->controller()->GetActiveIndex();
+  if (active_index != TabStripModel::kNoTab)
+    tab_strip_->tab_at(active_index)->SchedulePaint();
+}
+
+gfx::Rect TabGroupViews::GetBounds() const {
+  const Tab* last_tab = GetLastTabInGroup();
+  if (!last_tab)
+    return header_->bounds();
+
+  const int start_x = header_->x();
+  const int end_x = last_tab->bounds().right();
+
+  if (end_x <= start_x)
+    return header_->bounds();
+
+  const int y = header_->y();
+  const int height = header_->height();
+
+  return gfx::Rect(start_x, y, end_x - start_x, height);
+}
+
+Tab* TabGroupViews::GetLastTabInGroup() const {
+  const std::vector<int> tabs_in_group =
+      tab_strip_->controller()->ListTabsInGroup(group_);
+
+  if (tabs_in_group.size() <= 0)
+    return nullptr;
+
+  const int last_tab_index = tabs_in_group[tabs_in_group.size() - 1];
+  return tab_strip_->tab_at(last_tab_index);
+}
+
+SkColor TabGroupViews::GetGroupColor() const {
+  const TabGroupVisualData* data =
+      tab_strip_->controller()->GetVisualDataForGroup(group_);
+
+  return data->color();
+}

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

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_UI_VIEWS_TABS_TAB_GROUP_VIEWS_H_
+#define CHROME_BROWSER_UI_VIEWS_TABS_TAB_GROUP_VIEWS_H_
+
+#include <memory>
+
+#include "chrome/browser/ui/tabs/tab_group_id.h"
+#include "third_party/skia/include/core/SkColor.h"
+#include "ui/gfx/geometry/rect.h"
+
+class Tab;
+class TabGroupHeader;
+class TabGroupUnderline;
+class TabStrip;
+
+// The manager of all views associated with a tab group. This handles visual
+// calculations and updates. Painting is done in TabStrip.
+class TabGroupViews {
+ public:
+  TabGroupViews(TabStrip* tab_strip, TabGroupId group);
+  ~TabGroupViews();
+
+  TabGroupId group() const { return group_; }
+  TabGroupHeader* header() const { return header_.get(); }
+  TabGroupUnderline* underline() const { return underline_.get(); }
+
+  // Updates the visuals of each view in preparation for repainting.
+  void UpdateVisuals();
+
+  // Returns the bounds of the entire group, including the header and all tabs.
+  gfx::Rect GetBounds() const;
+
+  // Returns the last tab in the group. Used for some visual calculations.
+  Tab* GetLastTabInGroup() const;
+
+  // Returns the group color.
+  SkColor GetGroupColor() const;
+
+ private:
+  TabStrip* const tab_strip_;
+  const TabGroupId group_;
+  std::unique_ptr<TabGroupHeader> header_;
+  std::unique_ptr<TabGroupUnderline> underline_;
+};
+
+#endif  // CHROME_BROWSER_UI_VIEWS_TABS_TAB_GROUP_VIEWS_H_

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

@@ -43,6 +43,7 @@
 #include "chrome/browser/ui/views/tabs/tab_drag_controller.h"
 #include "chrome/browser/ui/views/tabs/tab_group_header.h"
 #include "chrome/browser/ui/views/tabs/tab_group_underline.h"
+#include "chrome/browser/ui/views/tabs/tab_group_views.h"
 #include "chrome/browser/ui/views/tabs/tab_hover_card_bubble_view.h"
 #include "chrome/browser/ui/views/tabs/tab_slot_view.h"
 #include "chrome/browser/ui/views/tabs/tab_strip_controller.h"
@@ -1242,44 +1243,31 @@ void TabStrip::AddTabToGroup(base::Optional<TabGroupId> group,
 }
 
 void TabStrip::OnGroupCreated(TabGroupId group) {
-  auto header = std::make_unique<TabGroupHeader>(this, group);
-  header->set_owned_by_client();
-  AddChildView(header.get());
+  std::unique_ptr<TabGroupViews> group_view =
+      std::make_unique<TabGroupViews>(this, group);
+  AddChildView(group_view->header());
+  AddChildView(group_view->underline());
   layout_helper_->InsertGroupHeader(
-      group, header.get(),
+      group, group_view->header(),
       base::BindOnce(&TabStrip::OnGroupCloseAnimationCompleted,
                      base::Unretained(this), group));
-  group_headers_[group] = std::move(header);
-
-  auto underline = std::make_unique<TabGroupUnderline>(this, group);
-  underline->set_owned_by_client();
-  AddChildView(underline.get());
-  group_underlines_[group] = std::move(underline);
+  group_views_[group] = std::move(group_view);
 }
 
 void TabStrip::OnGroupContentsChanged(TabGroupId group) {
-  DCHECK(group_headers_[group]);
-  DCHECK(group_underlines_[group]);
+  DCHECK(group_views_[group]);
   // The group header may be in the wrong place if the tab didn't actually
   // move in terms of model indices.
   layout_helper_->UpdateGroupHeaderIndex(group);
-  group_underlines_[group]->SchedulePaint();
-  const int active_index = controller_->GetActiveIndex();
-  if (active_index != ui::ListSelectionModel::kUnselectedIndex)
-    tab_at(active_index)->SchedulePaint();
+  group_views_[group]->UpdateVisuals();
   UpdateIdealBounds();
   AnimateToIdealBounds();
 }
 
 void TabStrip::OnGroupVisualsChanged(TabGroupId group) {
-  DCHECK(group_headers_[group]);
-  DCHECK(group_underlines_[group]);
-  group_headers_[group]->VisualsChanged();
-  group_underlines_[group]->SchedulePaint();
-  const int active_index = controller_->GetActiveIndex();
-  if (active_index != ui::ListSelectionModel::kUnselectedIndex)
-    tab_at(active_index)->SchedulePaint();
-  // The group title may have changed size.
+  DCHECK(group_views_[group]);
+  group_views_[group]->UpdateVisuals();
+  // The group title may have changed size, so update bounds.
   UpdateIdealBounds();
   AnimateToIdealBounds();
 }
@@ -1288,6 +1276,7 @@ void TabStrip::OnGroupDeleted(TabGroupId group) {
   layout_helper_->RemoveGroupHeader(group);
   UpdateIdealBounds();
   AnimateToIdealBounds();
+  // The group_views_ mapping is erased in OnGroupCloseAnimationCompleted().
 }
 
 bool TabStrip::ShouldTabBeVisible(const Tab* tab) const {
@@ -2033,13 +2022,11 @@ void TabStrip::PaintChildren(const views::PaintInfo& paint_info) {
   for (Tab* tab : selected_and_hovered_tabs)
     tab->Paint(paint_info);
 
-  // Paint group headers.
-  for (const auto& header_pair : group_headers_)
-    header_pair.second->Paint(paint_info);
-
-  // Paint group underlines.
-  for (const auto& underline_pair : group_underlines_)
-    underline_pair.second->Paint(paint_info);
+  // Paint group headers and underlines.
+  for (const auto& group_view_pair : group_views_) {
+    group_view_pair.second->header()->Paint(paint_info);
+    group_view_pair.second->underline()->Paint(paint_info);
+  }
 
   // Always paint the active tab over all the inactive tabs.
   if (active_tab && !is_dragging)
@@ -2059,7 +2046,7 @@ void TabStrip::PaintChildren(const views::PaintInfo& paint_info) {
   if (tabs_dragging.size() > 0) {
     const base::Optional<TabGroupId> dragged_group = tabs_dragging[0]->group();
     if (dragged_group.has_value())
-      group_underlines_[dragged_group.value()]->Paint(paint_info);
+      group_views_[dragged_group.value()]->underline()->Paint(paint_info);
   }
 
   // If the active tab is being dragged, it goes last.
@@ -2232,11 +2219,10 @@ void TabStrip::Init() {
 }
 
 std::map<TabGroupId, TabGroupHeader*> TabStrip::GetGroupHeaders() {
-  // Transform |group_headers_| to raw pointers to avoid exposing unique_ptrs.
   std::map<TabGroupId, TabGroupHeader*> group_headers;
-  for (const auto& header_pair : group_headers_) {
-    group_headers.insert(
-        std::make_pair(header_pair.first, header_pair.second.get()));
+  for (const auto& group_view_pair : group_views_) {
+    group_headers.insert(std::make_pair(group_view_pair.first,
+                                        group_view_pair.second->header()));
   }
   return group_headers;
 }
@@ -2634,8 +2620,7 @@ void TabStrip::OnTabCloseAnimationCompleted(Tab* tab) {
 }
 
 void TabStrip::OnGroupCloseAnimationCompleted(TabGroupId group) {
-  group_headers_.erase(group);
-  group_underlines_.erase(group);
+  group_views_.erase(group);
   // TODO(crbug.com/905491): We might want to simulate a mouse move here, like
   // we do in OnTabCloseAnimationCompleted.
 }

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

@@ -27,6 +27,7 @@
 #include "chrome/browser/ui/views/tabs/tab_controller.h"
 #include "chrome/browser/ui/views/tabs/tab_drag_context.h"
 #include "chrome/browser/ui/views/tabs/tab_group_header.h"
+#include "chrome/browser/ui/views/tabs/tab_group_views.h"
 #include "chrome/browser/ui/views/tabs/tab_strip.h"
 #include "ui/base/material_design/material_design_controller.h"
 #include "ui/base/material_design/material_design_controller_observer.h"
@@ -46,7 +47,6 @@ class Browser;
 class NewTabButton;
 class StackedTabStripLayout;
 class Tab;
-class TabGroupUnderline;
 class TabGroupId;
 class TabHoverCardBubbleView;
 class TabStripController;
@@ -210,7 +210,7 @@ class TabStrip : public views::AccessiblePaneView,
 
   // Returns the TabGroupHeader with ID |id|.
   TabGroupHeader* group_header(TabGroupId id) {
-    return group_headers_[id].get();
+    return group_views_[id].get()->header();
   }
 
   // Returns the NewTabButton.
@@ -640,11 +640,7 @@ class TabStrip : public views::AccessiblePaneView,
   // in |layout_helper_| until the remove animation completes.
   views::ViewModelT<Tab> tabs_;
 
-  // Map associating each group to its TabGroupHeader instance.
-  std::map<TabGroupId, std::unique_ptr<TabGroupHeader>> group_headers_;
-
-  // Map associating each group to its TabGroupUnderline instance.
-  std::map<TabGroupId, std::unique_ptr<TabGroupUnderline>> group_underlines_;
+  std::map<TabGroupId, std::unique_ptr<TabGroupViews>> group_views_;
 
   // The view tracker is used to keep track of if the hover card has been
   // destroyed by its widget.

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

@@ -219,15 +219,15 @@ class TabStripTest : public ChromeViewsTestBase,
 
   std::vector<TabGroupHeader*> ListGroupHeaders() const {
     std::vector<TabGroupHeader*> result;
-    for (auto const& header_pair : tab_strip_->group_headers_)
-      result.push_back(header_pair.second.get());
+    for (auto const& group_view_pair : tab_strip_->group_views_)
+      result.push_back(group_view_pair.second->header());
     return result;
   }
 
   std::vector<TabGroupUnderline*> ListGroupUnderlines() const {
     std::vector<TabGroupUnderline*> result;
-    for (auto const& underline_pair : tab_strip_->group_underlines_)
-      result.push_back(underline_pair.second.get());
+    for (auto const& group_view_pair : tab_strip_->group_views_)
+      result.push_back(group_view_pair.second->underline());
     return result;
   }