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 62 matching lines @@
 
 const gfx::ImageSkia& LabelButton::GetImage(ButtonState for_state) {
   if (for_state != STATE_NORMAL && button_state_images_[for_state].isNull())
     return button_state_images_[STATE_NORMAL];
   return button_state_images_[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 {
   return label_->text();
 }
 
 void LabelButton::SetText(const base::string16& text) {
   SetAccessibleName(text);
   label_->SetText(text);
+  ResetCachedSize();
 }
 
 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_->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) {
   label_->set_subpixel_rendering_enabled(enabled);
 }
 
 bool LabelButton::GetTextMultiLine() const {
   return label_->is_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);

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

I don't think the alignment should generally change the preferred size, but it
might be safe to clear it here.

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

Done.

   InvalidateLayout();
 }
 
 void LabelButton::SetDirectionalityMode(gfx::DirectionalityMode mode) {
   label_->set_directionality_mode(mode);

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

The directionality could possibly change the preferred size (multi-line BiDi
text may need to break into more lines), so clear it here.

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

This went away after msw's CL.

 }
 
 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) {

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

Setting the style will definitely change the preferred size, (generally by
changing the border, thus yielding new insets), maybe clearing the size on
SetBorder would be sufficient, but I'm not sure offhand.

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

Done.

   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));
 
   OnNativeThemeChanged(GetNativeTheme());
 }
 
 void LabelButton::SetFocusPainter(scoped_ptr<Painter> focus_painter) {
   focus_painter_ = focus_painter.Pass();
 }
 
 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 (!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_);
+    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;
+
+    // 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;
   }
-
-  // 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()));
-  return size;
+  return button_size_;
 }
 
 void LabelButton::Layout() {
   gfx::HorizontalAlignment adjusted_alignment = GetHorizontalAlignment();
   if (base::i18n::IsRTL() && adjusted_alignment != gfx::ALIGN_CENTER)
     adjusted_alignment = (adjusted_alignment == gfx::ALIGN_LEFT) ?
         gfx::ALIGN_RIGHT : gfx::ALIGN_LEFT;
 
   gfx::Rect child_area(GetChildAreaBounds());
   child_area.Inset(GetInsets());
@@ skipping 36 matching lines @@
 }
 
 const char* LabelButton::GetClassName() const {
   return kViewClassName;
 }
 
 scoped_ptr<LabelButtonBorder> LabelButton::CreateDefaultBorder() const {
   return scoped_ptr<LabelButtonBorder>(new LabelButtonBorder(style_));
 }
 
 void LabelButton::SetBorder(scoped_ptr<Border> border) {

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

SetBorder may change the insets and thus the preferred size.

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

Done.

   border_is_themed_border_ = false;
   View::SetBorder(border.Pass());
 }
 
 gfx::Rect LabelButton::GetChildAreaBounds() {
   return GetLocalBounds();
 }
 
 void LabelButton::OnPaint(gfx::Canvas* canvas) {
   View::OnPaint(canvas);
@@ skipping 64 matching lines @@
     colors[STATE_HOVERED] = colors[STATE_PRESSED] = colors[STATE_NORMAL];
 
   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() {

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

Updating the image may change its preferred size, and should trigger
ChildPreferredSizeChanged here, which should clear the preferred size, as I also
describe in my StateChanged comment.

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

Acknowledged.

   image_->SetImage(GetImage(state()));
 }
 
 void LabelButton::UpdateThemedBorder() {

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

Looks like clearing on SetBorder would be sufficient for this.

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

Acknowledged.

   // 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)
   views::LinuxUI* linux_ui = views::LinuxUI::instance();
   if (linux_ui) {
     SetBorder(linux_ui->CreateNativeBorder(
         this, label_button_border.Pass()));
   } else
 #endif
   {
     SetBorder(label_button_border.PassAs<Border>());
   }
 
   border_is_themed_border_ = true;
 }
 
 void LabelButton::StateChanged() {
   const gfx::Size previous_image_size(image_->GetPreferredSize());
   UpdateImage();

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

Updating the image may change the preferred size (if there's a difference in the
image preferred sizes). We explicitly handle that case with a Layout call below,
but ImageView::SetImage calls its own PreferredSizeChanged, which should call
ChildPreferredSizeChanged here, and that should probably perform or trigger
[Invalidate]Layout instead... This isn't entirely in the scope of this CL, but
you'll at least want to clear the preferred size in ChildPreferredSizeChanged.

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

Having gone down this rabbit hole int the past, I've found that when you
automatically call Layout in places like ChildPreferredSizeChanged you can
easily wind up doing an excessive number of layouts. This is why Views relies on
explicit, strategically placed Layout calls.

   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) {

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

This should definitely clear the cached preferred size.

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

Done.

   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() {
+  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