Reduce the number of text reshaping in RenderText

ui/gfx/render_text.cc

Index: ui/gfx/render_text.cc
diff --git a/ui/gfx/render_text.cc b/ui/gfx/render_text.cc
index 50d29e83fe14585dc6002bc9b2a502b49d5c0b31..51e1a2bb290a1dd96a0cfdc43462ca36ac9775e8 100644
--- a/ui/gfx/render_text.cc
+++ b/ui/gfx/render_text.cc
@@ -13,6 +13,7 @@
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/trace_event/trace_event.h"
 #include "third_party/icu/source/common/unicode/rbbi.h"
 #include "third_party/icu/source/common/unicode/utf16.h"
 #include "third_party/skia/include/core/SkTypeface.h"
@@ -444,7 +445,7 @@ void RenderText::SetText(const base::string16& text) {
     text_direction_ = base::i18n::UNKNOWN_DIRECTION;
 
   obscured_reveal_index_ = -1;
-  UpdateLayoutText();
+  OnTextAttributeChanged();
 }
 
 void RenderText::SetHorizontalAlignment(HorizontalAlignment alignment) {
@@ -463,7 +464,7 @@ void RenderText::SetFontList(const FontList& font_list) {
   SetStyle(UNDERLINE, (font_style & gfx::Font::UNDERLINE) != 0);
   baseline_ = kInvalidBaseline;
   cached_bounds_and_offset_valid_ = false;
-  ResetLayout();
+  OnLayoutTextAttributeChanged(false);
 }
 
 void RenderText::SetCursorEnabled(bool cursor_enabled) {
@@ -481,7 +482,7 @@ void RenderText::SetObscured(bool obscured) {
     obscured_ = obscured;
     obscured_reveal_index_ = -1;
     cached_bounds_and_offset_valid_ = false;
-    UpdateLayoutText();
+    OnTextAttributeChanged();
   }
 }
 
@@ -491,13 +492,13 @@ void RenderText::SetObscuredRevealIndex(int index) {
 
   obscured_reveal_index_ = index;
   cached_bounds_and_offset_valid_ = false;
-  UpdateLayoutText();
+  OnTextAttributeChanged();
 }
 
 void RenderText::SetReplaceNewlineCharsWithSymbols(bool replace) {
   replace_newline_chars_with_symbols_ = replace;
   cached_bounds_and_offset_valid_ = false;
-  UpdateLayoutText();
+  OnTextAttributeChanged();
 }
 
 void RenderText::SetMultiline(bool multiline) {
@@ -505,6 +506,7 @@ void RenderText::SetMultiline(bool multiline) {
     multiline_ = multiline;
     cached_bounds_and_offset_valid_ = false;
     lines_.clear();
+    OnDisplayTextAttributeChanged();
   }
 }
 
@@ -514,13 +516,14 @@ void RenderText::SetMinLineHeight(int line_height) {
   min_line_height_ = line_height;
   cached_bounds_and_offset_valid_ = false;
   lines_.clear();
+  OnDisplayTextAttributeChanged();
 }
 
 void RenderText::SetElideBehavior(ElideBehavior elide_behavior) {
   // TODO(skanuj) : Add a test for triggering layout change.
   if (elide_behavior_ != elide_behavior) {
     elide_behavior_ = elide_behavior;
-    UpdateLayoutText();
+    OnDisplayTextAttributeChanged();
   }
 }
 
@@ -530,8 +533,10 @@ void RenderText::SetDisplayRect(const Rect& r) {
     baseline_ = kInvalidBaseline;
     cached_bounds_and_offset_valid_ = false;
     lines_.clear();
-    if (elide_behavior_ != NO_ELIDE)
-      UpdateLayoutText();
+    if (elide_behavior_ != NO_ELIDE &&
+        elide_behavior_ != FADE_TAIL) {
+      OnDisplayTextAttributeChanged();
+    }
   }
 }
 
@@ -657,7 +662,7 @@ void RenderText::SetCompositionRange(const Range& composition_range) {
         Range(0, text_.length()).Contains(composition_range));
   composition_range_.set_end(composition_range.end());
   composition_range_.set_start(composition_range.start());
-  ResetLayout();
+  OnLayoutTextAttributeChanged(false);

[msw, 2015/02/13 05:53:34]

nit: Maybe add a TODO comment here related to my doc suggestion. Something like:
"Altering composition underlines shouldn't require layout changes. It's
currently necessary because RenderTextHarfBuzz paints text decorations by run,
and RenderTextMac applies all styles during layout."

[oshima, 2015/02/13 21:03:09]

