Reduce the number of text reshaping in RenderText

ui/gfx/render_text_harfbuzz.h

Index: ui/gfx/render_text_harfbuzz.h
diff --git a/ui/gfx/render_text_harfbuzz.h b/ui/gfx/render_text_harfbuzz.h
index d0e167ecbed1cc421ec7b9eef17fb1aa0d8e50f1..e811b876e3c1b3ccea585e078e32280e0d18797e 100644
--- a/ui/gfx/render_text_harfbuzz.h
+++ b/ui/gfx/render_text_harfbuzz.h
@@ -79,6 +79,54 @@ struct GFX_EXPORT TextRunHarfBuzz {
   DISALLOW_COPY_AND_ASSIGN(TextRunHarfBuzz);
 };
 
+// Manages the list of TextRunHarfBuzz and its logical <-> visual index mapping.
+class TextRunList {
+ public:
+  TextRunList();
+  ~TextRunList();
+
+  size_t size() const { return runs_.size(); }
+
+  // Converts the index between logical and visual index.
+  size_t visual_to_logical(size_t index) const {
+    return visual_to_logical_[index];
+  }
+  size_t logical_to_visual(size_t index) const {
+    return logical_to_visual_[index];
+  }
+
+  const ScopedVector<internal::TextRunHarfBuzz>& runs() const {

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

nit: this can be a one-liner, ditto for the non-const version.

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

Done.

+    return runs_;
+  }
+  ScopedVector<internal::TextRunHarfBuzz>& runs() {
+    return runs_;
+  }
+
+  // Adds the new |run| to the run list.
+  void Add(internal::TextRunHarfBuzz* run);
+
+  // Reset the run list.
+  void Reset();
+
+  // Initialize the index mapping.
+  void InitIndexMap();
+
+  // Precomputes the offsets for all runs.
+  void ComputePrecedingRunWidths();
+
+  // Get the total width of runs. This is not useful when multiline is enabled.
+  int GetWidth() const;
+
+ private:
+  ScopedVector<internal::TextRunHarfBuzz> runs_;

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

nit: keep the comment: "// Text runs in logical order."

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

Done.

+
+  // Maps visual run indices to logical run indices and vice versa.
+  std::vector<int32_t> visual_to_logical_;
+  std::vector<int32_t> logical_to_visual_;
+
+  DISALLOW_COPY_AND_ASSIGN(TextRunList);
+};
+
 }  // namespace internal
 
 class GFX_EXPORT RenderTextHarfBuzz : public RenderText {
@@ -86,8 +134,9 @@ class GFX_EXPORT RenderTextHarfBuzz : public RenderText {
   RenderTextHarfBuzz();
   ~RenderTextHarfBuzz() override;
 
-  // Overridden from RenderText.
+  // RenderText:
   scoped_ptr<RenderText> CreateInstanceOfSameType() const override;
+  const base::string16& GetLayoutText() override;
   Size GetStringSize() override;
   SizeF GetStringSizeF() override;
   SelectionModel FindCursorPosition(const Point& point) override;
@@ -95,7 +144,7 @@ class GFX_EXPORT RenderTextHarfBuzz : public RenderText {
   Range GetGlyphBounds(size_t index) override;
 
  protected:
-  // Overridden from RenderText.
+  // RenderText:
   int GetLayoutTextBaseline() override;
   SelectionModel AdjacentCharSelectionModel(
       const SelectionModel& selection,
@@ -104,10 +153,11 @@ class GFX_EXPORT RenderTextHarfBuzz : public RenderText {
       const SelectionModel& selection,
       VisualCursorDirection direction) override;
   std::vector<Rect> GetSubstringBounds(const Range& range) override;
-  size_t TextIndexToLayoutIndex(size_t index) const override;
-  size_t LayoutIndexToTextIndex(size_t index) const override;
+  size_t TextIndexToLayoutIndex(size_t index) override;
+  size_t LayoutIndexToTextIndex(size_t index) override;
   bool IsValidCursorIndex(size_t index) override;
-  void ResetLayout() override;
+  void OnLayoutTextAttributeChanged(bool text_changed) override;
+  void OnDisplayTextAttributeChanged() override;
   void EnsureLayout() override;
   void DrawVisualText(Canvas* canvas) override;
 
@@ -131,7 +181,7 @@ class GFX_EXPORT RenderTextHarfBuzz : public RenderText {
 
   // Return the run index that contains the argument; or the length of the
   // |runs_| vector if argument exceeds the text length or width.
-  size_t GetRunContainingCaret(const SelectionModel& caret) const;
+  size_t GetRunContainingCaret(const SelectionModel& caret);
   size_t GetRunContainingXCoord(float x, float* offset) const;
 
   // Given a |run|, returns the SelectionModel that contains the logical first
@@ -142,38 +192,67 @@ class GFX_EXPORT RenderTextHarfBuzz : public RenderText {
   SelectionModel LastSelectionModelInsideRun(
       const internal::TextRunHarfBuzz* run);
 
-  // Break the text into logical runs and populate the visual <-> logical maps.
-  void ItemizeText();
+  // Break the text into logical runs and populate the visual <-> logical maps
+  // into |run_list_out|.
+  void ItemizeTextToRuns(const base::string16& string,
+                         internal::TextRunList* run_list_out);
 
-  // Helper method for ShapeRun() that calls ShapeRunWithFont() with |run|,
-  // |family|, and |render_params|, returning true if the family provides all
-  // needed glyphs and false otherwise. Additionally updates |best_family|,
+  // Helper method for ShapeRun() that calls ShapeRunWithFont() with |text|,
+  // |run|, |family|, and |render_params|, returning true if the family provides
+  // all needed glyphs and false otherwise. Additionally updates |best_family|,

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

nit: "all the glyphs needed for |run|, and false otherwise." (since providing
|text| as an arg makes the subject of this function a bit more ambiguous).

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

Done.

   // |best_render_params|, and |best_missing_glyphs| if |family| has fewer than
   // |best_missing_glyphs| missing glyphs.
-  bool CompareFamily(internal::TextRunHarfBuzz* run,
+  bool CompareFamily(const base::string16& text,
                      const std::string& family,
                      const gfx::FontRenderParams& render_params,
+                     internal::TextRunHarfBuzz* run,

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

nit: I think this would be better as the 2nd param (after |text|).

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

Style guide says: 
When defining a function, parameter order is: inputs, then outputs.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Para...

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

Hmm, it's really a mutable input parameter, not an output... but I guess you can
keep it like this if you really consider it an output...

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

It's "input-output" parameter, and that's why I placed in between.

                      std::string* best_family,
                      gfx::FontRenderParams* best_render_params,
                      size_t* best_missing_glyphs);
 
-  // Shape the glyphs needed for the text |run|.
-  void ShapeRun(internal::TextRunHarfBuzz* run);
-  bool ShapeRunWithFont(internal::TextRunHarfBuzz* run,
+  // Shape the glyphs of all runs in |run_list| using |text|.
+  void ShapeRunList(const base::string16& text,
+                    internal::TextRunList* run_list);
+
+  // Shape the glyphs needed for the |text| and |run|.

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

nit: "needed for the |run| within the |text|." or similar.

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

Done.

+  void ShapeRun(const base::string16& text,
+                internal::TextRunHarfBuzz* run);
+  bool ShapeRunWithFont(const base::string16& text,
                         const std::string& font_family,
-                        const FontRenderParams& params);
+                        const FontRenderParams& params,
+                        internal::TextRunHarfBuzz* run);

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

nit: I think this would be better as the 2nd param (after |text|).

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

same here.

 
-  // Text runs in logical order.
-  ScopedVector<internal::TextRunHarfBuzz> runs_;
 
-  // Maps visual run indices to logical run indices and vice versa.
-  std::vector<int32_t> visual_to_logical_;
-  std::vector<int32_t> logical_to_visual_;
+  // Makes sure that text runs for layout text are shaped.

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

This also elides the text as needed, and that should be mentioned in the
function name/comment or split off to another helper. I'd recommend splitting
out a separate EnsureDisplayText() helper and calling that directly from
EnsureLayout and GetLayoutText (which ought to be renamed GetDisplayText). Both
functions should be as rigorous as possible about ensuring that their
prerequisite conditions are satisfied (EnsureDisplayText should
DCHECK(!update_layout_run_list_), etc.)

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

Let me do that in separate CL.

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

Add a TODO

+  void EnsureLayoutRunList();
+
+  // ICU grapheme iterator for the layout text. Can be NULL in case of an error.
+  base::i18n::BreakIterator* GetGraphemeIterator();
+
+  // Convert an index in |text_| to the index in |given_text|. The
+  // |given_text| should be either |eilded_text_| or |layout_text_|

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

nit: |display_text_| or |layout_text_|

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

Done.

+  // depending on the elide state.
+  size_t TextIndexToGivenTextIndex(const base::string16& given_text,
+                                   size_t index);
+
+  // Returns the current run list, |elided_run_list_| if the text is
+  // elided, or |layout_run_list_| otherwise.
+  internal::TextRunList* GetRunList();
+  const internal::TextRunList* GetRunList() const;
+
+  // Text run list sorted in logical order for |layout text_| and

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

nit: |layout_text_|, and you can probably remove "sorted in logical order" if
you add my suggested comment to the TextRunList::runs_ member.

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

Done.

+  // |display_text_|.  |display_run_list_| is created only when the text
+  // is elided.
+  internal::TextRunList layout_run_list_;
+  scoped_ptr<internal::TextRunList> display_run_list_;
 
-  bool needs_layout_;
+  bool update_layout_run_list_ : 1;

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

It might be nicer to just clear/Reset the TextRunList (or reset its scoped_ptr)
to mark these as invalid (and in need of an update), but I realize that
conflates empty layout text (or no display text in the un-elided case) with
invalid run lists...

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

Yep. run can be empty if the text is empty, and the text can be empty when it
may or may not have to be updated. Given that these conditions, I thought having
explicit flag is better in terms of readability.

+  bool update_display_run_list_ : 1;
+  bool update_grapheme_iterator_ : 1;
+  bool update_display_text_ : 1;
 
-  // ICU grapheme iterator for the layout text. Valid when |!needs_layout_|. Can
-  // be NULL in case of an error.
+  // ICU grapheme iterator for the layout text. Use GetGraphemeIterator()
+  // to access the iterator.
   scoped_ptr<base::i18n::BreakIterator> grapheme_iterator_;
 
   // The total size of the layouted text.