Fixed StyledLabel size caching

ui/views/controls/styled_label.cc

 // Copyright 2013 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/styled_label.h"
 
 #include <vector>
 
 #include "base/strings/string_util.h"
 #include "ui/gfx/font_list.h"
@@ skipping 79 matching lines @@
 }
 
 
 // StyledLabel ----------------------------------------------------------------
 
 StyledLabel::StyledLabel(const base::string16& text,
                          StyledLabelListener* listener)
     : specified_line_height_(0),
       listener_(listener),
       displayed_on_background_color_set_(false),
-      auto_color_readability_enabled_(true) {
+      auto_color_readability_enabled_(true),
+      layout_built_for_width_(0) {
   base::TrimWhitespace(text, base::TRIM_TRAILING, &text_);
 }
 
 StyledLabel::~StyledLabel() {}
 
 void StyledLabel::SetText(const base::string16& text) {
   text_ = text;
   style_ranges_.clear();
   RemoveAllChildViews(true);
   PreferredSizeChanged();
@@ skipping 22 matching lines @@
   default_style_info_ = style_info;
   PreferredSizeChanged();
 }
 
 void StyledLabel::SetLineHeight(int line_height) {
   specified_line_height_ = line_height;
   PreferredSizeChanged();
 }
 
 void StyledLabel::SetDisplayedOnBackgroundColor(SkColor color) {
+  if (displayed_on_background_color_ == color)
+    return;
+
   displayed_on_background_color_ = color;
   displayed_on_background_color_set_ = true;
+
+  for (int i = 0, count = child_count(); i < count; ++i) {
+    static_cast<Label*>(child_at(i))->

[sky, 2014/12/11 15:48:54]

DCHECK the class name of the child is Label::kViewClassName.

[edjomin, 2014/12/12 08:58:23]

Done.

+        SetBackgroundColor(displayed_on_background_color_);
+  }
 }
 
 gfx::Insets StyledLabel::GetInsets() const {
   gfx::Insets insets = View::GetInsets();
 
   // We need a focus border iff we contain a link that will have a focus border.
   // That in turn will be true only if the link is non-empty.
   for (StyleRanges::const_iterator i(style_ranges_.begin());
         i != style_ranges_.end(); ++i) {
     if (i->style_info.is_link && !i->range.is_empty()) {
       const gfx::Insets focus_border_padding(
           Label::kFocusBorderPadding, Label::kFocusBorderPadding,
           Label::kFocusBorderPadding, Label::kFocusBorderPadding);
       insets += focus_border_padding;
       break;
     }
   }
 
   return insets;
 }
 
 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);
-  }
+  // 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.

[sky, 2014/12/11 15:48:53]

Update your comment, and make sure you capitalize first letters and end in
periods. See other comments in Chrome for what I mean.

[edjomin, 2014/12/12 08:58:23]

Done.

[edjomin, 2014/12/12 08:58:23]

Removed this comment because it's useless now, sorry.

+  // calculated_size_ should be cached for non-dry calls only,
+  // because they create controls alongside with size calculation
+  // otherwise all caching is useless
+  calculated_size_ =
+      const_cast<StyledLabel*>(this)->CalculateAndDoLayout(w, true);
   return calculated_size_.height();
 }
 
 void StyledLabel::Layout() {
   calculated_size_ = CalculateAndDoLayout(GetLocalBounds().width(), false);
+  layout_built_for_width_ = calculated_size_.width();
 }
 
 void StyledLabel::PreferredSizeChanged() {
   calculated_size_ = gfx::Size();
   View::PreferredSizeChanged();
 }
 
 void StyledLabel::LinkClicked(Link* source, int event_flags) {
   if (listener_)
     listener_->StyledLabelLinkClicked(link_targets_[source], event_flags);
 }
 
 gfx::Size StyledLabel::CalculateAndDoLayout(int width, bool dry_run) {
+  width -= GetInsets().width();
+  if (width == calculated_size_.width() &&
+      (dry_run || layout_built_for_width_ == width))
+    return calculated_size_;
+
   if (!dry_run) {
     RemoveAllChildViews(true);
     link_targets_.clear();
   }
 
-  width -= GetInsets().width();
   if (width <= 0 || text_.empty())
     return gfx::Size();
 
   const int line_height = specified_line_height_ > 0 ? specified_line_height_
       : CalculateLineHeight(font_list_);
   // The index of the line we're on.
   int line = 0;
   // The x position (in pixels) of the line we're on, relative to content
   // bounds.
   int x = 0;
@@ skipping 107 matching lines @@
   }
 
   // The user-specified line height only applies to interline spacing, so the
   // final line's height is unaffected.
   int total_height = line * line_height +
       CalculateLineHeight(font_list_) + GetInsets().height();
   return gfx::Size(width, total_height);
 }
 
 }  // namespace views