Reduce the number of text reshaping in RenderText

ui/gfx/render_text.h

Index: ui/gfx/render_text.h
diff --git a/ui/gfx/render_text.h b/ui/gfx/render_text.h
index 6c094dcc36d51c6e29574aeedcedb47376a3a38c..cbadda3590d9cb4a65860eb58303779bff969f95 100644
--- a/ui/gfx/render_text.h
+++ b/ui/gfx/render_text.h
@@ -274,8 +274,6 @@ class GFX_EXPORT RenderText {
   void SetElideBehavior(ElideBehavior elide_behavior);
   ElideBehavior elide_behavior() const { return elide_behavior_; }
 
-  const base::string16& layout_text() const { return layout_text_; }
-
   const Rect& display_rect() const { return display_rect_; }
   void SetDisplayRect(const Rect& r);
 
@@ -358,6 +356,11 @@ class GFX_EXPORT RenderText {
   // |GetTextDirection()|, not the direction of a particular run.
   VisualCursorDirection GetVisualDirectionOfLogicalEnd();
 
+  // Returns the text used for layout, which may be obscured,

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

nit: s/layout/display

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

Done.

+  // truncated or elided. The subclass may compute elided text on the fly,
+  // or use precomputed the elided text.
+  virtual const base::string16& GetLayoutText() = 0;

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

I think this should be renamed GetDisplayText().

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

Done.

+
   // Returns the size required to display the current string (which is the
   // wrapped size in multiline mode). The returned size does not include space
   // reserved for the cursor or the offset text shadows.
@@ -448,6 +451,10 @@ class GFX_EXPORT RenderText {
  protected:
   RenderText();
 
+  const base::string16& layout_text() const { return layout_text_; }
+  const base::string16& display_text() const { return display_text_; }
+  bool text_elided() const { return text_elided_; }
+
   const BreakList<SkColor>& colors() const { return colors_; }
   const std::vector<BreakList<bool> >& styles() const { return styles_; }
 
@@ -515,14 +522,20 @@ class GFX_EXPORT RenderText {
   // or bounds changes may invalidate returned values.
   virtual std::vector<Rect> GetSubstringBounds(const Range& range) = 0;
 
-  // Convert between indices into |text_| and indices into |obscured_text_|,
-  // which differ when the text is obscured. Regardless of whether or not the
-  // text is obscured, the character (code point) offsets always match.
-  virtual size_t TextIndexToLayoutIndex(size_t index) const = 0;
-  virtual size_t LayoutIndexToTextIndex(size_t index) const = 0;
+  // Convert between indices into |text_| and indices into
+  // GetLayoutText(), which differ when the text is obscured,

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

nit: GetDisplayText()

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

Done.

+  // truncated or elided. Regardless of whether or not the text is
+  // obscured, the character (code point) offsets always match.
+  virtual size_t TextIndexToLayoutIndex(size_t index) = 0;

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

nit: Rename these TextIndexToDisplayIndex and DisplayIndexToTextIndex.

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

Done.

+  virtual size_t LayoutIndexToTextIndex(size_t index) = 0;
 
-  // Reset the layout to be invalid.
-  virtual void ResetLayout() = 0;
+  // Notifies that layout text, or attributes that affect the layout text
+  // shape have changed. |text_changed| is true if the content of the
+  // |layout_text_| has changed, not just attributes.
+  virtual void OnLayoutTextAttributeChanged(bool text_changed) = 0;
+
+  // Notifies that attributes that affect the display text shape have changed.
+  virtual void OnDisplayTextAttributeChanged() = 0;
 
   // Ensure the text is laid out, lines are computed, and |lines_| is valid.
   virtual void EnsureLayout() = 0;
@@ -530,8 +543,8 @@ class GFX_EXPORT RenderText {
   // Draw the text.
   virtual void DrawVisualText(Canvas* canvas) = 0;
 
-  // Returns the text used for layout, which may be obscured or truncated.
-  const base::string16& GetLayoutText() const;
+  // Update the display text.
+  void UpdateDisplayText(float text_width);
 
   // Returns layout text positions that are suitable for breaking lines.

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

nit: s/layout/display/

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

Done.

   const BreakList<size_t>& GetLineBreaks();
@@ -608,11 +621,14 @@ class GFX_EXPORT RenderText {
   // it is a NO-OP.
   void MoveCursorTo(size_t position, bool select);
 
-  // Updates |layout_text_| if the text is obscured or truncated.
-  void UpdateLayoutText();
+  // Updates |layout_text_| and |display_text_| as needed.

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

nit: maybe this should say "as needed (or marks them dirty)."... Since calling
this for RTHB doesn't actually update |display_text_|, so using that or
display_text() immediately afterwards is incorrect.

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

Done.

+  void OnTextAttributeChanged();
 
   // Elides |text| as needed to fit in the |available_width| using |behavior|.
+  // |text_width| is the pre-calculated width of the text shaped by this render
+  // text, or pass 0 if the width is unknown.
   base::string16 Elide(const base::string16& text,
+                       float text_width,
                        float available_width,
                        ElideBehavior behavior);
 
@@ -691,11 +707,18 @@ class GFX_EXPORT RenderText {
   // The maximum length of text to display, 0 forgoes a hard limit.
   size_t truncate_length_;
 
+  // The obscured and/or truncated text used to layout the text to display.
+  base::string16 layout_text_;
+
+  // The elided text displayed visually. This is empty if the text
+  // does not have to be elided, or became empty as a result of eliding.
+  base::string16 display_text_;

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

It's a little unfortunate that we need to keep an additional copy of the text
when it's elided (especially if we're space-conscious enough to the point of
using bit-field bools), but I see how this makes the layout passes more
piecemeal and allows more efficient handling of display attributes changes.

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

Because just drawing text requires only display info, not layout info,
it's possible to clear layout side upon paint. Added TODO.

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

True, but then any display attribute changes would require not just updates to
the display text, but a full layout pass too, right? (I'm not sure this would be
a good idea to implement)

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

It's rare to apply style change after text has been displayed. Many texts rarely
gets new content (menu, tray), and when you get the new text, you have to update
them all anyway.

We should definitely measure how many of usages can benefit from this though.

[msw, 2015/02/14 00:11:26]

Good point. Agreed that measurements would help.

+
   // The behavior for eliding, fading, or truncating.
   ElideBehavior elide_behavior_;
 
-  // The obscured and/or truncated text that will be displayed.
-  base::string16 layout_text_;
+  // True if the text is elided given the current behavior and display area.
+  bool text_elided_;

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

This is nearly equivalent to display_text_.empty(), but I guess we can't simply
replace uses of this with !display_text_.empty(), for the case where eliding the
text causes it to vanish completely (not even an ellipsis can fit).

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

Yep, I tried it and reverted thanks to broken test.

 
   // Whether newline characters should be replaced with newline symbols.
   bool replace_newline_chars_with_symbols_;

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

You can remove this, SetReplaceNewlineCharsWithSymbols is never called.

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

Done.

@@ -738,8 +761,8 @@ class GFX_EXPORT RenderText {
   // A list of valid layout text line break positions.

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

nit: s/layout/display/

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

Done.

   BreakList<size_t> line_breaks_;
 
-  // Lines computed by EnsureLayout. These should be invalidated with
-  // ResetLayout and on |display_rect_| changes.
+  // Lines computed by EnsureLayout. These should be invalidated upon
+  // OnLayoutTextAttributeChanged and OnDisplayTextAttributeChanged call.

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

nit: "calls"

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

Done.

   std::vector<internal::Line> lines_;
 
   DISALLOW_COPY_AND_ASSIGN(RenderText);