Break runs by clusters rather than iteration over code points

ui/gfx/render_text_harfbuzz.cc

 // 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.
 
 #include "ui/gfx/render_text_harfbuzz.h"
 
 #include <limits>
+#include <set>
 
 #include "base/i18n/bidi_line_iterator.h"
 #include "base/i18n/break_iterator.h"
 #include "base/i18n/char_iterator.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/trace_event/trace_event.h"
 #include "third_party/harfbuzz-ng/src/hb.h"
 #include "third_party/icu/source/common/unicode/ubidi.h"
 #include "third_party/skia/include/core/SkColor.h"
@@ skipping 289 matching lines @@
                        SkScalar* width,
                        size_t* next_char) {
     DCHECK(words_);
     DCHECK(run.range.Contains(Range(start_char, start_char + 1)));
     SkScalar available_width = max_width_ - line_x_;
     BreakList<size_t>::const_iterator word = words_->GetBreak(start_char);
     BreakList<size_t>::const_iterator next_word = word + 1;
     // Width from |std::max(word->first, start_char)| to the current character.
     SkScalar word_width = 0;
     *width = 0;
+    std::set<size_t> used_glyphs;
 
     for (size_t i = start_char; i < run.range.end(); ++i) {
       // |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|.
       if (next_word != words_->breaks().end() && i >= next_word->first) {
         word = next_word++;
         word_width = 0;
       }
 
       Range glyph_range = run.CharRangeToGlyphRange(Range(i, i + 1));
+
+      // Sometimes multiple code points can point to the same glyph (e.g.

[msw, 2015/03/24 22:52:45]

This fix seems odd/wrong. Shouldn't the line above just get the glyph range from
|i| to IndexOfAdjacentGrapheme(i, CURSOR_FORWARD)?

[Jun Mukai, 2015/03/25 00:23:15]

This is inside of EnsureLayout(), which is called from
IndexOfAdjacentGrapheme(), so cannot invoke that method.
Also, as far as I tested, IsGraphemeBoundary() returns true on this specific
example (see the new test case).

Considering about that, however, I think the right way is to use GetClusterAt()
rather than CharRangeToGlyphRange() and run over the clusters. PTAL.

+      // diacritic marks). We need to skip in that case to avoid adding
+      // the width of the same glyph multiple times.
+      bool already_used = false;
+      for (size_t i = glyph_range.start(); i < glyph_range.end(); ++i) {
+        if (used_glyphs.find(i) != used_glyphs.end())
+          already_used = true;
+        used_glyphs.insert(i);
+      }
+      if (already_used)
+        continue;
+
       SkScalar char_width = ((glyph_range.end() >= run.glyph_count)
                                  ? SkFloatToScalar(run.width)
                                  : run.positions[glyph_range.end()].x()) -
                             run.positions[glyph_range.start()].x();
 
       *width += char_width;
       word_width += char_width;
 
       if (*width > available_width) {
         if (line_x_ != 0 || word_width < *width) {
@@ skipping 1177 matching lines @@
   DCHECK(!update_layout_run_list_);
   DCHECK(!update_display_run_list_);
   return text_elided() ? display_run_list_.get() : &layout_run_list_;
 }
 
 const internal::TextRunList* RenderTextHarfBuzz::GetRunList() const {
   return const_cast<RenderTextHarfBuzz*>(this)->GetRunList();
 }
 
 }  // namespace gfx