Fix the Char to Glyph mapping logic in RenderTextHarfBuzz

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 <map>
 
 #include "base/i18n/bidi_line_iterator.h"
 #include "base/i18n/break_iterator.h"
@@ skipping 375 matching lines @@
       strike(false),
       diagonal_strike(false),
       underline(false) {}
 
 TextRunHarfBuzz::~TextRunHarfBuzz() {}
 
 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) {
     if (pos >= glyph_to_char[i])
       return i;
   }
   NOTREACHED();
   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());
 
-  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);
+  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);
+  }
+
+  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.
 bool TextRunHarfBuzz::HasMissingGlyphs() const {
   static const int kMissingGlyphId = 0;
   for (size_t i = 0; i < glyph_count; ++i) {
     if (glyphs[i] == kMissingGlyphId)
       return true;
   }
   return false;
@@ skipping 541 matching lines @@
     run->positions[i].set(run->width + x_offset, y_offset);
     run->width +=
         SkScalarRoundToInt(SkFixedToScalar(hb_positions[i].x_advance));
   }
 
   hb_buffer_destroy(buffer);
   hb_font_destroy(harfbuzz_font);
 }
 
 }  // namespace gfx