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 10e7a1a5276931a44ad6e8ff6643d5a03bba9ed9..43432f9c60956a5619745eacf9860044fd677a5a 100644
--- a/ui/gfx/render_text_harfbuzz.cc
+++ b/ui/gfx/render_text_harfbuzz.cc
@@ -359,16 +359,25 @@ 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());
-
-  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);
+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 (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.