Remove layout during paint in Tab

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

 // Copyright (c) 2012 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_strip.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <iterator>
@@ skipping 710 matching lines @@
   }
   stacked_layout_ = stacked_layout;
   SetResetToShrinkOnExit(false);
   SwapLayoutIfNecessary();
   // When transitioning to stacked try to keep the active tab centered.
   if (touch_layout_ && active_index != -1) {
     touch_layout_->SetActiveTabLocation(
         active_center - ideal_bounds(active_index).width() / 2);
     AnimateToIdealBounds();
   }
+
+  for (int i = 0; i < tab_count(); ++i)
+    tab_at(i)->Layout();

[sky, 2016/01/12 22:29:11]

Can you elaborate as to why this is necessary (and add a comment too). In theory
if we do something interesting here, then SwapLayoutIfNecessary will trigger an
animation that will cause each tabs bounds to change and trigger a layout.

[enne (OOO), 2016/01/12 22:36:28]

Changing stacked layout can change whether or not buttons are visible. 
Animations seem somewhat of an implementation detail, but I don't know this
code.  Should the test just call layout, similar to how it was calling paint
before?  I'm not sure that makes the test very valuable, but maybe that's the
best approach.

[sky, 2016/01/12 22:41:02]

Yes, because the animation triggers layout.

 }
 
 gfx::Rect TabStrip::GetNewTabButtonBounds() {
   return newtab_button_->bounds();
 }
 
 bool TabStrip::SizeTabButtonToTopOfTabStrip() {
   // Extend the button to the screen edge in maximized and immersive fullscreen.
   views::Widget* widget = GetWidget();
   return browser_defaults::kSizeTabButtonToTopOfTabStrip ||
@@ skipping 395 matching lines @@
 
 bool TabStrip::ShouldHideCloseButtonForInactiveTabs() {
   if (!touch_layout_)
     return false;
 
   return !base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kDisableHideInactiveStackedTabCloseButtons);
 }
 
 void TabStrip::SelectTab(Tab* tab) {
+  int old_active = controller_->GetActiveIndex();

[sky, 2016/01/12 22:29:11]

How come TabStrip::SetSelection doesn't cover this?

[enne (OOO), 2016/01/12 22:36:29]

SelectTab and SetSelection are both public functions that do not call each
other.  The test calls SelectTab, and so SetSelection is not relevant here.

[sky, 2016/01/12 22:41:02]

SelectTab calls to the TabStripController, which should result in a call back to
SetSelection. Are you seeing otherwise? If it's from a test, then the test may
not be calling SetSelection as real code does.

[enne (OOO), 2016/01/12 22:54:54]

It does not appear to call SetSelection from the test.

[sky, 2016/01/13 00:41:09]

Please update the caller to do that then as that is the expectation of
Controller::SelectTab.

+
   int model_index = GetModelIndexOfTab(tab);
   if (IsValidModelIndex(model_index))
     controller_->SelectTab(model_index);
+
+  int new_active = controller_->GetActiveIndex();
+  if (new_active != old_active) {
+    tab_at(old_active)->ActiveStateChanged();
+    tab_at(new_active)->ActiveStateChanged();
+  }
 }
 
 void TabStrip::ExtendSelectionTo(Tab* tab) {
   int model_index = GetModelIndexOfTab(tab);
   if (IsValidModelIndex(model_index))
     controller_->ExtendSelectionTo(model_index);
 }
 
 void TabStrip::ToggleSelected(Tab* tab) {
   int model_index = GetModelIndexOfTab(tab);
@@ skipping 1697 matching lines @@
           ConvertPointToViewAndGetEventHandler(this, newtab_button_, point);
       if (view)
         return view;
     }
     Tab* tab = FindTabForEvent(point);
     if (tab)
       return ConvertPointToViewAndGetEventHandler(this, tab, point);
   }
   return this;
 }