Done.

 }
 
 void RenderText::SetColor(SkColor value) {
@@ -672,7 +677,7 @@ void RenderText::SetStyle(TextStyle style, bool value) {
   styles_[style].SetValue(value);
 
   cached_bounds_and_offset_valid_ = false;
-  ResetLayout();
+  OnLayoutTextAttributeChanged(false);

[msw, 2015/02/13 05:53:34]

nit: Maybe add a TODO comment here related to my doc suggestion. Something like:
"Applying text decorations (strike, diagonal strike, and underline) shouldn't
require layout changes. It's currently necessary because RenderTextMac applies
all styles during layout."

[oshima, 2015/02/13 21:03:09]

Setting BOLD type changes the layout. See RenderTextTest.StringSizeBoldWidth for
example.

[msw, 2015/02/13 21:49:22]

I realize that, but the styles I mentioned (strike, diagonal strike, and
underline) should not change the layout.

 }
 
 void RenderText::ApplyStyle(TextStyle style, bool value, const Range& range) {
@@ -684,7 +689,7 @@ void RenderText::ApplyStyle(TextStyle style, bool value, const Range& range) {
   styles_[style].ApplyValue(value, Range(start, end));
 
   cached_bounds_and_offset_valid_ = false;
-  ResetLayout();
+  OnLayoutTextAttributeChanged(false);

[msw, 2015/02/13 05:53:34]

nit: Maybe add a TODO comment here related to my doc suggestion. Something like:
"Applying text decorations (strike, diagonal strike, and underline) shouldn't
require layout changes. It's currently necessary because RenderTextMac applies
all styles during layout."

[oshima, 2015/02/13 21:03:09]

same here.

 }
 
 bool RenderText::GetStyle(TextStyle style) const {
@@ -699,7 +704,7 @@ void RenderText::SetDirectionalityMode(DirectionalityMode mode) {
   directionality_mode_ = mode;
   text_direction_ = base::i18n::UNKNOWN_DIRECTION;
   cached_bounds_and_offset_valid_ = false;
-  ResetLayout();
+  OnLayoutTextAttributeChanged(false);
 }
 
 base::i18n::TextDirection RenderText::GetTextDirection() {
@@ -931,6 +936,7 @@ RenderText::RenderText()
       obscured_reveal_index_(-1),
       truncate_length_(0),
       elide_behavior_(NO_ELIDE),
+      text_elided_(false),
       replace_newline_chars_with_symbols_(true),
       min_line_height_(0),
       multiline_(false),
@@ -967,8 +973,27 @@ void RenderText::SetSelectionModel(const SelectionModel& model) {
   cached_bounds_and_offset_valid_ = false;
 }
 
-const base::string16& RenderText::GetLayoutText() const {
-  return layout_text_;
+void RenderText::UpdateDisplayText(float text_width) {
+  if (multiline_ ||

[msw, 2015/02/13 05:53:34]

Add a TODO here to support eliding multi-line text for views::Label, etc.

[oshima, 2015/02/13 21:03:09]

Done.

+      elide_behavior() == NO_ELIDE ||
+      elide_behavior() == FADE_TAIL ||
+      text_width < display_rect_.width() ||
+      layout_text_.empty()) {
+    text_elided_ = false;
+    display_text_.clear();
+    return;
+  }
+
+  // This doesn't trim styles so ellipsis may get rendered as a different
+  // style than the preceding text. See crbug.com/327850.
+  display_text_.assign(Elide(layout_text_,
+                             text_width,
+                             static_cast<float>(display_rect_.width()),
+                            elide_behavior_));

[msw, 2015/02/13 05:53:34]

nit: indent one space

[oshima, 2015/02/13 21:03:09]

Done.

+
+  text_elided_ = display_text_ != layout_text_;
+  if (!text_elided_)
+    display_text_.clear();
 }
 
 const BreakList<size_t>& RenderText::GetLineBreaks() {
@@ -1174,8 +1199,9 @@ void RenderText::MoveCursorTo(size_t position, bool select) {
         (cursor == 0) ? CURSOR_FORWARD : CURSOR_BACKWARD));
 }
 
-void RenderText::UpdateLayoutText() {
+void RenderText::OnTextAttributeChanged() {
   layout_text_.clear();
+  display_text_.clear();
   line_breaks_.SetMax(0);
 
   if (obscured_) {
@@ -1224,33 +1250,27 @@ void RenderText::UpdateLayoutText() {
     }
   }
 
-  if (elide_behavior_ != NO_ELIDE &&
-      elide_behavior_ != FADE_TAIL &&
-      !layout_text_.empty() &&
-      GetContentWidth() > display_rect_.width()) {
-    // This doesn't trim styles so ellipsis may get rendered as a different
-    // style than the preceding text. See crbug.com/327850.
-    layout_text_.assign(Elide(layout_text_,
-                              static_cast<float>(display_rect_.width()),
-                              elide_behavior_));
-  }
-
   // Replace the newline character with a newline symbol in single line mode.

[msw, 2015/02/13 22:19:05]

DO NOT REMOVE THIS! It's okay to remove |replace_newline_chars_with_symbols_|,
since that is always true and never modified by a client. ReplaceChars *should*
be run for single-line text, but not multiline text... leave this code and
change the if statement to "if (!multiline_)"

[oshima, 2015/02/13 23:24:11]

Done. I think I should have done this in a separate CL. It's getting harder to
keep thing in sync.

   static const base::char16 kNewline[] = { '\n', 0 };
   static const base::char16 kNewlineSymbol[] = { 0x2424, 0 };
   if (!multiline_ && replace_newline_chars_with_symbols_)
     base::ReplaceChars(layout_text_, kNewline, kNewlineSymbol, &layout_text_);
 
-  ResetLayout();
+  OnLayoutTextAttributeChanged(true);
 }
 
 base::string16 RenderText::Elide(const base::string16& text,
+                                 float text_width,
                                  float available_width,
                                  ElideBehavior behavior) {
   if (available_width <= 0 || text.empty())
     return base::string16();
   if (behavior == ELIDE_EMAIL)
     return ElideEmail(text, available_width);
+  if (text_width && text_width < available_width)

[msw, 2015/02/13 05:53:34]

nit: explicitly check "text_width > 0"

[oshima, 2015/02/13 21:03:09]

Done.

+    return text;
+
+  TRACE_EVENT0("ui", "RenderText::Elide");
 
   // Create a RenderText copy with attributes that affect the rendering width.
   scoped_ptr<RenderText> render_text = CreateInstanceOfSameType();
@@ -1260,21 +1280,26 @@ base::string16 RenderText::Elide(const base::string16& text,
   render_text->set_truncate_length(truncate_length_);
   render_text->styles_ = styles_;
   render_text->colors_ = colors_;
-  render_text->SetText(text);
-  if (render_text->GetContentWidthF() <= available_width)
+  if (text_width == 0) {
+    render_text->SetText(text);
+    text_width = render_text->GetContentWidthF();
+  }
+  if (text_width <= available_width)
     return text;
 
   const base::string16 ellipsis = base::string16(kEllipsisUTF16);
   const bool insert_ellipsis = (behavior != TRUNCATE);
   const bool elide_in_middle = (behavior == ELIDE_MIDDLE);
   const bool elide_at_beginning = (behavior == ELIDE_HEAD);
-  StringSlicer slicer(text, ellipsis, elide_in_middle, elide_at_beginning);
 
-  render_text->SetText(ellipsis);
-  const float ellipsis_width = render_text->GetContentWidthF();
+  if (insert_ellipsis) {
+    render_text->SetText(ellipsis);
+    const float ellipsis_width = render_text->GetContentWidthF();
+    if (ellipsis_width > available_width)
+      return base::string16();
+  }
 
-  if (insert_ellipsis && (ellipsis_width > available_width))
-    return base::string16();
+  StringSlicer slicer(text, ellipsis, elide_in_middle, elide_at_beginning);
 
   // Use binary search to compute the elided text.
   size_t lo = 0;
@@ -1384,7 +1409,7 @@ base::string16 RenderText::ElideEmail(const base::string16& email,
         std::min<float>(available_domain_width,
             std::max<float>(available_width - full_username_width,
                             available_width / 2));
-    domain = Elide(domain, desired_domain_width, ELIDE_MIDDLE);
+    domain = Elide(domain, 0, desired_domain_width, ELIDE_MIDDLE);
     // Failing to elide the domain such that at least one character remains
     // (other than the ellipsis itself) remains: return a single ellipsis.
     if (domain.length() <= 1U)
@@ -1395,7 +1420,7 @@ base::string16 RenderText::ElideEmail(const base::string16& email,
   // is guaranteed to fit with at least one character remaining given all the
   // precautions taken earlier).
   available_width -= GetStringWidthF(domain, font_list());
-  username = Elide(username, available_width, ELIDE_TAIL);
+  username = Elide(username, 0, available_width, ELIDE_TAIL);
   return username + kAtSignUTF16 + domain;
 }
 
@@ -1426,9 +1451,8 @@ void RenderText::UpdateCachedBoundsAndOffset() {
 }
 
 void RenderText::DrawSelection(Canvas* canvas) {
-  const std::vector<Rect> sel = GetSubstringBounds(selection());
-  for (std::vector<Rect>::const_iterator i = sel.begin(); i < sel.end(); ++i)
-    canvas->FillRect(*i, selection_background_focused_color_);
+  for (const Rect& s : GetSubstringBounds(selection()))
+    canvas->FillRect(s, selection_background_focused_color_);
 }
 
 }  // namespace gfx