Clean up TabGroupUnderline implementation

Address remaining comments from https://chromium-review.googlesource.com/c/chromium/src/+/1809546

Bug: 905491
Change-Id: I92242ff19cd8c0d85500e7dd66b76a4d1b29fab9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1818434
Commit-Queue: Connie Wan <connily@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#699936}

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

@@ -17,6 +17,8 @@
 #include "ui/views/background.h"
 #include "ui/views/view.h"
 
+constexpr int TabGroupUnderline::kStrokeThickness;
+
 TabGroupUnderline::TabGroupUnderline(TabStrip* tab_strip, TabGroupId group)
     : tab_strip_(tab_strip), group_(group) {
   UpdateVisuals();
@@ -30,24 +32,24 @@ void TabGroupUnderline::OnPaint(gfx::Canvas* canvas) {
 void TabGroupUnderline::UpdateVisuals() {
   SkColor color = GetColor();
 
-  int start_x = GetStart();
-  int end_x = GetEnd();
+  const int start_x = GetStart();
+  const int end_x = GetEnd();
 
-  int start_y = tab_strip_->bounds().height() - 1;
-  constexpr int kStrokeWidth = 2;
+  const int start_y = tab_strip_->bounds().height() - 1;
 
-  SetBounds(start_x, start_y - kStrokeWidth, end_x - start_x, kStrokeWidth);
+  SetBounds(start_x, start_y - kStrokeThickness, end_x - start_x,
+            kStrokeThickness);
   SetBackground(views::CreateSolidBackground(color));
 }
 
-SkColor TabGroupUnderline::GetColor() {
+SkColor TabGroupUnderline::GetColor() const {
   const TabGroupVisualData* data =
       tab_strip_->controller()->GetVisualDataForGroup(group_);
 
   return data->color();
 }
 
-int TabGroupUnderline::GetStart() {
+int TabGroupUnderline::GetStart() const {
   const gfx::Rect group_header_bounds =
       tab_strip_->group_header(group_)->bounds();
 
@@ -55,7 +57,7 @@ int TabGroupUnderline::GetStart() {
   return group_header_bounds.x() + kInset;
 }
 
-int TabGroupUnderline::GetEnd() {
+int TabGroupUnderline::GetEnd() const {
   const std::vector<int> tabs_in_group =
       tab_strip_->controller()->ListTabsInGroup(group_);
   const int last_tab_index = tabs_in_group[tabs_in_group.size() - 1];

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

@@ -6,15 +6,17 @@
 #define CHROME_BROWSER_UI_VIEWS_TABS_TAB_GROUP_UNDERLINE_H_
 
 #include "chrome/browser/ui/tabs/tab_group_id.h"
-#include "chrome/browser/ui/views/tabs/tab_strip.h"
-#include "chrome/browser/ui/views/tabs/tab_strip_controller.h"
 #include "ui/views/view.h"
 
+class TabStrip;
+
 // 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
 // strip flow and positioned across all tabs in the group.
 class TabGroupUnderline : public views::View {
  public:
+  static constexpr int kStrokeThickness = 2;
+
   TabGroupUnderline(TabStrip* tab_strip, TabGroupId group);
 
   TabGroupId group() const { return group_; }
@@ -26,18 +28,18 @@ class TabGroupUnderline : public views::View {
   void OnPaint(gfx::Canvas* canvas) override;
 
  private:
-  TabStrip* const tab_strip_;
-  const TabGroupId group_;
-
   // The underline color is the group color.
-  SkColor GetColor();
+  SkColor GetColor() const;
 
   // The underline starts at the left edge of the header chip.
-  int GetStart();
+  int GetStart() const;
 
   // The underline ends at the right edge of the last grouped tab's close
   // button.
-  int GetEnd();
+  int GetEnd() const;
+
+  TabStrip* const tab_strip_;
+  const TabGroupId group_;
 
   DISALLOW_COPY_AND_ASSIGN(TabGroupUnderline);
 };

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

@@ -25,6 +25,7 @@
 #include "chrome/browser/ui/views/tabs/tab_animation_state.h"
 #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_strip.h"
 #include "ui/base/material_design/material_design_controller.h"
 #include "ui/base/material_design/material_design_controller_observer.h"
@@ -43,7 +44,6 @@
 class NewTabButton;
 class StackedTabStripLayout;
 class Tab;
-class TabGroupHeader;
 class TabGroupUnderline;
 class TabGroupId;
 class TabHoverCardBubbleView;
@@ -200,7 +200,9 @@ class TabStrip : public views::AccessiblePaneView,
   Tab* tab_at(int index) const { return tabs_.view_at(index); }
 
   // Returns the TabGroupHeader with ID |id|.
-  TabGroupHeader* group_header(TabGroupId id) { return GetGroupHeaders()[id]; }
+  TabGroupHeader* group_header(TabGroupId id) {
+    return group_headers_[id].get();
+  }
 
   // Returns the NewTabButton.
   NewTabButton* new_tab_button() { return new_tab_button_; }

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

@@ -1166,7 +1166,7 @@ TEST_P(TabStripTest, DiscontinuousGroup) {
   EXPECT_EQ(first_slot_x, headers[0]->x());
 }
 
-TEST_P(TabStripTest, DeleteTabGroupHeaderWhenEmpty) {
+TEST_P(TabStripTest, DeleteTabGroupHeaderAndUnderlineWhenEmpty) {
   tab_strip_->AddTabAt(0, TabRendererData(), false);
   tab_strip_->AddTabAt(1, TabRendererData(), false);
   base::Optional<TabGroupId> group = TabGroupId::GenerateNew();
@@ -1175,8 +1175,10 @@ TEST_P(TabStripTest, DeleteTabGroupHeaderWhenEmpty) {
   controller_->MoveTabIntoGroup(0, base::nullopt);
 
   EXPECT_EQ(1u, ListGroupHeaders().size());
+  EXPECT_EQ(1u, ListGroupUnderlines().size());
   controller_->MoveTabIntoGroup(1, base::nullopt);
   EXPECT_EQ(0u, ListGroupHeaders().size());
+  EXPECT_EQ(0u, ListGroupUnderlines().size());
 }
 
 TEST_P(TabStripTest, GroupUnderlineBasics) {
@@ -1195,25 +1197,11 @@ TEST_P(TabStripTest, GroupUnderlineBasics) {
   // Update underline manually in the absence of a real Paint cycle.
   underline->UpdateVisuals();
   constexpr int kInset = 20;
-  constexpr int kStrokeWidth = 2;
   EXPECT_EQ(underline->x(), kInset);
   EXPECT_GT(underline->width(), 0);
   EXPECT_EQ(underline->bounds().right(),
             tab_strip_->tab_at(0)->bounds().right() - kInset);
-  EXPECT_EQ(underline->height(), kStrokeWidth);
-}
-
-TEST_P(TabStripTest, DeleteTabGroupUnderlineWhenEmpty) {
-  tab_strip_->AddTabAt(0, TabRendererData(), false);
-  tab_strip_->AddTabAt(1, TabRendererData(), false);
-  base::Optional<TabGroupId> group = TabGroupId::GenerateNew();
-  controller_->MoveTabIntoGroup(0, group);
-  controller_->MoveTabIntoGroup(1, group);
-  controller_->MoveTabIntoGroup(0, base::nullopt);
-
-  EXPECT_EQ(1u, ListGroupUnderlines().size());
-  controller_->MoveTabIntoGroup(1, base::nullopt);
-  EXPECT_EQ(0u, ListGroupUnderlines().size());
+  EXPECT_EQ(underline->height(), TabGroupUnderline::kStrokeThickness);
 }
 
 TEST_P(TabStripTest, ChangingLayoutTypeResizesTabs) {

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

@@ -18,6 +18,7 @@
 #include "chrome/browser/ui/views/tabs/tab.h"
 #include "chrome/browser/ui/views/tabs/tab_close_button.h"
 #include "chrome/browser/ui/views/tabs/tab_controller.h"
+#include "chrome/browser/ui/views/tabs/tab_group_underline.h"
 #include "chrome/grit/theme_resources.h"
 #include "third_party/skia/include/core/SkScalar.h"
 #include "third_party/skia/include/pathops/SkPathOps.h"
@@ -655,7 +656,7 @@ float GM2TabStyle::GetThrobValue() const {
 int GM2TabStyle::GetStrokeThickness(bool should_paint_as_active) const {
   base::Optional<SkColor> group_color = tab_->GetGroupColor();
   if (group_color.has_value() && tab_->IsActive())
-    return 2;
+    return TabGroupUnderline::kStrokeThickness;
 
   if (tab_->IsActive() || should_paint_as_active)
     return tab_->controller()->GetStrokeThickness();