Refresh tab group visuals after selection change.

When the selected tab in a tab group changes, the length of the group
underline may need to change. This was not happening, resulting in the
attached bug.

This CL refactors TabStrip::SetSelection() in a handful of small ways:
- makes old selected tab and new selected tab explicit local variables
- simplifies conditional logic slightly
- makes an implicit assumption that new selected tab is valid explicit
  * there had been a check in one place for an invalid index
  * however, it just blindly dereferences it later in the function which
    would have caused a crash
- when active tab changes, if either tab was in a group, recalculates
  group underline and highlight bounds via UpdateTabGroupVisuals()

Fixed: 1098392
Change-Id: Ib4b41bd8ce3e7c601d0329cef9d96b8d5846d7b9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2261166
Commit-Queue: Dana Fried <dfried@chromium.org>
Commit-Queue: Connie Wan <connily@chromium.org>
Auto-Submit: Dana Fried <dfried@chromium.org>
Reviewed-by: Connie Wan <connily@chromium.org>
Cr-Commit-Position: refs/heads/master@{#781533}

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

@@ -1481,20 +1481,28 @@ bool TabStrip::ShouldDrawStrokes() const {
 }
 
 void TabStrip::SetSelection(const ui::ListSelectionModel& new_selection) {
-  if (selected_tabs_.active() != new_selection.active()) {
-    if (selected_tabs_.active() >= 0)
-      tab_at(selected_tabs_.active())->ActiveStateChanged();
-    if (new_selection.active() >= 0) {
-      Tab* new_active_tab = tab_at(new_selection.active());
-
+  DCHECK_GE(new_selection.active(), 0)
+      << "We should never transition to a state where no tab is active.";
+  Tab* const new_active_tab = tab_at(new_selection.active());
+  Tab* const old_active_tab =
+      selected_tabs_.active() >= 0 ? tab_at(selected_tabs_.active()) : nullptr;
+
+  if (new_active_tab != old_active_tab) {
+    if (old_active_tab) {
+      old_active_tab->ActiveStateChanged();
+      if (old_active_tab->group().has_value())
+        UpdateTabGroupVisuals(old_active_tab->group().value());
+    }
+    if (new_active_tab->group().has_value()) {
+      const tab_groups::TabGroupId new_group = new_active_tab->group().value();
       // If the tab that is about to be activated is in a collapsed group,
       // automatically expand the group.
-      base::Optional<tab_groups::TabGroupId> group = new_active_tab->group();
-      if (group.has_value() && controller()->IsGroupCollapsed(group.value()))
-        controller()->ToggleTabGroupCollapsedState(group.value());
-
-      new_active_tab->ActiveStateChanged();
+      if (controller()->IsGroupCollapsed(new_group))
+        controller()->ToggleTabGroupCollapsedState(new_group);
+      UpdateTabGroupVisuals(new_group);
     }
+
+    new_active_tab->ActiveStateChanged();
     layout_helper_->SetActiveTab(selected_tabs_.active(),
                                  new_selection.active());
   }
@@ -1538,8 +1546,7 @@ void TabStrip::SetSelection(const ui::ListSelectionModel& new_selection) {
       base::STLSetDifference<ui::ListSelectionModel::SelectedIndices>(
           new_selection.selected_indices(), selected_tabs_.selected_indices());
 
-  tab_at(new_selection.active())
-      ->NotifyAccessibilityEvent(ax::mojom::Event::kSelection, true);
+  new_active_tab->NotifyAccessibilityEvent(ax::mojom::Event::kSelection, true);
   selected_tabs_ = new_selection;
 
   UpdateHoverCard(nullptr);