RenderTextHarfBuzz: Allow mid-glyph cursors in multi-grapheme clusters

ui/gfx/render_text_harfbuzz.cc

Index: ui/gfx/render_text_harfbuzz.cc
diff --git a/ui/gfx/render_text_harfbuzz.cc b/ui/gfx/render_text_harfbuzz.cc
index 3d9bf58141e562af5276a9ec8fe2e9432670df65..ecf97d1f2c1bf6e7146fca98a2f5aacc28642bdc 100644
--- a/ui/gfx/render_text_harfbuzz.cc
+++ b/ui/gfx/render_text_harfbuzz.cc
@@ -399,6 +399,34 @@ inline hb_script_t ICUScriptToHBScript(UScriptCode script) {
   return hb_script_from_string(uscript_getShortName(script), -1);
 }
 
+template <class Iterator>
+void GetClusterAtImpl(size_t pos,
+                      Range range,
+                      Iterator elements_begin,
+                      Iterator elements_end,
+                      bool reversed,
+                      Range* chars,
+                      Range* glyphs) {
+  Iterator element = std::upper_bound(elements_begin, elements_end, pos);
+  chars->set_end(element == elements_end ? range.end() : *element);
+  glyphs->set_end(reversed ? elements_end - element : element - elements_begin);
+
+  DCHECK(element != elements_begin);

[msw, 2014/06/29 22:29:09]

nit: can you DCHECK_NE?

[ckocagil, 2014/07/24 21:46:56]

It doesn't compile with DCHECK_NE due to some template error.

[msw, 2014/07/25 00:36:59]

Acknowledged.

+  do {
+    --element;

[msw, 2014/06/29 22:29:09]

nit: maybe just make this a one-liner "while(--element != ...);"?

[ckocagil, 2014/07/24 21:46:56]

Done.

+  } while (element != elements_begin && *element == element[-1]);

[msw, 2014/06/29 22:29:09]

nit: Checking for |element[-1]| is a bit odd, can you use *(element - 1)?

[ckocagil, 2014/07/24 21:46:55]

Done.

+  chars->set_start(*element);
+  glyphs->set_start(reversed ?

[msw, 2014/06/29 22:29:10]

nit: move "reversed ?" to the next line.

[ckocagil, 2014/07/24 21:46:55]

Done.

+      elements_end - element : element - elements_begin);
+  if (reversed)
+    *glyphs = Range(glyphs->end(), glyphs->start());
+
+  DCHECK(!chars->is_reversed());
+  DCHECK(!chars->is_empty());
+  DCHECK(!glyphs->is_reversed());
+  DCHECK(!glyphs->is_empty());
+}
+
 }  // namespace
 
 namespace internal {
@@ -418,23 +446,30 @@ TextRunHarfBuzz::TextRunHarfBuzz()
 
 TextRunHarfBuzz::~TextRunHarfBuzz() {}
 
-size_t TextRunHarfBuzz::CharToGlyph(size_t pos) const {
-  DCHECK(range.start() <= pos && pos < range.end());
+void TextRunHarfBuzz::GetClusterAt(size_t pos,

[msw, 2014/06/29 22:29:09]

Hmm, It might make more sense to just offer GetCharsAt and GetGlyphsAt.
(you could avoid temp Ranges, and might be able to inline the impls)

[ckocagil, 2014/07/24 21:46:55]

But in the cases where we need both, we'd have to make two calls and do twice
the computation. Maybe I should make the pointers non-optional to prevent temps.

I changed the temp here a bit, please take another look.

[msw, 2014/07/25 00:37:00]

That's fair; it's a careful balance between perf and simplicity sometimes.

+                                   Range* chars,
+                                   Range* glyphs) const {
+  DCHECK(range.Contains(Range(pos, pos + 1)));
 
-  if (!is_rtl) {
-    size_t cluster_start = 0;
-    for (size_t i = 1; i < glyph_count && pos >= glyph_to_char[i]; ++i)
-      if (glyph_to_char[i] != glyph_to_char[i - 1])
-        cluster_start = i;
-    return cluster_start;
+  scoped_ptr<Range> temp_chars;
+  scoped_ptr<Range> temp_glyphs;
+  if (!chars) {
+    temp_chars.reset(new Range);
+    chars = temp_chars.get();
+  }
+  if (!glyphs) {
+    temp_glyphs.reset(new Range);
+    glyphs = temp_glyphs.get();
   }
 
-  for (size_t i = 0; i < glyph_count; ++i) {
-    if (pos >= glyph_to_char[i])
-      return i;
+  if (is_rtl) {
+    GetClusterAtImpl(pos, range, glyph_to_char.rbegin(), glyph_to_char.rend(),
+        true, chars, glyphs);
+    return;
   }
-  NOTREACHED();
-  return 0;
+
+  GetClusterAtImpl(pos, range, glyph_to_char.begin(), glyph_to_char.end(),
+      false, chars, glyphs);
 }
 
 Range TextRunHarfBuzz::CharRangeToGlyphRange(const Range& char_range) const {
@@ -442,29 +477,13 @@ Range TextRunHarfBuzz::CharRangeToGlyphRange(const Range& char_range) const {
   DCHECK(!char_range.is_reversed());
   DCHECK(!char_range.is_empty());
 
-  size_t first = 0;
-  size_t last = 0;
+  Range start_glyphs;
+  Range end_glyphs;
+  GetClusterAt(char_range.start(), NULL, &start_glyphs);
+  GetClusterAt(char_range.end() - 1, NULL, &end_glyphs);
 
-  if (is_rtl) {
-    // For RTL runs, we subtract 1 from |char_range| to get the leading edges.
-    last = CharToGlyph(char_range.end() - 1);
-    // Loop until we find a non-empty glyph range. For multi-character clusters,
-    // the loop is needed to find the cluster end. Do the same for LTR below.
-    for (size_t i = char_range.start(); i > range.start(); --i) {
-      first = CharToGlyph(i - 1);
-      if (first != last)
-        return Range(last, first);
-    }
-    return Range(last, glyph_count);
-  }
-
-  first = CharToGlyph(char_range.start());
-  for (size_t i = char_range.end(); i < range.end(); ++i) {
-    last = CharToGlyph(i);
-    if (first != last)
-      return Range(first, last);
-  }
-  return Range(first, glyph_count);
+  return is_rtl ? Range(end_glyphs.start(), start_glyphs.end()) :
+      Range(start_glyphs.start(), end_glyphs.end());
 }
 
 // Returns whether the given shaped run contains any missing glyphs.
@@ -477,17 +496,71 @@ bool TextRunHarfBuzz::HasMissingGlyphs() const {
   return false;
 }
 
-int TextRunHarfBuzz::GetGlyphXBoundary(size_t text_index, bool trailing) const {
-  if (text_index == range.end()) {
-    trailing = true;
-    --text_index;
+Range TextRunHarfBuzz::GetGraphemeBounds(const base::string16& text,

[msw, 2014/06/29 22:29:09]

Perhaps GetGraphemeBounds could also be a RenderText method.
(avoids the need to pass in the text, or an iterator)

[ckocagil, 2014/07/24 21:46:56]

I actually want to move more logic to TextRunHarfBuzz and eventually make it a
proper class, and moving this function to RTHB would be against that direction.

[msw, 2014/07/25 00:37:00]

Acknowledged.

+                                         size_t text_index) {
+  DCHECK_LT(text_index, range.end());
+
+  Range chars;
+  Range glyphs;
+  GetClusterAt(text_index, &chars, &glyphs);
+  const int cluster_begin_x = SkScalarRoundToInt(positions[glyphs.start()].x());
+  const int cluster_end_x = glyphs.end() < glyph_count ?
+      SkScalarRoundToInt(positions[glyphs.end()].x()) : width;
+
+  if (chars.length() != 1) {

[msw, 2014/06/29 22:29:09]

This block needs a comment about the bounds treatment of multi-char graphemes.

[ckocagil, 2014/07/24 21:46:56]

Done.

+    base::i18n::BreakIterator* grapheme_iterator = GetGraphemeIterator(text);
+    if (grapheme_iterator) {
+      int before = 0;
+      int total = 0;
+      for (size_t i = chars.start(); i < chars.end(); ++i) {

[msw, 2014/06/29 22:29:09]

Should we instead calculate and store all the grapheme bounds in the run,
instead of doing this extra logic each time the bounds are requested? 

[ckocagil, 2014/07/24 21:46:56]

It seems like ICU is caching bounds internally. Even if I'm wrong and it
doesn't, I would still say this is okay and prefer not doing any optimization
without profiling.

[msw, 2014/07/25 00:36:59]

Acknowledged.

+        if (grapheme_iterator->IsGraphemeBoundary(i - range.start())) {
+          if (i < text_index)
+            ++before;
+          ++total;
+        }
+      }
+
+      if (is_rtl)
+        before = total - before - 1;
+      if (total <= 1) {

[msw, 2014/06/29 22:29:10]

Could total ever really be 0? (DCHECK against that if not). If we are getting
the range of chars for the whole grapheme, and iterating over all of them above,
at least the first char should be a boundary, right?

[ckocagil, 2014/07/24 21:46:56]

You're right, I added a DCHECK.

+        return Range(preceding_run_widths + cluster_begin_x,

[msw, 2014/06/29 22:29:09]

Optionally reverse the conditional to execute the code below, and otherwise fall
through to the return statement at the bottom of the function.

[ckocagil, 2014/07/24 21:46:56]

Done.

+                     preceding_run_widths + cluster_end_x);
+      }
+      DCHECK_GE(before, 0);

[msw, 2014/06/29 22:29:09]

Wouldn't this be false if the index was the first of a multi-char grapheme?

[ckocagil, 2014/07/24 21:46:56]

I don't understand the question. Perhaps you read this as DCHECK_GT?

[msw, 2014/07/25 00:37:00]

Yeah, I think you're right, ignore that question.

+      DCHECK_LE(before, total);
+
+      const int cluster_width = cluster_end_x - cluster_begin_x;
+      int grapheme_begin_x = cluster_begin_x;
+      if (before > 0) {
+        grapheme_begin_x += static_cast<int>(0.5f +
+            cluster_width * before / static_cast<float>(total));
+      }
+      int grapheme_end_x = cluster_end_x;
+      if (before != total - 1) {
+        grapheme_end_x = cluster_begin_x + static_cast<int>(0.5f +
+            cluster_width * (before + 1) / static_cast<float>(total));
+      }
+      return Range(preceding_run_widths + grapheme_begin_x,
+                   preceding_run_widths + grapheme_end_x);
+    }
+  }
+
+  return Range(preceding_run_widths + cluster_begin_x,
+               preceding_run_widths + cluster_end_x);
+}
+
+base::i18n::BreakIterator* TextRunHarfBuzz::GetGraphemeIterator(

[msw, 2014/06/29 22:29:09]

It seems like the RenderText should have an iterator, not each run.

[ckocagil, 2014/07/24 21:46:55]

Done.

+    const base::string16& text) {
+  if (!grapheme_iterator.get()) {
+    grapheme_iterator.reset(new base::i18n::BreakIterator(
+        base::string16(), base::i18n::BreakIterator::BREAK_CHARACTER));
+    if (!grapheme_iterator->Init() || !grapheme_iterator->SetText(
+            text.c_str() + range.start(), range.length())) {
+      NOTREACHED();
+      return NULL;
+    }
   }
-  Range glyph_range = CharRangeToGlyphRange(Range(text_index, text_index + 1));
-  const size_t glyph_pos = (is_rtl == trailing) ?
-      glyph_range.start() : glyph_range.end();
-  const int x = glyph_pos < glyph_count ?
-      SkScalarRoundToInt(positions[glyph_pos].x()) : width;
-  return preceding_run_widths + x;
+  return grapheme_iterator.get();
 }
 
 }  // namespace internal
@@ -633,14 +706,16 @@ SelectionModel RenderTextHarfBuzz::AdjacentWordSelectionModel(
 }
 
 Range RenderTextHarfBuzz::GetGlyphBounds(size_t index) {
+  EnsureLayout();
   const size_t run_index =
       GetRunContainingCaret(SelectionModel(index, CURSOR_FORWARD));
   // Return edge bounds if the index is invalid or beyond the layout text size.
   if (run_index >= runs_.size())
     return Range(GetStringSize().width());
   const size_t layout_index = TextIndexToLayoutIndex(index);
-  return Range(runs_[run_index]->GetGlyphXBoundary(layout_index, false),
-               runs_[run_index]->GetGlyphXBoundary(layout_index, true));
+  internal::TextRunHarfBuzz* run = runs_[run_index];
+  Range bounds = run->GetGraphemeBounds(GetLayoutText(), layout_index);
+  return run->is_rtl ? Range(bounds.end(), bounds.start()) : bounds;
 }
 
 std::vector<Rect> RenderTextHarfBuzz::GetSubstringBounds(const Range& range) {
@@ -658,12 +733,15 @@ std::vector<Rect> RenderTextHarfBuzz::GetSubstringBounds(const Range& range) {
   // Add a Range for each run/selection intersection.
   // TODO(msw): The bounds should probably not always be leading the range ends.
   for (size_t i = 0; i < runs_.size(); ++i) {
-    const internal::TextRunHarfBuzz* run = runs_[visual_to_logical_[i]];
+    internal::TextRunHarfBuzz* run = runs_[visual_to_logical_[i]];
     Range intersection = run->range.Intersect(layout_range);
     if (intersection.IsValid()) {
       DCHECK(!intersection.is_reversed());
-      Range range_x(run->GetGlyphXBoundary(intersection.start(), false),
-                    run->GetGlyphXBoundary(intersection.end(), false));
+      Range range_x(
+          run->GetGraphemeBounds(GetLayoutText(), run->is_rtl ?
+              intersection.end() - 1 : intersection.start()).start(),
+          run->GetGraphemeBounds(GetLayoutText(), run->is_rtl ?
+              intersection.start() : intersection.end() - 1).end());
       if (range_x.is_empty())
         continue;
       range_x = Range(range_x.GetMin(), range_x.GetMax());
@@ -707,11 +785,13 @@ bool RenderTextHarfBuzz::IsValidCursorIndex(size_t index) {
   if (!IsValidLogicalIndex(index))
     return false;
   EnsureLayout();
-  // Disallow indices amid multi-character graphemes by checking glyph bounds.

[msw, 2014/06/29 22:29:09]

Please verify that these examples and Issue 327903 haven't regressed.

[ckocagil, 2014/07/24 21:46:56]

Done. Added a test too.

-  // These characters are not surrogate-pairs, but may yield a single glyph:
-  //   \x0915\x093f - (ki) - one of many Devanagari biconsonantal conjuncts.
-  //   \x0e08\x0e33 - (cho chan + sara am) - a Thai consonant and vowel pair.
-  return GetGlyphBounds(index) != GetGlyphBounds(index - 1);
+  internal::TextRunHarfBuzz* run =
+      runs_[GetRunContainingCaret(SelectionModel(index, CURSOR_BACKWARD))];
+  base::i18n::BreakIterator* grapheme_iterator =

[msw, 2014/06/29 22:29:09]

nit: Name this |iter| for a one-liner and below "return iter && iter->Is...".

[ckocagil, 2014/07/24 21:46:56]

Done. (I made it "return !iter || iter->...") since we return true when iter is
null.

[msw, 2014/07/25 00:36:59]

Acknowledged.

+      run->GetGraphemeIterator(GetLayoutText());
+  if (!grapheme_iterator)
+    return true;
+  return grapheme_iterator->IsGraphemeBoundary(index - run->range.start());
 }
 
 void RenderTextHarfBuzz::ResetLayout() {
@@ -989,7 +1069,7 @@ void RenderTextHarfBuzz::ShapeRun(internal::TextRunHarfBuzz* run) {
                                                                     NULL);
   run->glyph_count = glyph_count;
   run->glyphs.reset(new uint16[run->glyph_count]);
-  run->glyph_to_char.reset(new uint32[run->glyph_count]);
+  run->glyph_to_char.resize(run->glyph_count);
   run->positions.reset(new SkPoint[run->glyph_count]);
   for (size_t i = 0; i < run->glyph_count; ++i) {
     run->glyphs[i] = infos[i].codepoint;