More or less implement 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
new file mode 100644
index 0000000000000000000000000000000000000000..ded0a72d3cb8c1be8dc25c83a3565ee809e040a1
--- /dev/null
+++ b/ui/gfx/render_text_harfbuzz.cc
@@ -0,0 +1,797 @@
+// Copyright (c) 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 <stdlib.h>
+#include <iterator>
+#include <map>
+
+#include "base/i18n/break_iterator.h"
+#include "base/i18n/char_iterator.h"
+#include "third_party/harfbuzz-ng/src/hb.h"
+#include "third_party/icu/source/common/unicode/ubidi.h"
+#include "third_party/skia/include/core/SkColor.h"
+#include "third_party/skia/include/core/SkTypeface.h"
+#include "ui/gfx/canvas.h"
+#include "ui/gfx/render_text.h"

[msw, 2014/04/29 06:24:45]

Remove this include; it's redundant with render_text_harfbuzz.h.

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

Done.

+#include "ui/gfx/scoped_canvas.h"
+#include "ui/gfx/utf16_indexing.h"
+
+namespace gfx {
+
+namespace {
+
+// Font data provider for HarfBuzz using Skia. Copied from Blink.

[msw, 2014/04/29 06:24:45]

Add TODOs (and file a bug) to de-duplicate code like this. Many of the anon
namespace functions have origins in Blink; some may be non-trivial to
consolidate, but some could be reasonably easy.

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

Done.

+struct HarfBuzzFontData {

[msw, 2014/04/29 06:24:45]

nit: remove all "HarfBuzz" prefixes in this anonymous namespace.

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

Done.

+  HarfBuzzFontData(std::map<uint32_t, uint16_t>* glyph_cache)
+      : glyph_cache_(glyph_cache) {}
+
+  SkPaint paint_;
+  std::map<uint32_t, uint16_t>* glyph_cache_;
+};
+
+hb_position_t SkiaScalarToHarfBuzzPosition(SkScalar value) {
+  return SkScalarToFixed(value);
+}
+
+void SkiaGetGlyphWidthAndExtents(SkPaint* paint,
+                                hb_codepoint_t codepoint,

[msw, 2014/04/29 06:24:45]

nit: fix indentations.

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

Done.

+                                hb_position_t* width,
+                                hb_glyph_extents_t* extents) {
+  CHECK(codepoint <= 0xFFFF);

[msw, 2014/04/29 06:24:45]

nit: CHECK_LE

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

Done.

+  paint->setTextEncoding(SkPaint::kGlyphID_TextEncoding);

[msw, 2014/04/29 06:24:45]

Is it okay to set this here? Should the value be reset after?

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

We call setTextEncoding before doing anything that depends on it, so this is
fine.

+
+  SkScalar skWidth;

[msw, 2014/04/29 06:24:45]

nit: use chromium's unix_hacker naming convention for these.

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

Done.

+  SkRect skBounds;
+  uint16_t glyph = codepoint;
+
+  paint->getTextWidths(&glyph, sizeof(glyph), &skWidth, &skBounds);
+  if (width)
+    *width = SkiaScalarToHarfBuzzPosition(skWidth);
+  if (extents) {
+    // Invert y-axis because Skia is y-grows-down but we set up HarfBuzz to be
+    // y-grows-up.
+    extents->x_bearing = SkiaScalarToHarfBuzzPosition(skBounds.fLeft);
+    extents->y_bearing = SkiaScalarToHarfBuzzPosition(-skBounds.fTop);
+    extents->width = SkiaScalarToHarfBuzzPosition(skBounds.width());
+    extents->height = SkiaScalarToHarfBuzzPosition(-skBounds.height());
+  }
+}
+
+hb_bool_t HarfBuzzGetGlyph(hb_font_t* font,
+                           void* font_data,
+                           hb_codepoint_t unicode,
+                           hb_codepoint_t variation_selector,
+                           hb_codepoint_t* glyph,
+                           void* user_data) {
+  HarfBuzzFontData* hb_font_data =
+      reinterpret_cast<HarfBuzzFontData*>(font_data);
+  std::map<uint32_t, uint16_t>& cache = *hb_font_data->glyph_cache_;
+
+  bool exists = cache.count(unicode) != 0;
+  if (!exists) {
+    SkPaint* paint = &hb_font_data->paint_;
+    paint->setTextEncoding(SkPaint::kUTF32_TextEncoding);

[msw, 2014/04/29 06:24:45]

Is it okay to set this here? Should the value be reset after?

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

Addressed above.

+    uint16_t glyph16;
+    paint->textToGlyphs(&unicode, sizeof(hb_codepoint_t), &glyph16);
+    cache[unicode] = glyph16;
+    *glyph = glyph16;

[msw, 2014/04/29 06:24:45]

nit: this looks redundant with the assignment outside this block.

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

Ouch! Fixed!

+  }
+  *glyph = cache[unicode];
+  return !!*glyph;
+}
+
+hb_position_t HarfBuzzGetGlyphHorizontalAdvance(hb_font_t* font,
+                                                void* font_data,
+                                                hb_codepoint_t glyph,
+                                                void* user_data) {
+  HarfBuzzFontData* hb_font_data =
+      reinterpret_cast<HarfBuzzFontData*>(font_data);
+  hb_position_t advance = 0;
+
+  SkiaGetGlyphWidthAndExtents(&hb_font_data->paint_, glyph, &advance, 0);
+  return advance;
+}
+
+hb_bool_t HarfBuzzGetGlyphHorizontalOrigin(hb_font_t* font,
+                                           void* font_data,
+                                           hb_codepoint_t glyph,
+                                           hb_position_t* x,
+                                           hb_position_t* y,
+                                           void* user_data) {
+  // Just return true, following the way that HarfBuzz-FreeType
+  // implementation does.
+  return true;
+}
+
+hb_bool_t HarfBuzzGetGlyphExtents(hb_font_t* font,
+                                  void* font_data,
+                                  hb_codepoint_t glyph,
+                                  hb_glyph_extents_t* extents,
+                                  void* user_data) {
+  HarfBuzzFontData* hb_font_data =
+      reinterpret_cast<HarfBuzzFontData*>(font_data);
+
+  SkiaGetGlyphWidthAndExtents(&hb_font_data->paint_, glyph, 0, extents);
+  return true;
+}
+
+hb_font_funcs_t* HarfBuzzSkiaGetFontFuncs() {
+  static hb_font_funcs_t* harfbuzz_skia_font_funcs = 0;
+
+  // We don't set callback functions which we can't support.
+  // HarfBuzz will use the fallback implementation if they aren't set.

[msw, 2014/04/29 06:24:45]

nit: Add a TODO to support SkTypeface kerning operations like Blink.

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

Done.

+  if (!harfbuzz_skia_font_funcs) {
+    harfbuzz_skia_font_funcs = hb_font_funcs_create();
+    hb_font_funcs_set_glyph_func(
+        harfbuzz_skia_font_funcs, HarfBuzzGetGlyph, 0, 0);
+    hb_font_funcs_set_glyph_h_advance_func(
+        harfbuzz_skia_font_funcs, HarfBuzzGetGlyphHorizontalAdvance, 0, 0);
+    hb_font_funcs_set_glyph_h_origin_func(
+        harfbuzz_skia_font_funcs, HarfBuzzGetGlyphHorizontalOrigin, 0, 0);
+    hb_font_funcs_set_glyph_extents_func(
+        harfbuzz_skia_font_funcs, HarfBuzzGetGlyphExtents, 0, 0);
+    hb_font_funcs_make_immutable(harfbuzz_skia_font_funcs);
+  }
+  return harfbuzz_skia_font_funcs;
+}
+
+hb_blob_t* HarfBuzzSkiaGetTable(hb_face_t* face,
+                                hb_tag_t tag,
+                                void* user_data) {
+  SkTypeface* typeface = reinterpret_cast<SkTypeface*>(user_data);
+
+  const size_t table_size = typeface->getTableSize(tag);
+  if (!table_size)
+    return 0;
+
+  char* buffer = reinterpret_cast<char*>(malloc(table_size));
+  if (!buffer)
+    return 0;
+  size_t actual_size = typeface->getTableData(tag, 0, table_size, buffer);
+  if (table_size != actual_size) {
+    free(buffer);
+    return 0;
+  }
+
+  return hb_blob_create(const_cast<char*>(buffer), table_size,
+                        HB_MEMORY_MODE_WRITABLE, buffer, free);
+}
+
+template<typename Type>
+void DeleteByType(void* data) {
+  Type* typed_data = reinterpret_cast<Type*>(data);
+  delete typed_data;
+}
+
+// TODO(ckocagil): Implement a cache for HarfBuzz faces. SkTypeface provides
+// unique face IDs, which we can use as keys. Also see base::MRUCache.
+hb_face_t* CreateHarfBuzzFace(SkTypeface* skia_face) {
+  hb_face_t* face = hb_face_create_for_tables(HarfBuzzSkiaGetTable, skia_face,
+                                              0);
+  CHECK(face);
+  return face;
+}
+
+hb_font_t* CreateHarfBuzzFont(SkTypeface* skia_face, int text_size) {
+  typedef std::map<uint32_t, uint16_t> GlyphCache;
+  // TODO: This shouldn't grow indefinitely. See the comment above

[msw, 2014/04/29 06:24:45]

nit: TODO(ckocagil):

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

Done.

+  // CreateHarfBuzzFace;
+  static std::map<SkFontID, GlyphCache> glyph_caches;

[msw, 2014/04/29 06:24:45]

Could this map to a struct composed of a GlyphCache and a harfbuzz_face?

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

Done. Good idea! This resolved the harfbuzz face cache TODO above and gave us a
nice speed boost :-)

[msw, 2014/05/02 05:08:21]

Awesome :D

+
+  SkFontID font_id = skia_face->uniqueID();
+  if (glyph_caches.count(font_id) == 0)
+    glyph_caches[font_id] = GlyphCache();
+
+  hb_face_t* harfbuzz_face = CreateHarfBuzzFace(skia_face);
+  hb_font_t* harfbuzz_font = hb_font_create(harfbuzz_face);
+  unsigned int upem = hb_face_get_upem(harfbuzz_face);

[msw, 2014/04/29 06:24:45]

Please add a comment explaining what's going on here. e.g. why is this done like
the HarfBuzz test code, and not like Blink's HarfBuzzFace::createFont()?

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

AFAIK this shapes the glyphs at maximum size and doesn't take our display into
account. I added a TODO to investigate whether we need to scale this by font
size (for hinting and whatnot).

+  hb_face_destroy(harfbuzz_face);
+  hb_font_set_scale(harfbuzz_font, upem, upem);

[msw, 2014/04/29 06:24:45]

Would it make sense to set the scale separately?

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

Separate from what?

[msw, 2014/05/02 05:08:21]

In a separate function from the creation of the font.
(in case we wanted to save fonts independent of size)
Maybe it's not a worthwhile idea, ignore it if so.

[ckocagil, 2014/05/06 03:38:40]

I'm still not exactly sure what the hb_font_set_scale call actually does. I'll
revisit this code when I do.

+  HarfBuzzFontData* hb_font_data = new HarfBuzzFontData(&glyph_caches[font_id]);

[msw, 2014/04/29 06:24:45]

Could this be cached as well?

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

I implement a cache for this, but both FontData and harfbuzz font objects are
pretty lightweight so there wasn't much difference in perf. I revert that
change.

[msw, 2014/05/02 05:08:21]

Fair enough, thanks for giving it a shot.

+  hb_font_data->paint_.setTypeface(skia_face);
+  hb_font_data->paint_.setTextSize(text_size);
+  hb_font_set_funcs(harfbuzz_font, HarfBuzzSkiaGetFontFuncs(), hb_font_data,
+                    DeleteByType<HarfBuzzFontData>);
+  hb_font_make_immutable(harfbuzz_font);
+  return harfbuzz_font;
+}
+
+size_t CharToGlyph(const internal::TextRunHarfBuzz& run, size_t pos) {
+  DCHECK(run.range.start() <= pos && pos < run.range.end());
+
+  if (run.direction == UBIDI_LTR) {
+    for (size_t i = 0; i < run.glyph_count - 1; ++i) {

[msw, 2014/04/29 06:24:45]

optional nit: remove braces here and with the for loop below.

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

Done.

+      if (pos < run.glyph_to_char[i + 1])
+        return i;
+    }
+    return run.glyph_count - 1;
+  }
+
+  for (size_t i = 0; i < run.glyph_count; ++i) {
+    if (pos >= run.glyph_to_char[i])
+      return i;
+  }
+  DCHECK(false);

[msw, 2014/04/29 06:24:45]

nit: use NOTREACHED().

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

Done.

+  return -1;

[msw, 2014/04/29 06:24:45]

nit: should this return 0 to match the equally viable default return value for
the LTR case above? I think the DCHECK above is sufficient warning.

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

Done.

+}
+
+Range CharRangeToGlyphRange(const internal::TextRunHarfBuzz& run,

[msw, 2014/04/29 06:24:45]

nit: Copy/adapt the comment from render_text_win.cc

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

Done.

+                            const Range& range) {
+  DCHECK(run.range.Contains(range));
+  DCHECK(!range.is_reversed());
+  DCHECK(!range.is_empty());
+
+  size_t first = CharToGlyph(run, range.start());

[msw, 2014/04/29 06:24:45]

nit: const for |first| and |last|.

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

Done.

+  size_t last = CharToGlyph(run, range.end() - 1);
+  return Range(std::min(first, last), std::max(first, last) + 1);
+}
+
+// Returns true if characters of |block_code| may trigger font fallback.
+bool IsUnusualBlockCode(const UBlockCode block_code) {
+  return block_code == UBLOCK_GEOMETRIC_SHAPES ||
+         block_code == UBLOCK_MISCELLANEOUS_SYMBOLS;
+}
+
+bool HasMissingGlyphs(const internal::TextRunHarfBuzz& run) {
+  const int kMissingGlyphId = 0;

[msw, 2014/04/29 06:24:45]

nit: static

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

Done.

+

[msw, 2014/04/29 06:24:45]

nit: remove blank line

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

Done.

+  for (size_t i = 0; i < run.glyph_count; ++i)
+    if (run.glyphs[i] == kMissingGlyphId)
+      return true;
+  return false;
+}
+
+}  // namespace
+
+namespace internal {
+
+TextRunHarfBuzz::TextRunHarfBuzz()
+    : width(0),
+      direction(UBIDI_LTR),
+      glyph_count(-1),
+      strike(false),
+      diagonal_strike(false),
+      underline(false) {}

[msw, 2014/04/29 06:24:45]

nit: initialize |level|, |font_size|, and |font_style| too (in the correct
order).

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

Done.

+
+TextRunHarfBuzz::~TextRunHarfBuzz() {}
+
+}  // namespace internal
+
+RenderTextHarfBuzz::RenderTextHarfBuzz()
+    : RenderText(),
+      needs_layout_(true) {}

