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/break_iterator.h"
 #include "base/i18n/char_iterator.h"
@@ skipping 327 matching lines @@
       diagonal_strike(false),
       underline(false) {}
 
 TextRunHarfBuzz::~TextRunHarfBuzz() {}
 
 size_t TextRunHarfBuzz::CharToGlyph(size_t pos) const {
   DCHECK(range.start() <= pos && pos < range.end());
 
   if (direction == UBIDI_LTR) {
     for (size_t i = 0; i < glyph_count - 1; ++i) {
       if (pos < glyph_to_char[i + 1])

[msw, 2014/06/02 19:37:58]

For logical LTR string "a" that yields two glyphs "xy", it looks like this
incorrectly returns 1 for input |pos|==0 (I'd expect it to return 0 for the
first glyph, 'x', not 1 for the second glyph 'y'). Am I missing something?
Should this be:
if (direction == UBIDI_LTR) {
  for (size_t i = 0; i < glyph_count; ++i) {
    if (pos <= glyph_to_char[i + 1])
      return i;
  return glyph_count - 1;
}

[msw, 2014/06/03 22:32:09]

What about this? LMK if I'm wrong or how we should follow-up.

[ckocagil, 2014/06/05 06:51:59]

This should be fixed now.

         return i;
     }
     return glyph_count - 1;
   }
 
   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 (direction == UBIDI_RTL) {
+    // For RTL runs, we subtract 1 from |char_range| to get the trailing edges.

[msw, 2014/05/28 01:18:30]

nit: the meaning of "to get the trailing edges" isn't entirely clear to me, can
you clarify or elaborate on this comment?

[ckocagil, 2014/06/02 02:57:42]

I tried to find a better way to explain this, but couldn't.

Let's say we're trying to select [1,2) in a 3-char string.

LTR:
chars:  A[B)C
glyphs: a[b)c

RTL:
chars:   A[B)C
glyphs: (c]b a  oops!

it should be "c(b]a" and that's what subtracting 1 does.

[msw, 2014/06/02 19:37:58]

So a char range of [1,2) would yield "[c)ba" without -1's like:
first = CharToGlyph(char_range.start()) => CharToGlyph(1) => 1
last = CharToGlyph(char_range.end()) => CharToGlyph(2) => 0
return Range(std::min(first, last), std::max(first, last));
return Range(std::min(1, 0), std::max(1, 0));
return Range(0, 1) => "[c)ba"

But a char range of [1,2) would yield "c[b)a" with -1's like:
first = CharToGlyph(char_range.start() - 1) => CharToGlyph(0) => 2
last = CharToGlyph(char_range.end() - 1) => CharToGlyph(1) => 1
return Range(std::min(first, last), std::max(first, last));
return Range(std::min(2, 1), std::max(2, 1));
return Range(1, 2) => "c[b)a"

It looks right for odd dual-glyph or surrogate pair cases too:

String A[B)D where 'B' is a two-glyph char for "d[cb)a" yields:
first = CharToGlyph(char_range.start() - 1) => CharToGlyph(0) => 3
last = CharToGlyph(char_range.end() - 1) => CharToGlyph(1) => 1
return Range(std::min(first, last), std::max(first, last));
return Range(std::min(3, 1), std::max(3, 1));
return Range(1, 3) => "d[cb)a"

String A[BC)D where 'BC' yields just one glyph for "d[b)a" yields:
first = CharToGlyph(char_range.start() - 1) => CharToGlyph(0) => 2
last = CharToGlyph(char_range.end() - 1) => CharToGlyph(1) => 1
return Range(std::min(first, last), std::max(first, last));
return Range(std::min(2, 1), std::max(2, 1));
return Range(1, 2) => "d[b)a"

So, I guess that subtracting 1 makes sense, but I'm not sure that it's really
"to get the trailing edges". It almost seems like you're trying to get the
*leading* edges, at least in the case of "A[B)C", where the *leading* edges of
chars 1 ('B') and 2 ('C') are "A|B|C", and their trailing edges are "AB|C|".
Even if you apply the term to the visual string "cba", you want the leading
edges of the corresponding glyphs "c|b|a", where the trailing edges would be the
incorrect "|c|ba". But perhaps you mean the trailing edges of the preceding
characters? Can you change it to "leading", explain why "trailing" makes more
sense, or perhaps say "For RTL runs, we subtract 1 from |char_range| to get the
trailing edges of the preceding characters" or similar?

[ckocagil, 2014/06/03 22:25:17]

What I mean by trailing is: imagine if CharToGlyph returned a range of glyphs.
For LTR we'd use CharToGlyph(i).start(), for RTL we'd use CharToGlyph(i).end().

Okay, I changed it to "leading". lgty?

+    first = char_range.start() == range.start() ?
+        glyph_count : CharToGlyph(char_range.start() - 1);
+    last = CharToGlyph(char_range.end() - 1);
+  } else {
+    first = CharToGlyph(char_range.start());
+    last = char_range.end() == range.end() ?
+        glyph_count : CharToGlyph(char_range.end());

[msw, 2014/05/28 00:52:33]

This doesn't seem right. It seems like if the end character is the last in the
run, this will include all of its glyphs (glyph_count exceeds the index value of
the last glyph associated with the end character), but if it's mid-run,
CharToGlyph will return "the index of the first first glyph that corresponds to
the character", and thus not really include the glyphs of that character. 

[ckocagil, 2014/05/28 01:02:02]

|char_range.end() == range.end()| doesn't mean "last character in the run", it
means "one past the last character". Remember that these are half-open
intervals.

[msw, 2014/05/28 01:18:30]

Ah, that makes more way more sense.

+  }
+  return Range(std::min(first, last), std::max(first, last));
 }
 
 // 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 542 matching lines @@
   // Shape the text.
   hb_shape(harfbuzz_font, buffer, NULL, 0);
 
   // Populate the run fields with the resulting glyph data in the buffer.
   unsigned int glyph_count = 0;
   hb_glyph_info_t* infos = hb_buffer_get_glyph_infos(buffer, &glyph_count);
   hb_glyph_position_t* hb_positions = hb_buffer_get_glyph_positions(buffer,
                                                                     NULL);
   run->glyph_count = glyph_count;
   run->glyphs.reset(new uint16[run->glyph_count]);
   run->glyph_to_char.reset(new uint32[run->glyph_count]);

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

Would it be helpful and prudent to make a similar char_to_cluster_start array?

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

It would be helpful, but this is a trade off between memory and cpu (and code
complexity). I think this CL is currently fine (not too complex), as long as the
tests are sufficient.

   run->positions.reset(new SkPoint[run->glyph_count]);
   for (size_t i = 0; i < run->glyph_count; ++i) {
     run->glyphs[i] = infos[i].codepoint;
     run->glyph_to_char[i] = infos[i].cluster;
     const int x_offset =
         SkScalarRoundToInt(SkFixedToScalar(hb_positions[i].x_offset));
     const int y_offset =
         SkScalarRoundToInt(SkFixedToScalar(hb_positions[i].y_offset));
     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