LabelButton: cache the last computed preferred size

ui/views/controls/button/label_button.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 "ui/views/controls/button/label_button.h"
 
 #include "base/logging.h"
 #include "grit/ui_resources.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/animation/throb_animation.h"
@@ skipping 71 matching lines @@
   UpdateImage();
 }
 
 const base::string16& LabelButton::GetText() const {
   return label_->text();
 }
 
 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) {
   button_state_colors_[for_state] = color;
   if (for_state == STATE_DISABLED)
     label_->SetDisabledColor(color);
   else if (for_state == state())
     label_->SetEnabledColor(color);
   explicitly_set_colors_[for_state] = true;
 }
 
 void LabelButton::SetTextShadows(const gfx::ShadowValues& shadows) {
   label_->SetShadows(shadows);
 }
 
 void LabelButton::SetTextSubpixelRenderingEnabled(bool enabled) {
   label_->SetSubpixelRenderingEnabled(enabled);
 }
 
 bool LabelButton::GetTextMultiLine() const {
   return label_->multi_line();
 }
 
 void LabelButton::SetTextMultiLine(bool text_multi_line) {
   label_->SetMultiLine(text_multi_line);
+  ResetCachedSize();
 }
 
 const gfx::FontList& LabelButton::GetFontList() const {
   return label_->font_list();
 }
 
 void LabelButton::SetFontList(const gfx::FontList& font_list) {
   cached_normal_font_list_ = font_list;
   cached_bold_font_list_ = font_list.DeriveWithStyle(
       font_list.GetFontStyle() | gfx::Font::BOLD);
 
   // STYLE_BUTTON uses bold text to indicate default buttons.
   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 {
   return label_->GetHorizontalAlignment();
 }
 
 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;
   is_default_ = is_default;
   ui::Accelerator accel(ui::VKEY_RETURN, ui::EF_NONE);
   is_default_ ? AddAccelerator(accel) : RemoveAccelerator(accel);
 
   // STYLE_BUTTON uses bold text to indicate default buttons.
   if (style_ == STYLE_BUTTON) {
     label_->SetFontList(
         is_default ? cached_bold_font_list_ : cached_normal_font_list_);
   }
 }
 
 void LabelButton::SetStyle(ButtonStyle style) {
   style_ = style;
   // Inset the button focus rect from the actual border; roughly match Windows.
   if (style == STYLE_BUTTON) {
     SetFocusPainter(scoped_ptr<Painter>());
   } else {
     SetFocusPainter(Painter::CreateDashedFocusPainterWithInsets(
                         gfx::Insets(3, 3, 3, 3)));
   }
   if (style == STYLE_BUTTON) {
     label_->SetHorizontalAlignment(gfx::ALIGN_CENTER);
     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) {
   focus_painter_ = focus_painter.Pass();
 }
 
 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());
   label.SetMultiLine(GetTextMultiLine());
 
   if (style() == STYLE_BUTTON) {
     // Some text appears wider when rendered normally than when rendered bold.
     // Accommodate the widest, as buttons may show bold and shouldn't resize.
     const int current_width = label.GetPreferredSize().width();
     label.SetFontList(cached_bold_font_list_);
@@ skipping 15 matching lines @@
 
   // Increase the minimum size monotonically with the preferred size.
   size.SetToMax(min_size_);
   min_size_ = size;
 
   // Return the largest known size clamped to the maximum size (if valid).
   if (max_size_.width() > 0)
     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:

   w -= GetInsets().width();
   const gfx::Size image_size(image_->GetPreferredSize());
   w -= image_size.width();
   if (image_size.width() > 0 && !GetText().empty())
     w -= kSpacing;
 
   int height = std::max(image_size.height(), label_->GetHeightForWidth(w));
   if (border())
     height = std::max(height, border()->GetMinimumSize().height());
 
@@ skipping 51 matching lines @@
   return kViewClassName;
 }
 
 scoped_ptr<LabelButtonBorder> LabelButton::CreateDefaultBorder() const {
   return scoped_ptr<LabelButtonBorder>(new LabelButtonBorder(style_));
 }
 
 void LabelButton::SetBorder(scoped_ptr<Border> border) {
   border_is_themed_border_ = false;
   View::SetBorder(border.Pass());
+  ResetCachedSize();
 }
 
 gfx::Rect LabelButton::GetChildAreaBounds() {
   return GetLocalBounds();
 }
 
 void LabelButton::OnPaint(gfx::Canvas* canvas) {
   View::OnPaint(canvas);
   Painter::PaintFocusPainter(this, canvas, focus_painter_.get());
 }
@@ skipping 64 matching lines @@
   for (size_t state = STATE_NORMAL; state < STATE_COUNT; ++state) {
     if (!explicitly_set_colors_[state]) {
       SetTextColor(static_cast<ButtonState>(state), colors[state]);
       explicitly_set_colors_[state] = false;
     }
   }
 }
 
 void LabelButton::UpdateImage() {
   image_->SetImage(GetImage(state()));
+  ResetCachedSize();
 }
 
 void LabelButton::UpdateThemedBorder() {
   // Don't override borders set by others.
   if (!border_is_themed_border_)
     return;
 
   scoped_ptr<LabelButtonBorder> label_button_border = CreateDefaultBorder();
 
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
@@ skipping 15 matching lines @@
   UpdateImage();
   const SkColor color = button_state_colors_[state()];
   if (state() != STATE_DISABLED && label_->enabled_color() != color)
     label_->SetEnabledColor(color);
   label_->SetEnabled(state() != STATE_DISABLED);
   if (image_->GetPreferredSize() != previous_image_size)
     Layout();
 }
 
 void LabelButton::ChildPreferredSizeChanged(View* child) {
+  ResetCachedSize();
   PreferredSizeChanged();
 }
 
 void LabelButton::OnNativeThemeChanged(const ui::NativeTheme* theme) {
   ResetColorsFromNativeTheme();
   UpdateThemedBorder();
   // Invalidate the layout to pickup the new insets from the border.
   InvalidateLayout();
 }
 
@@ skipping 27 matching lines @@
   GetExtraParams(params);
   return ui::NativeTheme::kNormal;
 }
 
 ui::NativeTheme::State LabelButton::GetForegroundThemeState(
     ui::NativeTheme::ExtraParams* params) const {
   GetExtraParams(params);
   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