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 fa358c1c9d3d78e5370eb7be9573ac91cc8c9dfb..68b10c72e783cf5a9f67b6a56860948fb3e83457 100644
--- a/ui/views/controls/button/label_button.cc
+++ b/ui/views/controls/button/label_button.cc
@@ -80,6 +80,7 @@ const gfx::ImageSkia& LabelButton::GetImage(ButtonState for_state) {
 void LabelButton::SetImage(ButtonState for_state, const gfx::ImageSkia& image) {
   button_state_images_[for_state] = image;
   UpdateImage();
+  ResetCachedSize();

[msw, 2014/07/08 00:24:03]

I think this call might be unnecessary if we have one in
ChildPreferredSizeChanged. See my comments in StateChanged and UpdateImage.

[noms (inactive), 2014/07/21 15:40:43]

Done.

 }
 
 const base::string16& LabelButton::GetText() const {
@@ -89,6 +90,7 @@ const base::string16& LabelButton::GetText() const {
 void LabelButton::SetText(const base::string16& text) {
   SetAccessibleName(text);
   label_->SetText(text);
+  ResetCachedSize();
 }
 
 void LabelButton::SetTextColor(ButtonState for_state, SkColor color) {
@@ -102,6 +104,7 @@ void LabelButton::SetTextColor(ButtonState for_state, SkColor color) {
 
 void LabelButton::SetTextShadows(const gfx::ShadowValues& shadows) {
   label_->set_shadows(shadows);
+  ResetCachedSize();

[Evan Stade, 2014/07/08 18:23:08]

do shadows really affect the size of the button? that would surprise me

[noms (inactive), 2014/07/21 15:40:43]

Done.

 }
 
 void LabelButton::SetTextSubpixelRenderingEnabled(bool enabled) {
@@ -114,6 +117,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 +133,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 {
@@ -186,47 +192,53 @@ void LabelButton::SetFocusPainter(scoped_ptr<Painter> focus_painter) {
 }
 
 gfx::Size LabelButton::GetPreferredSize() const {
-  // Use a temporary label copy for sizing to avoid calculation side-effects.
-  Label label(GetText(), cached_normal_font_list_);
-  label.set_shadows(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_);
-    if (label.GetPreferredSize().width() < current_width)
-      label.SetFontList(cached_normal_font_list_);
-  }
-
-  // Resize multi-line labels given the current limited available width.
-  const gfx::Size image_size(image_->GetPreferredSize());
-  const int image_width = image_size.width();
-  if (GetTextMultiLine() && (width() > image_width + kSpacing))
-    label.SizeToFit(width() - image_width - (image_width > 0 ? kSpacing : 0));
-
-  // Calculate the required size.
-  gfx::Size size(label.GetPreferredSize());
-  if (image_width > 0 && size.width() > 0)
-    size.Enlarge(kSpacing, 0);
-  size.SetToMax(gfx::Size(0, image_size.height()));
-  const gfx::Insets insets(GetInsets());
-  size.Enlarge(image_size.width() + insets.width(), insets.height());
-
-  // Make the size at least as large as the minimum size needed by the border.
-  size.SetToMax(border() ? border()->GetMinimumSize() : gfx::Size());
-
-  // Increase the minimum size monotonically with the preferred size.
-  size.SetToMax(min_size_);
-  min_size_ = size;
+  if (!button_size_valid_) {

[msw, 2014/07/08 00:24:03]

nit: reverse condition and early return instead.

[noms (inactive), 2014/07/21 15:40:43]

Done.

+    // Use a temporary label copy for sizing to avoid calculation side-effects.
+    Label label(GetText(), cached_normal_font_list_);
+    label.set_shadows(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_);
+      if (label.GetPreferredSize().width() < current_width)
+        label.SetFontList(cached_normal_font_list_);
+    }
 
-  // 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;
+    // Resize multi-line labels given the current limited available width.
+    const gfx::Size image_size(image_->GetPreferredSize());
+    const int image_width = image_size.width();
+    if (GetTextMultiLine() && (width() > image_width + kSpacing))
+      label.SizeToFit(width() - image_width - (image_width > 0 ? kSpacing : 0));
+
+    // Calculate the required size.
+    gfx::Size size(label.GetPreferredSize());
+    if (image_width > 0 && size.width() > 0)
+      size.Enlarge(kSpacing, 0);
+    size.SetToMax(gfx::Size(0, image_size.height()));
+    const gfx::Insets insets(GetInsets());
+    size.Enlarge(image_size.width() + insets.width(), insets.height());
+
+    // Make the size at least as large as the minimum size needed by the border.
+    size.SetToMax(border() ? border()->GetMinimumSize() : gfx::Size());
+
+    // 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()));
+
+    // Cache this computed size, as recomputing it is an expensive operation.
+    button_size_valid_ = true;
+    button_size_ = size;
+  }
+  return button_size_;
 }
 
 void LabelButton::Layout() {
@@ -452,4 +464,9 @@ ui::NativeTheme::State LabelButton::GetForegroundThemeState(
   return ui::NativeTheme::kHovered;
 }
 
+void LabelButton::ResetCachedSize() {
+  button_size_valid_ = false;

[msw, 2014/07/08 00:24:04]

nit: avoid the flag and use empty as an invalid size; then also DCHECK that the
actual preferred size is non-empty in GetPreferredSize.

[noms (inactive), 2014/07/21 15:40:43]

Isn't it possible that a LabelButton would legitimately have a 0 size, though?
In which case we shouldn't recompute it if we don't have to? (I may be
overthinking this)

On 2014/07/08 00:24:04, msw wrote:

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