Break runs by clusters rather than iteration over code points

In some languages, the current logic (iteration over code
points and then get the glyphs by CharRangeToGlyphRange)
makes wrong effect, because both a character and its
diacritic marks can point to the same glyph range and
it misunderstands the width of the word for wrapping.

This CL changes the iteration logic for clusters so that
it skips the code points for the diacritic marks.

BUG=470073
R=msw@chromium.org, ckocagil@chromium.org
TEST=the new test case covers

Review URL: https://codereview.chromium.org/1036663003

Cr-Commit-Position: refs/heads/master@{#322316}

ui/gfx/render_text_harfbuzz.cc

@@ -299,7 +299,6 @@ class HarfBuzzLineBreaker {
   // |*next_char| will be greater than |start_char| (to avoid constructing empty
   // lines).
   // Returns whether to skip the line before |*next_char|.
-  // TODO(ckocagil): Check clusters to avoid breaking ligatures and diacritics.
   // TODO(ckocagil): We might have to reshape after breaking at ligatures.
   //                 See whether resolving the TODO above resolves this too.
   // TODO(ckocagil): Do not reserve width for whitespace at the end of lines.
@@ -316,7 +315,8 @@ class HarfBuzzLineBreaker {
     SkScalar word_width = 0;
     *width = 0;
 
-    for (size_t i = start_char; i < run.range.end(); ++i) {
+    Range char_range;
+    for (size_t i = start_char; i < run.range.end(); i += char_range.length()) {
       // |word| holds the word boundary at or before |i|, and |next_word| holds
       // the word boundary right after |i|. Advance both |word| and |next_word|
       // when |i| reaches |next_word|.
@@ -325,7 +325,10 @@ class HarfBuzzLineBreaker {
         word_width = 0;
       }
 
-      Range glyph_range = run.CharRangeToGlyphRange(Range(i, i + 1));
+      Range glyph_range;
+      run.GetClusterAt(i, &char_range, &glyph_range);
+      DCHECK_LT(0U, char_range.length());
+
       SkScalar char_width = ((glyph_range.end() >= run.glyph_count)
                                  ? SkFloatToScalar(run.width)
                                  : run.positions[glyph_range.end()].x()) -
@@ -345,7 +348,7 @@ class HarfBuzzLineBreaker {
           *next_char = i;
         } else {
           // Continue from the next character.
-          *next_char = i + 1;
+          *next_char = i + char_range.length();
         }
         return true;
       }

ui/gfx/render_text_unittest.cc

@@ -2610,6 +2610,23 @@ TEST_F(RenderTextTest, HarfBuzz_EmptyRun) {
   EXPECT_EQ(Range(0, 0), glyphs);
 }
 
+// Ensure the line breaker doesn't compute the word's width bigger than the
+// actual size. See http://crbug.com/470073
+TEST_F(RenderTextTest, HarfBuzz_WordWidthWithDiacritics) {
+  RenderTextHarfBuzz render_text;
+  const base::string16 kWord = WideToUTF16(L"\u0906\u092A\u0915\u0947 ");
+  render_text.SetText(kWord);
+  const gfx::SizeF text_size = render_text.GetStringSizeF();
+
+  render_text.SetText(kWord + kWord);
+  render_text.SetMultiline(true);
+  EXPECT_EQ(text_size.width() * 2, render_text.GetStringSizeF().width());
+  EXPECT_EQ(text_size.height(), render_text.GetStringSizeF().height());
+  render_text.SetDisplayRect(gfx::Rect(0, 0, std::ceil(text_size.width()), 0));
+  EXPECT_NEAR(text_size.width(), render_text.GetStringSizeF().width(), 1.0f);
+  EXPECT_EQ(text_size.height() * 2, render_text.GetStringSizeF().height());
+}
+
 // Ensure a string fits in a display rect with a width equal to the string's.
 TEST_F(RenderTextTest, StringFitsOwnWidth) {
   scoped_ptr<RenderText> render_text(RenderText::CreateInstance());