Fix the Char to Glyph mapping logic in RenderTextHarfBuzz

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 bd99f5edb6ed34b9b5a1d27bb3d09d783a52b3ae..e50ca3cef026181d091255bd34d6b623bce6dd25 100644
--- a/ui/gfx/render_text_harfbuzz.cc
+++ b/ui/gfx/render_text_harfbuzz.cc
@@ -393,11 +393,14 @@ size_t TextRunHarfBuzz::CharToGlyph(size_t pos) const {
   DCHECK(range.start() <= pos && pos < range.end());
 
   if (!is_rtl) {
-    for (size_t i = 0; i < glyph_count - 1; ++i) {
-      if (pos < glyph_to_char[i + 1])
-        return i;
+    size_t cluster_start = 0;

[msw, 2014/06/06 02:00:42]

Q: Should RTL find the cluster start too?

[ckocagil, 2014/06/07 05:40:33]

It already returns the cluster start without any special handling.

+    for (size_t i = 1; i < glyph_count; ++i) {
+      if (pos < glyph_to_char[i])

[msw, 2014/06/06 02:00:42]

nit: make this part of the loop's terminating condition.

[ckocagil, 2014/06/07 05:40:33]

Done.

+        break;
+      if (glyph_to_char[i] != glyph_to_char[i - 1])
+        cluster_start = i;
     }
-    return glyph_count - 1;
+    return cluster_start;
   }
 
   for (size_t i = 0; i < glyph_count; ++i) {
@@ -408,16 +411,32 @@ size_t TextRunHarfBuzz::CharToGlyph(size_t pos) const {
   return 0;
 }
 
-Range TextRunHarfBuzz::CharRangeToGlyphRange(const Range& range) const {
-  DCHECK(range.Contains(range));
-  DCHECK(!range.is_reversed());
-  DCHECK(!range.is_empty());
+Range TextRunHarfBuzz::CharRangeToGlyphRange(const Range& char_range) const {
+  DCHECK(range.Contains(char_range));
+  DCHECK(!char_range.is_reversed());
+  DCHECK(!char_range.is_empty());
+
+  size_t first = 0;
+  size_t last = 0;
+
+  if (is_rtl) {
+    // For RTL runs, we subtract 1 from |char_range| to get the leading edges.
+    last = CharToGlyph(char_range.end() - 1);
+    for (size_t i = char_range.start(); i > range.start(); --i) {

[msw, 2014/06/06 02:00:42]

Why do we need these loops now? Maybe add a comment?

[ckocagil, 2014/06/07 05:40:33]

Every cluster has a non-empty set of glyphs. CharToGlyph gives us the cluster
begin. To figure out the cluster end, we loop until we find a non-empty glyph
range.

Added a comment.

[msw, 2014/06/08 00:10:17]

Hmm, okay I suppose. Can you give me an example where |first =
CharToGlyph(char_range.start() -1)| wouldn't yield something distinct from |last
= CharToGlyph(char_range.end() -1)| and the loop would be needed? Also, is such
a case tested in your new unit test?

[ckocagil, 2014/06/08 01:30:07]

The loops are needed for glyphs that correspond to multiple characters. Let me
give you an LTR example (same idea for RTL):

A B C D -> ab c d (where ab is a single glyph)
We want CharRangeToGlyphRange [0, 1) to be [0, 1). However, both CharToGlyph(0)
and CharToGlyph(1) give us the cluster start, glyph 0, which gives us the empty
glyph range [0, 0). This is because the cluster doesn't end at char 1, it ends
at char 2. To find the actual cluster end, we need to loop.

Yes, both loops are tested by our cases. I actually wrote the tests first and
then added the loops to fix this problem.

[msw, 2014/06/08 01:58:14]

Great explanation, thank you! (it might almost be worth adding as a comment to
the code).

[ckocagil, 2014/06/08 03:15:30]

Expanded the comment a bit without getting too detailed

+      first = CharToGlyph(i - 1);
+      if (first != last)
+        return Range(last, first);
+    }
+    return Range(last, glyph_count);
+  }
 
-  const size_t first = CharToGlyph(range.start());
-  const size_t last = CharToGlyph(range.end() - 1);
-  // TODO(ckocagil): What happens when the character has zero or multiple
-  // glyphs? Is the "+ 1" below correct then?
-  return Range(std::min(first, last), std::max(first, last) + 1);
+  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);
 }
 
 // Returns whether the given shaped run contains any missing glyphs.