Fix layout/repaint of tab on focus or alert change.

Also removes code associated with "show close button on hover" in
non-touch situations because showing of close button on inactive tabs is
now purely a function of tab size and the code had no effect.

Previously, tabs were relying on invalidation of the tabstrip to repaint
when active status changed. This resulted in cases where when the number
of tabs at minimum size exceded the size of the browser window, pinned
tabs would not repaint when a new tab was added, resulting in them
appearing to still be active (until something else invalidated the
tabstrip, like a mouse event or resize).

Now we're using InvalidateLayout() and SchedulePaint() correctly to make
sure things which could change the render or layout state of a tab
result in the tab being laid out and/or repainted.

Bug: 896849
Change-Id: If2fcdafe80a0d22dc1d7c5c2c4a703729b8ed55b
Reviewed-on: https://chromium-review.googlesource.com/c/1357329
Commit-Queue: Dana Fried <dfried@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613252}

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

@@ -510,14 +510,12 @@ void Tab::OnMouseEntered(const ui::MouseEvent& event) {
       controller_->GetHoverOpacityForRadialHighlight());
   hover_controller_.Show(GlowHoverController::SUBTLE);
   UpdateForegroundColors();
-  Layout();
 }
 
 void Tab::OnMouseExited(const ui::MouseEvent& event) {
   mouse_hovered_ = false;
   hover_controller_.Hide();
   UpdateForegroundColors();
-  Layout();
 }
 
 void Tab::OnGestureEvent(ui::GestureEvent* event) {
@@ -673,16 +671,15 @@ bool Tab::IsActive() const {
 void Tab::ActiveStateChanged() {
   UpdateTabIconNeedsAttentionBlocked();
   UpdateForegroundColors();
-  Layout();
+  InvalidateLayout();
 }
 
 void Tab::AlertStateChanged() {
-  Layout();
+  InvalidateLayout();
 }
 
 void Tab::FrameColorsChanged() {
   UpdateForegroundColors();
-  SchedulePaint();
 }
 
 void Tab::SelectedStateChanged() {
@@ -724,8 +721,7 @@ void Tab::SetData(TabRendererData data) {
   if (data_.alert_state != old.alert_state || data_.title != old.title)
     TooltipTextChanged();
 
-  Layout();
-  SchedulePaint();
+  InvalidateLayout();
 }
 
 void Tab::StepLoadingAnimation(const base::TimeDelta& elapsed_time) {
@@ -917,12 +913,8 @@ void Tab::UpdateIconVisibility() {
     if (showing_icon_)
       available_width -= favicon_width;
 
-    // Show the close button if it's allowed to show on hover, even if it's
-    // forced to be hidden normally.
-    const bool show_on_hover = controller_->ShouldShowCloseButtonOnHover();
-    showing_close_button_ |= show_on_hover && hover_controller_.ShouldDraw();
     showing_close_button_ &= large_enough_for_close_button;
-    if (showing_close_button_ || show_on_hover)
+    if (showing_close_button_)
       available_width -= close_button_width;
 
     // If no other controls are visible, show the alert icon or the favicon
@@ -1059,4 +1051,6 @@ void Tab::UpdateForegroundColors() {
     button_color_ = generated_icon_color;
     alert_indicator_->OnParentTabButtonColorChanged();
   }
+
+  SchedulePaint();
 }

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

@@ -42,11 +42,6 @@ class TabController {
   // Returns true if the close button for the given tab is forced to be hidden.
   virtual bool ShouldHideCloseButtonForTab(Tab* tab) const = 0;
 
-  // Returns true if the close button on an inactive tab should be shown on
-  // mouse hover. This is predicated on ShouldHideCloseButtonForInactiveTabs()
-  // returning true.
-  virtual bool ShouldShowCloseButtonOnHover() = 0;
-
   // Returns true if ShouldPaintTab() could return a non-empty clip path.
   virtual bool MaySetClip() = 0;
 

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

@@ -953,10 +953,6 @@ bool TabStrip::ShouldHideCloseButtonForTab(Tab* tab) const {
   return !!touch_layout_;
 }
 
-bool TabStrip::ShouldShowCloseButtonOnHover() {
-  return !touch_layout_;
-}
-
 bool TabStrip::MaySetClip() {
   // Only touch layout needs to restrict the clip.
   return touch_layout_ || IsStackingDraggedTabs();

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

@@ -251,7 +251,6 @@ class TabStrip : public views::AccessiblePaneView,
   bool SupportsMultipleSelection() override;
   NewTabButtonPosition GetNewTabButtonPosition() const override;
   bool ShouldHideCloseButtonForTab(Tab* tab) const override;
-  bool ShouldShowCloseButtonOnHover() override;
   bool MaySetClip() override;
   void SelectTab(Tab* tab) override;
   void ExtendSelectionTo(Tab* tab) override;

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

@@ -216,6 +216,12 @@ class TabStripTest : public ChromeViewsTestBase,
     tab_strip_->StoppedDraggingTab(tab, &is_first_tab);
   }
 
+  void LayoutTabs() {
+    for (int i = 0; i < tab_strip_->tab_count(); ++i) {
+      tab_strip_->tab_at(i)->Layout();
+    }
+  }
+
   // Owned by TabStrip.
   FakeBaseTabStripController* controller_ = nullptr;
   TabStrip* tab_strip_ = nullptr;
@@ -477,6 +483,7 @@ TEST_P(TabStripTest, TabCloseButtonVisibilityWhenStacked) {
   // tab close button hidden and the newly-active tab should show
   // its tab close button.
   tab_strip_->SelectTab(tab2);
+  LayoutTabs();
   ASSERT_FALSE(tab1->IsActive());
   ASSERT_TRUE(tab2->IsActive());
   EXPECT_FALSE(tab0->showing_close_button_);
@@ -487,6 +494,7 @@ TEST_P(TabStripTest, TabCloseButtonVisibilityWhenStacked) {
   // After closing the active tab, the tab which becomes active should
   // show its tab close button.
   tab_strip_->CloseTab(tab1, CLOSE_TAB_FROM_TOUCH);
+  LayoutTabs();
   tab1 = nullptr;
   ASSERT_TRUE(tab2->IsActive());
   EXPECT_FALSE(tab0->showing_close_button_);
@@ -550,6 +558,7 @@ TEST_P(TabStripTest, TabCloseButtonVisibilityWhenNotStacked) {
   // An active tab added to the tabstrip should show its tab close
   // button.
   controller_->AddTab(4, true);
+  LayoutTabs();
   Tab* tab4 = tab_strip_->tab_at(4);
   ASSERT_TRUE(tab4->IsActive());
   EXPECT_TRUE(tab4->showing_close_button_);
@@ -562,6 +571,7 @@ TEST_P(TabStripTest, TabCloseButtonVisibilityWhenNotStacked) {
   // tab close button hidden and the newly-active tab should show
   // its tab close button.
   tab_strip_->SelectTab(tab2);
+  LayoutTabs();
   ASSERT_FALSE(tab4->IsActive());
   ASSERT_TRUE(tab2->IsActive());
   EXPECT_FALSE(tab0->showing_close_button_);
@@ -573,6 +583,7 @@ TEST_P(TabStripTest, TabCloseButtonVisibilityWhenNotStacked) {
   // After closing the active tab, the tab which becomes active should
   // show its tab close button.
   tab_strip_->CloseTab(tab2, CLOSE_TAB_FROM_TOUCH);
+  LayoutTabs();
   tab2 = nullptr;
   ASSERT_TRUE(tab3->IsActive());
   DoLayout();

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

@@ -50,7 +50,6 @@ class FakeTabController : public TabController {
     return LEADING;
   }
   bool ShouldHideCloseButtonForTab(Tab* tab) const override { return false; }
-  bool ShouldShowCloseButtonOnHover() override { return false; }
   bool MaySetClip() override { return false; }
   void SelectTab(Tab* tab) override {}
   void ExtendSelectionTo(Tab* tab) override {}
@@ -857,6 +856,7 @@ TEST_F(TabTest, ExtraLeftPaddingShownOnSiteWithoutFavicon) {
   TabRendererData data;
   data.show_icon = false;
   tab.SetData(data);
+  tab.Layout();
   EndTitleAnimation(&tab);
   EXPECT_FALSE(icon->visible());
   // Title should be placed where the favicon was.
@@ -947,6 +947,7 @@ TEST_F(AlertIndicatorTest, ShowsAndHidesAlertIndicator) {
   start_media.alert_state = TabAlertState::AUDIO_PLAYING;
   start_media.pinned = media_tab->data().pinned;
   media_tab->SetData(std::move(start_media));
+  media_tab->Layout();
 
   // When audio starts, pinned inactive tab shows indicator.
   EXPECT_FALSE(showing_icon(media_tab));
@@ -957,6 +958,7 @@ TEST_F(AlertIndicatorTest, ShowsAndHidesAlertIndicator) {
   stop_media.alert_state = TabAlertState::NONE;
   stop_media.pinned = media_tab->data().pinned;
   media_tab->SetData(std::move(stop_media));
+  media_tab->Layout();
 
   // When audio ends, pinned inactive tab fades out indicator.
   EXPECT_FALSE(showing_icon(media_tab));
@@ -967,6 +969,7 @@ TEST_F(AlertIndicatorTest, ShowsAndHidesAlertIndicator) {
   // out animation to stop, reach out and stop the fade animation directly,
   // to make sure that it updates the tab appropriately when it's done.
   StopAnimation(media_tab);
+  media_tab->Layout();
 
   EXPECT_TRUE(showing_icon(media_tab));
   EXPECT_FALSE(showing_alert_indicator(media_tab));