LabelButton: cache the last computed preferred size

ui/views/controls/button/label_button.cc

Index: ui/views/controls/button/label_button.cc
diff --git a/ui/views/controls/button/label_button.cc b/ui/views/controls/button/label_button.cc
index 5ee97918dc9f794c1d1189ffa3e570449e6c16f7..882420cde935ad9271145b71a55d7b161f077b03 100644
--- a/ui/views/controls/button/label_button.cc
+++ b/ui/views/controls/button/label_button.cc
@@ -89,6 +89,7 @@ const base::string16& LabelButton::GetText() const {
 void LabelButton::SetText(const base::string16& text) {
   SetAccessibleName(text);
   label_->SetText(text);
+  ResetCachedSize();

[msw, 2014/07/23 19:20:16]

Ideally, changing child Label view's text should trigger
ChildPreferredSizeChanged here, making this call unnecessary. It'd be great to
have unit tests for such behavior, and to weed out unnecessary explicit
ResetCachedSize calls. I won't block the CL on that condition, but I would
greatly appreciate your help with that sort of followup work.

[noms (inactive), 2014/07/24 14:20:25]

I removed it from here and from all the other places in which Label triggers an
update.

On 2014/07/23 19:20:16, msw wrote:

 }
 
 void LabelButton::SetTextColor(ButtonState for_state, SkColor color) {
@@ -114,6 +115,7 @@ bool LabelButton::GetTextMultiLine() const {
 
 void LabelButton::SetTextMultiLine(bool text_multi_line) {
   label_->SetMultiLine(text_multi_line);
+  ResetCachedSize();
 }
 
 const gfx::FontList& LabelButton::GetFontList() const {
@@ -129,10 +131,12 @@ void LabelButton::SetFontList(const gfx::FontList& font_list) {
   label_->SetFontList(
       style_ == STYLE_BUTTON && is_default_ ?
       cached_bold_font_list_ : cached_normal_font_list_);
+  ResetCachedSize();
 }
 
 void LabelButton::SetElideBehavior(gfx::ElideBehavior elide_behavior) {
   label_->SetElideBehavior(elide_behavior);
+  ResetCachedSize();
 }
 
 gfx::HorizontalAlignment LabelButton::GetHorizontalAlignment() const {
@@ -141,9 +145,20 @@ gfx::HorizontalAlignment LabelButton::GetHorizontalAlignment() const {
 
 void LabelButton::SetHorizontalAlignment(gfx::HorizontalAlignment alignment) {
   label_->SetHorizontalAlignment(alignment);
+  ResetCachedSize();
   InvalidateLayout();
 }
 
+void LabelButton::SetMinSize(const gfx::Size& min_size) {
+  min_size_ = min_size;
+  ResetCachedSize();
+}
+
+void LabelButton::SetMaxSize(const gfx::Size& max_size) {
+  max_size_ = max_size;
+  ResetCachedSize();
+}
+
 void LabelButton::SetIsDefault(bool is_default) {
   if (is_default == is_default_)
     return;
@@ -172,9 +187,9 @@ void LabelButton::SetStyle(ButtonStyle style) {
     SetFocusable(true);
   }
   if (style == STYLE_BUTTON)
-    set_min_size(gfx::Size(70, 33));
-
+    SetMinSize(gfx::Size(70, 33));
   OnNativeThemeChanged(GetNativeTheme());
+  ResetCachedSize();
 }
 
 void LabelButton::SetFocusPainter(scoped_ptr<Painter> focus_painter) {
@@ -182,6 +197,9 @@ void LabelButton::SetFocusPainter(scoped_ptr<Painter> focus_painter) {
 }
 
 gfx::Size LabelButton::GetPreferredSize() const {
+  if (button_size_valid_)
+    return button_size_;
+
   // Use a temporary label copy for sizing to avoid calculation side-effects.
   Label label(GetText(), cached_normal_font_list_);
   label.SetShadows(label_->shadows());
@@ -217,7 +235,11 @@ gfx::Size LabelButton::GetPreferredSize() const {
     size.set_width(std::min(max_size_.width(), size.width()));
   if (max_size_.height() > 0)
     size.set_height(std::min(max_size_.height(), size.height()));
-  return size;
+
+  // Cache this computed size, as recomputing it is an expensive operation.
+  button_size_valid_ = true;
+  button_size_ = size;
+  return button_size_;
 }
 
 int LabelButton::GetHeightForWidth(int w) const {

[msw, 2014/07/23 19:20:16]

I wonder if we should similarly cache the preferred heights of a limited number
of argument widths, I bet this is used heavily by some layout managers. You
don't need to bother with that in this CL, but followup would be appreciated.

[noms (inactive), 2014/07/24 14:20:25]

Oh, that's a neat idea. I could help with that :)

On 2014/07/23 19:20:16, msw wrote:

@@ -292,6 +314,7 @@ scoped_ptr<LabelButtonBorder> LabelButton::CreateDefaultBorder() const {
 void LabelButton::SetBorder(scoped_ptr<Border> border) {
   border_is_themed_border_ = false;
   View::SetBorder(border.Pass());
+  ResetCachedSize();
 }
 
 gfx::Rect LabelButton::GetChildAreaBounds() {
@@ -376,6 +399,7 @@ void LabelButton::ResetColorsFromNativeTheme() {
 
 void LabelButton::UpdateImage() {
   image_->SetImage(GetImage(state()));
+  ResetCachedSize();
 }
 
 void LabelButton::UpdateThemedBorder() {
@@ -411,6 +435,7 @@ void LabelButton::StateChanged() {
 }
 
 void LabelButton::ChildPreferredSizeChanged(View* child) {
+  ResetCachedSize();
   PreferredSizeChanged();
 }
 
@@ -458,4 +483,9 @@ ui::NativeTheme::State LabelButton::GetForegroundThemeState(
   return ui::NativeTheme::kHovered;
 }
 
+void LabelButton::ResetCachedSize() {

[msw, 2014/07/23 19:20:16]

I wonder if this should call PreferredSizeChanged(), but you shouldn't add that
in this CL; it wasn't done before and can be investigated later if needed.

[noms (inactive), 2014/07/24 14:20:25]

Acknowledged.

+  button_size_valid_ = false;
+  button_size_= gfx::Size();
+}
+
 }  // namespace views