[msw, 2014/04/29 06:24:45]

nit: does an empty object need layout? (RenderTextWin doesn't).

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

Nope. Fixed.

+
+RenderTextHarfBuzz::~RenderTextHarfBuzz() {}
+
+Size RenderTextHarfBuzz::GetStringSize() {
+  EnsureLayout();
+  int width = 0;
+  for (size_t i = 0; i < runs_.size(); ++i)
+    width += runs_[i]->width;

[msw, 2014/04/29 06:24:45]

I think starting with a single-line implementation is a good idea :)

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

But this is already single-line. Now I changed it to utilized Line data though.

[msw, 2014/05/02 05:08:21]

Right, I was just saying that you chose the right path.
I'll take a look at the newest patch set soon.

+  return Size(width, font_list().GetHeight());
+}
+
+SelectionModel RenderTextHarfBuzz::FindCursorPosition(const Point& point) {
+  EnsureLayout();
+
+  int x = ToTextPoint(point).x();
+  int offset = 0;
+  size_t run_index = GetRunContainingXCoord(x, &offset);
+  if (run_index >= runs_.size())
+    return EdgeSelectionModel((x < 0) ? CURSOR_LEFT : CURSOR_RIGHT);
+  internal::TextRunHarfBuzz& run = *runs_[run_index];

[msw, 2014/04/29 06:24:45]

nit: use a const ref or a pointer.

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

Done (the former)

+
+  for (size_t i = 0; i < run.glyph_count; ++i) {
+    const SkScalar end =
+        i + 1 == run.glyph_count ? run.width : run.positions[i + 1].x();
+    const SkScalar middle = (end + run.positions[i].x()) / 2;
+    if (offset < middle)
+      return SelectionModel(LayoutIndexToTextIndex(
+          run.glyph_to_char[i] + (run.direction == UBIDI_RTL)),

[msw, 2014/04/29 06:24:45]

nit: I think these should be explicitly "* ? 1 : 0".

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

Done.

+          CURSOR_BACKWARD);

[msw, 2014/04/29 06:24:45]

nit: I think these are opposite from how RenderTextWin handles |trailing|.

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

Hopefully fixed. I think I managed to get them right this time, manual testing
works as intended.

+    if (offset < end)
+      return SelectionModel(LayoutIndexToTextIndex(
+          run.glyph_to_char[i] + (run.direction == UBIDI_LTR)),
+          CURSOR_FORWARD);
+  }
+  return EdgeSelectionModel(CURSOR_RIGHT);
+}
+
+bool RenderTextHarfBuzz::IsCursorablePosition(size_t position) {

[msw, 2014/04/29 06:24:45]

nit: rebase once https://codereview.chromium.org/252563003 lands.

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

Will do.

+  if (position == 0 || position == text().length())
+    return true;
+  EnsureLayout();
+
+  // TODO: Can this be simplified by using |TextRunHarfBuzz::glyph_to_char|?

[msw, 2014/04/29 06:24:45]

nit: TODO(ckocagil):, and yeah, that ought to work.

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

Done (added my name, didn't resolve it)

+  // Check that the index is at a valid code point (not mid-surrgate-pair),
+  // that it is not truncated from layout text (its glyph is shown on screen),
+  // and that its glyph has distinct bounds (not mid-multi-character-grapheme).
+  // An example of a multi-character-grapheme that is not a surrogate-pair is:
+  // \x0915\x093f - (ki) - one of many Devanagari biconsonantal conjuncts.
+  return gfx::IsValidCodePointIndex(text(), position) &&

[msw, 2014/04/29 06:24:45]

nit: remove unnecessary gfx:: namespace qualifier.

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

Done.

+         position < LayoutIndexToTextIndex(GetLayoutText().length()) &&
+         GetGlyphBounds(position) != GetGlyphBounds(position - 1);
+}
+
+std::vector<RenderText::FontSpan> RenderTextHarfBuzz::GetFontSpansForTesting() {
+  NOTIMPLEMENTED();
+  return std::vector<RenderText::FontSpan>();
+}
+
+int RenderTextHarfBuzz::GetLayoutTextBaseline() {
+  EnsureLayout();
+  return lines()[0].baseline;
+}
+
+SelectionModel RenderTextHarfBuzz::AdjacentCharSelectionModel(
+    const SelectionModel& selection,
+    VisualCursorDirection direction) {
+  DCHECK(!needs_layout_);
+  internal::TextRunHarfBuzz* run;
+  size_t run_index = GetRunContainingCaret(selection);
+  if (run_index >= runs_.size()) {
+    // The cursor is not in any run: we're at the visual and logical edge.
+    SelectionModel edge = EdgeSelectionModel(direction);
+    if (edge.caret_pos() == selection.caret_pos())
+      return edge;
+    int visual_index = (direction == CURSOR_RIGHT) ? 0 : runs_.size() - 1;
+    run = runs_[visual_to_logical_[visual_index]];
+  } else {
+    // If the cursor is moving within the current run, just move it by one
+    // grapheme in the appropriate direction.
+    run = runs_[run_index];
+    size_t caret = selection.caret_pos();
+    bool forward_motion =
+        (run->direction == UBIDI_RTL) == (direction == CURSOR_LEFT);
+    if (forward_motion) {
+      if (caret < LayoutIndexToTextIndex(run->range.end())) {
+        caret = IndexOfAdjacentGrapheme(caret, CURSOR_FORWARD);
+        return SelectionModel(caret, CURSOR_BACKWARD);
+      }
+    } else {
+      if (caret > LayoutIndexToTextIndex(run->range.start())) {
+        caret = IndexOfAdjacentGrapheme(caret, CURSOR_BACKWARD);
+        return SelectionModel(caret, CURSOR_FORWARD);
+      }
+    }
+    // The cursor is at the edge of a run; move to the visually adjacent run.
+    int visual_index = logical_to_visual_[run_index];
+    visual_index += (direction == CURSOR_LEFT) ? -1 : 1;
+    if (visual_index < 0 || visual_index >= static_cast<int>(runs_.size()))
+      return EdgeSelectionModel(direction);
+    run = runs_[visual_to_logical_[visual_index]];
+  }
+  bool forward_motion =
+      (run->direction == UBIDI_RTL) == (direction == CURSOR_LEFT);
+  return forward_motion ? FirstSelectionModelInsideRun(run) :
+                          LastSelectionModelInsideRun(run);
+}
+
+SelectionModel RenderTextHarfBuzz::AdjacentWordSelectionModel(

[msw, 2014/04/29 06:24:45]

Having a common implementation (that matches the current Windows implementation)
will cause Linux and ChromeOS behavior to change. We'll probably want to make
that happen in a separate CL beforehand (like my old WIP
<https://codereview.chromium.org/21140002>), or implement separate behavior for
Win vs. Posix. Add a TODO with this information for now.

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

Added TODO.

+    const SelectionModel& selection,
+    VisualCursorDirection direction) {
+  if (obscured())
+    return EdgeSelectionModel(direction);
+
+  base::i18n::BreakIterator iter(text(), base::i18n::BreakIterator::BREAK_WORD);
+  bool success = iter.Init();
+  DCHECK(success);
+  if (!success)
+    return selection;
+
+  size_t pos;
+  if (direction == CURSOR_RIGHT) {
+    pos = std::min(selection.caret_pos() + 1, text().length());
+    while (iter.Advance()) {
+      pos = iter.pos();
+      if (iter.IsWord() && pos > selection.caret_pos())
+        break;
+    }
+  } else {  // direction == CURSOR_LEFT
+    // Notes: We always iterate words from the beginning.
+    // This is probably fast enough for our usage, but we may
+    // want to modify WordIterator so that it can start from the
+    // middle of string and advance backwards.
+    pos = std::max<int>(selection.caret_pos() - 1, 0);
+    while (iter.Advance()) {
+      if (iter.IsWord()) {
+        size_t begin = iter.pos() - iter.GetString().length();
+        if (begin == selection.caret_pos()) {
+          // The cursor is at the beginning of a word.
+          // Move to previous word.
+          break;
+        } else if (iter.pos() >= selection.caret_pos()) {
+          // The cursor is in the middle or at the end of a word.
+          // Move to the top of current word.
+          pos = begin;
+          break;
+        } else {
+          pos = iter.pos() - iter.GetString().length();
+        }
+      }
+    }
+  }
+  return SelectionModel(pos, CURSOR_FORWARD);
+}
+
+Range RenderTextHarfBuzz::GetGlyphBounds(size_t index) {
+  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(GetGlyphXBoundary(run_index, layout_index, false),
+               GetGlyphXBoundary(run_index, layout_index, true));
+}
+
+std::vector<Rect> RenderTextHarfBuzz::GetSubstringBounds(const Range& range) {
+  DCHECK(!needs_layout_);
+  DCHECK(Range(0, text().length()).Contains(range));
+  Range layout_range(TextIndexToLayoutIndex(range.start()),
+                     TextIndexToLayoutIndex(range.end()));
+  DCHECK(Range(0, GetLayoutText().length()).Contains(layout_range));
+
+  std::vector<Rect> rects;
+  if (layout_range.is_empty())
+    return rects;
+  std::vector<Range> bounds;
+
+  // 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]];
+    Range intersection = run->range.Intersect(layout_range);
+    if (intersection.IsValid()) {
+      DCHECK(!intersection.is_reversed());
+      Range range_x(GetGlyphXBoundary(i, intersection.start(), false),
+                    GetGlyphXBoundary(i, intersection.end(), false));
+      if (range_x.is_empty())
+        continue;
+      range_x = Range(range_x.GetMin(), range_x.GetMax());
+      // Union this with the last range if they're adjacent.
+      DCHECK(bounds.empty() || bounds.back().GetMax() <= range_x.GetMin());
+      if (!bounds.empty() && bounds.back().GetMax() == range_x.GetMin()) {
+        range_x = Range(bounds.back().GetMin(), range_x.GetMax());
+        bounds.pop_back();
+      }
+      bounds.push_back(range_x);
+    }
+  }
+  for (size_t i = 0; i < bounds.size(); ++i) {
+    std::vector<Rect> current_rects = TextBoundsToViewBounds(bounds[i]);
+    rects.insert(rects.end(), current_rects.begin(), current_rects.end());
+  }
+  return rects;
+}
+
+size_t RenderTextHarfBuzz::TextIndexToLayoutIndex(size_t index) const {
+  DCHECK_LE(index, text().length());
+  ptrdiff_t i = obscured() ? UTF16IndexToOffset(text(), 0, index) : index;
+  CHECK_GE(i, 0);
+  // Clamp layout indices to the length of the text actually used for layout.
+  return std::min<size_t>(GetLayoutText().length(), i);
+}
+
+size_t RenderTextHarfBuzz::LayoutIndexToTextIndex(size_t index) const {
+  if (!obscured())
+    return index;
+
+  DCHECK_LE(index, GetLayoutText().length());
+  const size_t text_index = UTF16OffsetToIndex(text(), 0, index);
+  DCHECK_LE(text_index, text().length());
+  return text_index;
+}
+
+void RenderTextHarfBuzz::ResetLayout() {
+  needs_layout_ = true;
+}
+
+void RenderTextHarfBuzz::EnsureLayout() {

[msw, 2014/04/29 06:24:45]

I recommend splitting some of this out like RenderTextWin::ItemizeLogicalText.

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

I did some splitting here.

+  if (needs_layout_) {
+    // TODO: Skip complex processing if text isn't complex.

[msw, 2014/04/29 06:24:45]

nit: you can remove this TODO here (it's only applicable to RenderTextWin).

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

Done.

+    // TODO: Handle styles.

[msw, 2014/04/29 06:24:45]

nit: TODO(ckocagil):

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

Resolved! :-)

+    const base::string16& text = GetLayoutText();
+    UErrorCode result = U_ZERO_ERROR;
+
+    runs_.clear();
+
+    if (!text.empty()) {
+      UBiDi* line = ubidi_openSized(text.length(), 0, &result);

[msw, 2014/04/29 06:24:45]

Can you use base::i18n::BiDiLineIterator?

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

It lacks |ubidi_getBaseDirection| which we need here. I added a TODO to add that
functionality to BiDiLineIterator and utilize it.

+      CHECK(U_SUCCESS(result));

[msw, 2014/04/29 06:24:45]

Can you DCHECK or bail gracefully if this fails? Ditto below. 

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

Done, we now NOTREACHED() and return in the case of a failure. Although this
results in a stack overflow from recursive gfx::ElideText calls. (I think it
expects non-zero string size, but it gets zero).

[msw, 2014/05/02 05:08:21]

Yikes, please avoid a stack overflow, even on failure, if possible!
Clearing the text, faking layout data, etc. is preferable.

[ckocagil, 2014/05/06 03:38:40]

Done, we now fallback to fake layout data.

+
+      ubidi_setPara(line, text.c_str(), text.length(), UBIDI_DEFAULT_LTR, NULL,

[msw, 2014/04/29 06:24:45]

This default value should probably come from GetTextDirection.

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

Done.

+                    &result);
+      CHECK(U_SUCCESS(result));
+
+      // Temporarily apply composition underlines and selection colors.
+      ApplyCompositionAndSelectionStyles();
+
+      // Build the list of runs from the script items and ranged styles. Use an
+      // empty color BreakList to avoid breaking runs at color boundaries.
+      BreakList<SkColor> empty_colors;
+      empty_colors.SetMax(text.length());
+      internal::StyleIterator style(empty_colors, styles());
+      //const size_t max_run_length = kMaxGlyphs / 2;

[msw, 2014/04/29 06:24:45]

Remove this and the commented code below too.

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

Done.

+      for (size_t run_break = 0; run_break < text.length();) {
+        internal::TextRunHarfBuzz* run = new internal::TextRunHarfBuzz;
+        run->range.set_start(run_break);
+        run->font_style = (style.style(BOLD) ? Font::BOLD : 0) |
+                          (style.style(ITALIC) ? Font::ITALIC : 0);
+

[msw, 2014/04/29 06:24:45]

nit: remove blank line

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

Done.

+        run->strike = style.style(STRIKE);
+        run->diagonal_strike = style.style(DIAGONAL_STRIKE);
+        run->underline = style.style(UNDERLINE);
+
+        int32 script_item_break = 0;
+        ubidi_getLogicalRun(line, run_break, &script_item_break, &run->level);
+
+        // Find the next break and advance the iterators as needed.
+        run_break = std::min(static_cast<size_t>(script_item_break),
+                             TextIndexToLayoutIndex(style.GetRange().end()));
+
+        // Clamp run lengths to avoid exceeding the maximum supported glyph count.
+        /*if ((run_break - run->range.start()) > max_run_length) {
+          run_break = run->range.start() + max_run_length;
+          if (!IsValidCodePointIndex(layout_text, run_break))
+            --run_break;
+        }*/
+
+        // Break runs adjacent to character substrings in certain code blocks.
+        // This avoids using their fallback fonts for more characters than needed,

[msw, 2014/04/29 06:24:45]

nit: limit to 80 chars here and elsewhere below.

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

Done.

+        // in cases like "\x25B6 Media Title", etc. http://crbug.com/278913
+        if (run_break > run->range.start()) {
+          const size_t run_start = run->range.start();
+          const int32 run_length = static_cast<int32>(run_break - run_start);
+          base::i18n::UTF16CharIterator iter(text.c_str() + run_start,
+                                             run_length);
+          const UBlockCode first_block_code = ublock_getCode(iter.get());
+          const bool first_block_unusual = IsUnusualBlockCode(first_block_code);
+          while (iter.Advance() && iter.array_pos() < run_length) {
+            const UBlockCode current_block_code = ublock_getCode(iter.get());
+            if (current_block_code != first_block_code &&
+                (first_block_unusual || IsUnusualBlockCode(current_block_code))) {
+              run_break = run_start + iter.array_pos();
+              break;
+            }
+          }
+        }
+
+        DCHECK(IsValidCodePointIndex(text, run_break));
+

[msw, 2014/04/29 06:24:45]

nit: remove blank line

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

Done.

+        style.UpdatePosition(LayoutIndexToTextIndex(run_break));
+        run->range.set_end(run_break);
+        const UChar* uchar_start = ubidi_getText(line);
+        run->direction = ubidi_getBaseDirection(uchar_start + run->range.start(),
+                                                run->range.length());
+        // TODO: Is there any case where we would prefer defaulting to UBIDI_RTL?

[msw, 2014/04/29 06:24:45]

nit: TODO(ckocagil):, also the neutral direction should probably inherit the
value from the overall string directionality (this is likely what
RenderTextWin::ItemizeLogicalText does via "script_state_.uBidiLevel =
(GetTextDirection() == base::i18n::RIGHT_TO_LEFT) ? 1 : 0;"). Otherwise, perhaps
it could fallback to the UI via base::i18n::IsRTL()...

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

Used GetTextDirection() here.

+        if (run->direction == UBIDI_NEUTRAL)
+          run->direction = UBIDI_LTR;
+        runs_.push_back(run);
+      }
+
+      for (size_t i = 0; i < runs_.size(); ++i)
+        ShapeRun(runs_[i]);
+
+      const size_t num_runs = runs_.size();
+      scoped_ptr<UBiDiLevel[]> levels(new UBiDiLevel[num_runs]);
+      for (size_t i = 0; i < num_runs; ++i)
+        levels[i] = runs_[i]->level;
+      visual_to_logical_.resize(num_runs);
+      ubidi_reorderVisual(levels.get(), num_runs, &visual_to_logical_[0]);
+      logical_to_visual_.resize(num_runs);
+      ubidi_reorderLogical(levels.get(), num_runs, &logical_to_visual_[0]);
+
+      // Undo the temporarily applied composition underlines and selection colors.
+      UndoCompositionAndSelectionStyles();
+
+      ubidi_close(line);
+    }
+
+    needs_layout_ = false;
+    std::vector<internal::Line> empty_lines;
+    set_lines(&empty_lines);
+  }
+
+  if (lines().empty()) {
+    std::vector<internal::Line> lines;
+    lines.push_back(internal::Line());
+
+    int current_x = 0;
+    SkPaint paint;
+
+    for (size_t i = 0; i < runs_.size(); ++i) {
+      const internal::TextRunHarfBuzz& run = *runs_[visual_to_logical_[i]];
+      internal::LineSegment segment;
+      segment.x_range = Range(current_x, current_x + run.width);
+      segment.char_range = run.range;
+      segment.run = i;
+      lines[0].segments.push_back(segment);
+
+      paint.setTypeface(run.skia_face.get());
+      paint.setTextSize(run.font_size);
+      SkPaint::FontMetrics metrics;
+      paint.getFontMetrics(&metrics);
+
+      lines[0].size.set_width(lines[0].size.width() + run.width);
+      lines[0].size.set_height(std::max(lines[0].size.height(),
+          SkScalarRoundToInt(metrics.fDescent - metrics.fAscent)));
+      lines[0].baseline = std::max(lines[0].baseline,
+                                   SkScalarRoundToInt(-metrics.fAscent));
+    }
+
+    set_lines(&lines);
+  }
+}
+
+void RenderTextHarfBuzz::DrawVisualText(Canvas* canvas) {

[msw, 2014/04/29 06:24:45]

nit: DCHECK(!needs_layout_);

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

Done.

+  int current_x = 0;
+
+  internal::SkiaTextRenderer renderer(canvas);

[msw, 2014/04/29 06:24:45]

Should this apply font smoothing and cleartype settings like RenderTextWin?

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

Looks like it is needed for disabling cleartype when it's disabled from Windows
settings. Done.

+  ApplyFadeEffects(&renderer);
+  ApplyTextShadows(&renderer);
+
+  ApplyCompositionAndSelectionStyles();
+
+  const Vector2d line_offset = GetLineOffset(0);
+
+  for (size_t i = 0; i < runs_.size(); ++i) {

[msw, 2014/04/29 06:24:45]

Do you prefer to do this in terms of runs and not segments? I think that's the
conservative approach, but on the other hand, using segments would keep the code
closer to RenderTextWin and closer to supporting a multi-line implementation. (I
think it's fine as is, but hoped you might explain why you chose this route)

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

Simply because I implemented Lines and Segments after this function. Let's keep
it this way until we fix the primary issues and make this the default
RenderText.

+    const internal::TextRunHarfBuzz& run = *runs_[visual_to_logical_[i]];
+

[msw, 2014/04/29 06:24:45]

nit: remove blank line

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

Done.

+    renderer.SetTypeface(run.skia_face.get());
+    renderer.SetTextSize(run.font_size);
+
+    ScopedCanvas scoped_canvas(canvas);
+    Vector2d origin = line_offset + Vector2d(current_x, lines()[0].baseline);
+    canvas->Translate(origin);

[msw, 2014/04/29 06:24:45]

Won't this undesirably apply the vector atop the existing translation?

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

Why would that be undesirable? If the RenderText client gives us a translated
Canvas, then it probably wants us to draw at a translated position.

[msw, 2014/05/02 05:08:21]

It looks like this continuously applies offsets without ever unapplying them.
Given run[0] at x=10, run[1] at x=20, the combo draws run[1] at x=30.

[ckocagil, 2014/05/06 03:38:40]

The ScopedCanvas resets it. It's now an explicit Save/Restore pair.

[msw, 2014/05/09 22:55:18]

Ah! Sorry I missed that; ScopedCanvas is fine if you prefer that.

+
+    for (BreakList<SkColor>::const_iterator it =
+             colors().GetBreak(run.range.start());
+         it != colors().breaks().end() && it->first < run.range.end();
+         ++it) {
+      const Range intersection = colors().GetRange(it).Intersect(run.range);
+      const Range colored_glyphs = CharRangeToGlyphRange(run, intersection);
+      DCHECK(!colored_glyphs.is_empty());

[msw, 2014/04/29 06:24:45]

See how I've adjusted this in https://codereview.chromium.org/252563003

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

I'll merge that CL here after it's landed.

[msw, 2014/05/09 22:55:18]

It has landed, please sync and rebase (in a separate patch set upload, from
addressing comments, if possible).

[ckocagil, 2014/05/12 09:53:29]

Done.

+
+      renderer.SetForegroundColor(it->second);
+      renderer.DrawPosText(&run.positions[colored_glyphs.start()],
+                           &run.glyphs[colored_glyphs.start()],
+                           colored_glyphs.length());
+      int width = (colored_glyphs.end() == run.glyph_count ?
+          run.width : run.positions[colored_glyphs.end()].x()) -

[msw, 2014/04/29 06:24:45]

nit: wrap after ":" and indent the following line four more spaces, I think...

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

Done.

+          run.positions[colored_glyphs.start()].x();
+      renderer.DrawDecorations(0, 0, width, run.underline, run.strike,

[msw, 2014/04/29 06:24:45]

nit: don't you need to supply the starting position coordinates?

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

No, we achieve that by translating the canvas above.

+                               run.diagonal_strike);
+    }
+
+    current_x += run.width;
+  }
+
+  renderer.EndDiagonalStrike();
+
+  UndoCompositionAndSelectionStyles();
+}
+
+void RenderTextHarfBuzz::ShapeRun(internal::TextRunHarfBuzz* run) {
+  const base::string16& text = GetLayoutText();
+

[msw, 2014/04/29 06:24:45]

nit: remove blank line

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

Done.

+  const Font& primary_font = font_list().GetPrimaryFont();

[msw, 2014/04/29 06:24:45]

Add a TODO about handling FontList better and CC yukishiino@, we'll probably
need to add some sort of font fallback mechanism like RenderTextWin has in
ChooseFallbackFont.

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

Added a TODO for fallback. Better how?

[msw, 2014/05/02 05:08:21]

We should use the FontList's other Fonts besides GetPrimaryFont as fallbacks.

+  run->skia_face = internal::CreateSkiaTypeface(primary_font.GetFontName(),
+                                                run->font_style);
+  run->font_size = primary_font.GetFontSize();
+  /*DeriveFontIfNecessary(run->font.GetFontSize(), run->font.GetHeight(),

[msw, 2014/04/29 06:24:45]

nit: remove this if the size is properly adjusted via hb_font_set_scale, etc.

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

Done.

+                        run->font_style, &run->font);*/
+
+  hb_font_t* harfbuzz_font = CreateHarfBuzzFont(run->skia_face.get(),
+                                                run->font_size);
+
+  hb_buffer_t* text_buffer = hb_buffer_create();

[msw, 2014/04/29 06:24:45]

nit: please add some comments for these HarfBuzz function calls.

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

Done.

+  hb_buffer_add_utf16(text_buffer,
+                      reinterpret_cast<const uint16*>(text.c_str()),
+                      text.length(), run->range.start(), run->range.length());
+  hb_buffer_guess_segment_properties(text_buffer);

[msw, 2014/04/29 06:24:45]

Address eae's comment; at least add a TODO and file a bug for now.
eae: any advice? I'm guessing we'll need to port shapeHarfBuzzRuns?

[eae, 2014/04/29 16:01:53]

Ideally we'd be able to share the script/direction resolution code. For now
setting the direction explicitly is probably enough.

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

I added logic to split runs by scripts which uses Unicode script extensions too.

+
+  hb_shape(harfbuzz_font, text_buffer, NULL, 0);
+
+  hb_glyph_info_t* infos = hb_buffer_get_glyph_infos(text_buffer,
+                                                     &run->glyph_count);
+  CHECK(run->glyph_count != -1);

[msw, 2014/04/29 06:24:45]

nit: CHECK_NE, bail more gracefully if possible.

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

Actually there isn't anything to check here. Removed.

+  run->glyphs.reset(new uint16[run->glyph_count]);
+  for (size_t j = 0; j < run->glyph_count; ++j)

[msw, 2014/04/29 06:24:45]

nit: Use |i| or some better name instead of |j| here and below.

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

Done.

+    run->glyphs[j] = infos[j].codepoint;
+
+  run->glyph_to_char.reset(new uint32[run->glyph_count]);
+  for (size_t j = 0; j < run->glyph_count; ++j)

[msw, 2014/04/29 06:24:45]

nit: would it make more sense to loop over the glyphs once?

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

Done.

+    run->glyph_to_char[j] = infos[j].cluster;
+
+  hb_glyph_position_t* hb_positions =
+      hb_buffer_get_glyph_positions(text_buffer, NULL);
+
+  run->positions.reset(new SkPoint[run->glyph_count]);

[msw, 2014/04/29 06:24:45]

nit: should this use gfx::Point or PointF (for [future] subpixel positioning?)

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

Is there any benefit other than using a gfx:: type?

[msw, 2014/05/02 05:08:21]

I see that the SkPoint values are used later on; this is okay for now.

+
+  for (size_t j = 0; j < run->glyph_count; ++j) {
+    const int x_offset =
+        SkScalarRoundToInt(SkFixedToScalar(hb_positions[j].x_offset));
+    const int y_offset =
+        SkScalarRoundToInt(SkFixedToScalar(hb_positions[j].y_offset));
+    run->positions[j].set(run->width + x_offset, y_offset);
+    run->width +=

[msw, 2014/04/29 06:24:45]

nit: does incrementing the run width like this yield the correct values when you
add the run width and the x_offset above in this loop?

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

It visually does, and I don't see why it shouldn't. (Is your question about
ASCII chars or weird chars?)

[msw, 2014/05/02 05:08:21]

I figured that each glyph's x_offset would be relative to the start of the run.
Further, the x_advance is akin to (but nuanced from) the width of each glyph.
If so, run->positions[j] looks like it's double-counting the prior offsets.

[ckocagil, 2014/05/06 03:38:40]

Nope, those offsets are for individual glyphs. e.g a font can make "ü" out of
"u" and two periods by offsetting the periods.

+        SkScalarRoundToInt(SkFixedToScalar(hb_positions[j].x_advance));
+  }
+
+  hb_buffer_destroy(text_buffer);
+  hb_font_destroy(harfbuzz_font);
+}
+
+size_t RenderTextHarfBuzz::GetRunContainingCaret(
+    const SelectionModel& caret) const {
+  DCHECK(!needs_layout_);
+  size_t layout_position = TextIndexToLayoutIndex(caret.caret_pos());
+  LogicalCursorDirection affinity = caret.caret_affinity();
+  for (size_t run = 0; run < runs_.size(); ++run)
+    if (RangeContainsCaret(runs_[run]->range, layout_position, affinity))
+      return run;
+  return runs_.size();
+}
+
+int RenderTextHarfBuzz::GetGlyphXBoundary(size_t run_index,
+                                          size_t text_index,
+                                          bool trailing) {
+  internal::TextRunHarfBuzz& run = *runs_[run_index];
+
+  int x = 0;
+

[msw, 2014/04/29 06:24:45]

nit: remove blank line

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

Done.

+  for (size_t i = 0; i < run_index; ++i)
+    x += runs_[i]->width;

[msw, 2014/04/29 06:24:45]

Add TextRunHarfBuzz.preceding_run_widths or similar to RenderTextWin's TextRun?

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

Done.

+
+  const bool last_glyph = text_index == run.range.end();

[msw, 2014/04/29 06:24:45]

nit: inline this at its one use below

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

Done.

+  Range glyph_range;
+  if (last_glyph) {
+    trailing = true;
+    glyph_range = run.direction == UBIDI_LTR ?
+        Range(run.glyph_count - 1, run.glyph_count) : Range(0, 1);
+  } else {
+    glyph_range = CharRangeToGlyphRange(run, Range(text_index, text_index + 1));
+  }
+  int trailing_step = trailing;

[msw, 2014/04/29 06:24:45]

nit: use explicit "* ? 1 : 0" here.

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

Done.

+  size_t glyph_pos = glyph_range.start() +

[msw, 2014/04/29 06:24:45]

Would it be helpful to keep HarfBuzz's glyph advance value for this?

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

Which glyph advance value and helpful how?

[msw, 2014/05/02 05:08:21]

Could hb_positions[j].x_advance be used to get the trailing x boundry?

[ckocagil, 2014/05/06 03:38:40]

See my comment about x_offset above.

+      (run.direction == UBIDI_LTR ? trailing : !trailing);

[msw, 2014/04/29 06:24:45]

nit: use "* ? trailing_step : (1 - trailing_step)" here. (otherwise
trailing_step is unused)

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

Done.

+  x += glyph_pos < run.glyph_count ?
+      SkScalarRoundToInt(run.positions[glyph_pos].x()) : run.width;
+  return x;
+}
+
+SelectionModel RenderTextHarfBuzz::FirstSelectionModelInsideRun(
+    const internal::TextRunHarfBuzz* run) {
+  size_t position = LayoutIndexToTextIndex(run->range.start());
+  position = IndexOfAdjacentGrapheme(position, CURSOR_FORWARD);
+  return SelectionModel(position, CURSOR_BACKWARD);
+}
+
+SelectionModel RenderTextHarfBuzz::LastSelectionModelInsideRun(
+    const internal::TextRunHarfBuzz* run) {
+  size_t position = LayoutIndexToTextIndex(run->range.end());
+  position = IndexOfAdjacentGrapheme(position, CURSOR_BACKWARD);
+  return SelectionModel(position, CURSOR_FORWARD);
+}
+
+size_t RenderTextHarfBuzz::GetRunContainingXCoord(int x, int* offset) const {
+  DCHECK(!needs_layout_);
+  if (x < 0)
+    return runs_.size();
+  // Find the text run containing the argument point (assumed already offset).
+  int current_x = 0;
+  for (size_t run = 0; run < runs_.size(); ++run) {

[msw, 2014/04/29 06:24:45]

The header says |runs_| is in logical order, shouldn't this map to visual?
I'm confused as to why RenderTextWin doesn't do that either...

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

No idea how this worked, maybe we usually don't have very complicated mappings?
Anyway I fixed it here.

[msw, 2014/05/02 05:08:21]

I'm scared :p (I'll need to debug this on Windows for my own sanity)

+    current_x += runs_[run]->width;
+    if (x < current_x) {
+      *offset = x - (current_x - runs_[run]->width);
+      return run;
+    }
+  }
+  return runs_.size();
+}
+
+}  // namespace gfx