Fixed StyledLabel size caching

ui/views/controls/styled_label.cc

Index: ui/views/controls/styled_label.cc
diff --git a/ui/views/controls/styled_label.cc b/ui/views/controls/styled_label.cc
index 700e62a392115963ae77adb8abb4a2fb60aa0205..5c93e5bc6558317bde502842b9834893173e8a38 100644
--- a/ui/views/controls/styled_label.cc
+++ b/ui/views/controls/styled_label.cc
@@ -142,6 +142,7 @@ void StyledLabel::SetLineHeight(int line_height) {
 void StyledLabel::SetDisplayedOnBackgroundColor(SkColor color) {
   displayed_on_background_color_ = color;
   displayed_on_background_color_set_ = true;
+  calculated_size_ = gfx::Size();  // force layout recalculation

[sky, 2014/11/18 18:30:03]

This comment is a bit wrong. It's not a layout calculation you need, but
recreating the children, right?

[edjomin, 2014/11/19 17:04:00]

Yes, you are right. I reset cached size here to force next call to Layout()
recreate all controls. I'll change the comment

 }
 
 gfx::Insets StyledLabel::GetInsets() const {
@@ -164,15 +165,16 @@ gfx::Insets StyledLabel::GetInsets() const {
 }
 
 int StyledLabel::GetHeightForWidth(int w) const {
-  if (w != calculated_size_.width()) {
-    // TODO(erg): Munge the const-ness of the style label. CalculateAndDoLayout
-    // doesn't actually make any changes to member variables when |dry_run| is
-    // set to true. In general, the mutating and non-mutating parts shouldn't
-    // be in the same codepath.
-    calculated_size_ =
-        const_cast<StyledLabel*>(this)->CalculateAndDoLayout(w, true);
-  }
-  return calculated_size_.height();
+  // TODO(erg): Munge the const-ness of the style label. CalculateAndDoLayout
+  // doesn't actually make any changes to member variables when |dry_run| is
+  // set to true. In general, the mutating and non-mutating parts shouldn't
+  // be in the same codepath.
+
+  // do not store calculated_size_ for dry_run call.
+  // calculated_size_ should be cached for non-dry calls only,
+  // because they create controls alongside with size calculation
+  // otherwise all caching is useless
+  return const_cast<StyledLabel*>(this)->CalculateAndDoLayout(w, true).height();

[sky, 2014/11/18 18:30:03]

I think we should continue caching here, but make Layout() do a layout if the
size changes.

[edjomin, 2014/11/19 17:04:00]

This was my first idea but it doesn't work this way.  If I cache size here next
call to Layout will not create controls because size remains the same. If I
force controls recreation in Layout() caching becomes useless. I thought this
solution would be better than adding additional member variables. Which solution
is preferable?

Let me describe situation in which I encountered this issue.
I have view with GridLayout that contains StyledLabel. First of all I get one or
two calls to GetHeightForWidth issued by GridLayout when it calculates row
sizes. Then I get series of 4 calls to Layout from different sources. Size is
not changing but each call to Layout removes all child controls and creates them
again. So view appears with noticeable delay.

[sky, 2014/11/19 19:05:51]

Layout and sizing for this class is expensive, so it's likely worth optimizing
both.

I get what you're saying. I'm suggesting you cache both. The last width Layout()
was done at, and calculated_size_ for the last preferred size that was
calculated.

 }
 
 void StyledLabel::Layout() {
@@ -190,12 +192,15 @@ void StyledLabel::LinkClicked(Link* source, int event_flags) {
 }
 
 gfx::Size StyledLabel::CalculateAndDoLayout(int width, bool dry_run) {
+  width -= GetInsets().width();
+  if (width == calculated_size_.width())
+    return calculated_size_;
+
   if (!dry_run) {
     RemoveAllChildViews(true);
     link_targets_.clear();
   }
 
-  width -= GetInsets().width();
   if (width <= 0 || text_.empty())
     return gfx::Size();