Reduce the number of text reshaping in RenderText

ui/gfx/render_text_harfbuzz.h

 // Copyright 2014 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.
 
 #ifndef UI_GFX_RENDER_TEXT_HARFBUZZ_H_
 #define UI_GFX_RENDER_TEXT_HARFBUZZ_H_
 
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
 #include "third_party/harfbuzz-ng/src/hb.h"
@@ skipping 61 matching lines @@
   int font_size;
   int font_style;
   bool strike;
   bool diagonal_strike;
   bool underline;
 
  private:
   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 {
  public:
   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;
   std::vector<FontSpan> GetFontSpansForTesting() override;
   Range GetGlyphBounds(size_t index) override;
 
  protected:
-  // Overridden from RenderText.
+  // RenderText:
   int GetLayoutTextBaseline() override;
   SelectionModel AdjacentCharSelectionModel(
       const SelectionModel& selection,
       VisualCursorDirection direction) override;
   SelectionModel AdjacentWordSelectionModel(
       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;
 
  private:
   friend class RenderTextTest;
   FRIEND_TEST_ALL_PREFIXES(RenderTextTest, Multiline_NormalWidth);
   FRIEND_TEST_ALL_PREFIXES(RenderTextTest, HarfBuzz_RunDirection);
   FRIEND_TEST_ALL_PREFIXES(RenderTextTest, HarfBuzz_BreakRunsByUnicodeBlocks);
   FRIEND_TEST_ALL_PREFIXES(RenderTextTest, HarfBuzz_BreakRunsByEmoji);
   FRIEND_TEST_ALL_PREFIXES(RenderTextTest, HarfBuzz_SubglyphGraphemeCases);
   FRIEND_TEST_ALL_PREFIXES(RenderTextTest, HarfBuzz_SubglyphGraphemePartition);
   FRIEND_TEST_ALL_PREFIXES(RenderTextTest, HarfBuzz_NonExistentFont);
   FRIEND_TEST_ALL_PREFIXES(RenderTextTest, HarfBuzz_UniscribeFallback);
 
   // Specify the width of a glyph for test. The width of glyphs is very
   // platform-dependent and environment-dependent. Otherwise multiline test
   // will become really flaky.
   void set_glyph_width_for_test(uint8 test_width) {
     glyph_width_for_test_ = test_width;
   }
 
   // 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
   // or last caret position inside (not at a boundary of) the run.
   // The returned value represents a cursor/caret position without a selection.
   SelectionModel FirstSelectionModelInsideRun(
       const internal::TextRunHarfBuzz* run);
   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();
 
-  bool needs_layout_;
+  // ICU grapheme iterator for the layout text. Can be NULL in case of an error.
+  base::i18n::BreakIterator* GetGraphemeIterator();
 
-  // ICU grapheme iterator for the layout text. Valid when |!needs_layout_|. Can
-  // be NULL in case of an error.
+  // 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 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. Use GetGraphemeIterator()
+  // to access the iterator.
   scoped_ptr<base::i18n::BreakIterator> grapheme_iterator_;
 
   // The total size of the layouted text.
   SizeF total_size_;
 
   // Fixed width of glyphs. This should only be set in test environments.
   uint8 glyph_width_for_test_;
 
   DISALLOW_COPY_AND_ASSIGN(RenderTextHarfBuzz);
 };
 
 }  // namespace gfx
 
 #endif  // UI_GFX_RENDER_TEXT_HARFBUZZ_H_