RenderTextHarfBuzz: Add support for multi line text selection.

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 e543bc5987583206686d593146fff9e6028f834b..37baef2af53425bd7b7ef7fd8015c8708e0c79a6 100644
--- a/ui/gfx/render_text_harfbuzz.cc
+++ b/ui/gfx/render_text_harfbuzz.cc
@@ -291,8 +291,15 @@ class HarfBuzzLineBreaker {
       if (!word_segments.empty() &&
           text_[word_segments.back().char_range.start()] == '\n') {
         new_line = true;
-        word_width -= word_segments.back().width();
-        word_segments.pop_back();
+
+        // Since the line should atleast contain some information regarding the

[msw, 2016/12/07 20:20:35]

nit: 'at least'

[karandeepb, 2016/12/12 10:36:18]

Done.

+        // text range it corresponds to, don't pop the newline segment, if it's
+        // the only segment in the line. This ensures that every line has a non-
+        // empty segments vector (except the last in some cases).
+        if (word_segments.size() != 1u || available_width_ != max_width_) {
+          word_width -= word_segments.back().width();
+          word_segments.pop_back();
+        }
       }
 
       // If the word is not the first word in the line and it can't fit into
@@ -815,39 +822,70 @@ SizeF RenderTextHarfBuzz::GetStringSizeF() {
   return total_size_;
 }
 
-SelectionModel RenderTextHarfBuzz::FindCursorPosition(const Point& point) {
+SelectionModel RenderTextHarfBuzz::FindCursorPosition(const Point& view_point) {
   EnsureLayout();
+  DCHECK(!lines().empty());

[msw, 2016/12/07 20:20:35]

Is lines() empty after layout for non-multiline instances? (I forget)

[karandeepb, 2016/12/12 10:36:18]

No it isn't. HarfBuzzLineBreaker constructor always initiates a new line.

+
+  int line_index = GetLineContainingYCoord((view_point - GetLineOffset(0)).y());
+  // Clip line index to a valid value in case kDragToEndIfOutsideVerticalBounds
+  // is false. Else, drag to end.
+  if (line_index < 0) {
+    if (RenderText::kDragToEndIfOutsideVerticalBounds)
+      return EdgeSelectionModel(GetVisualDirectionOfLogicalBeginning());
+    else
+      line_index++;

[msw, 2016/12/07 20:20:35]

nit: line_index = 0;

[karandeepb, 2016/12/12 10:36:19]

Done.

+  }
+  if (line_index >= (int)lines().size()) {

[msw, 2016/12/07 20:20:35]

nit: use static_cast, or checked_cast from base/numerics/safe_conversions.h

[karandeepb, 2016/12/12 10:36:19]

Done.

+    if (RenderText::kDragToEndIfOutsideVerticalBounds)
+      return EdgeSelectionModel(GetVisualDirectionOfLogicalEnd());
+    else
+      line_index--;

[msw, 2016/12/07 20:20:35]

nit: line_index = lines().size() - 1; (with cast like above)

[karandeepb, 2016/12/12 10:36:18]

Done.

+  }
+  const internal::Line& line = lines()[line_index];
+
+  float point_offset_relative_segment = 0;
+  const int segment_index = GetLineSegmentContainingXCoord(
+      line, (view_point - GetLineOffset(line_index)).x(),
+      &point_offset_relative_segment);
+  if (segment_index < 0)
+    return LineSelectionModel(line, CURSOR_LEFT);
+  if (segment_index >= (int)line.segments.size())

[msw, 2016/12/07 20:20:35]

ditto nit: casting

[karandeepb, 2016/12/12 10:36:19]

Done.

+    return LineSelectionModel(line, CURSOR_RIGHT);
+  const internal::LineSegment& segment = line.segments[segment_index];
+
+  const internal::TextRunHarfBuzz& run = *GetRunList()->runs()[segment.run];
+  const Range segment_glyph_range =

[msw, 2016/12/07 20:20:35]

optional nit: just cache the min value instead

[karandeepb, 2016/12/12 10:36:18]

Done.

+      run.CharRangeToGlyphRange(segment.char_range);
+  const float segment_offset_relative_run =
+      segment_glyph_range.GetMin() != 0
+          ? SkScalarToFloat(run.positions[segment_glyph_range.GetMin()].x())
+          : 0;
+  const float point_offset_relative_run =
+      point_offset_relative_segment + segment_offset_relative_run;
 
-  int x = ToTextPoint(point).x();
-  float offset = 0;
-  size_t run_index = GetRunContainingXCoord(x, &offset);
-
-  internal::TextRunList* run_list = GetRunList();
-  if (run_index >= run_list->size())
-    return EdgeSelectionModel((x < 0) ? CURSOR_LEFT : CURSOR_RIGHT);
-  const internal::TextRunHarfBuzz& run = *run_list->runs()[run_index];
   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;
+    const float end = i + 1 == run.glyph_count
+                          ? run.width
+                          : SkScalarToFloat(run.positions[i + 1].x());
+    const float middle = (end + SkScalarToFloat(run.positions[i].x())) / 2;
 
-    if (offset < middle) {
+    if (point_offset_relative_run < middle) {
       return SelectionModel(DisplayIndexToTextIndex(
           run.glyph_to_char[i] + (run.is_rtl ? 1 : 0)),
           (run.is_rtl ? CURSOR_BACKWARD : CURSOR_FORWARD));
     }
-    if (offset < end) {
+    if (point_offset_relative_run < end) {
       return SelectionModel(DisplayIndexToTextIndex(
           run.glyph_to_char[i] + (run.is_rtl ? 0 : 1)),
           (run.is_rtl ? CURSOR_FORWARD : CURSOR_BACKWARD));
     }
   }
-  return EdgeSelectionModel(CURSOR_RIGHT);
+
+  return LineSelectionModel(line, CURSOR_RIGHT);
 }
 
 bool RenderTextHarfBuzz::IsSelectionSupported() const {
-  // TODO(karandeepb): Support multi-line text selection.
-  return !multiline();
+  return true;
 }
 
 std::vector<RenderText::FontSpan> RenderTextHarfBuzz::GetFontSpansForTesting() {
@@ -1019,42 +1057,38 @@ std::vector<Rect> RenderTextHarfBuzz::GetSubstringBounds(const Range& range) {
   std::vector<Rect> rects;
   if (layout_range.is_empty())
     return rects;
-  std::vector<Range> bounds;
 
   internal::TextRunList* run_list = GetRunList();
-
-  // Add a Range for each run/selection intersection.
-  for (size_t i = 0; i < run_list->size(); ++i) {
-    internal::TextRunHarfBuzz* run =
-        run_list->runs()[run_list->visual_to_logical(i)];
-    Range intersection = run->range.Intersect(layout_range);
-    if (!intersection.IsValid())
-      continue;
-    DCHECK(!intersection.is_reversed());
-    const size_t left_index =
-        run->is_rtl ? intersection.end() - 1 : intersection.start();
-    const Range leftmost_character_x =
-        run->GetGraphemeBounds(this, left_index).Round();
-    const size_t right_index =
-        run->is_rtl ? intersection.start() : intersection.end() - 1;
-    const Range rightmost_character_x =
-        run->GetGraphemeBounds(this, right_index).Round();
-    Range range_x(leftmost_character_x.start(), rightmost_character_x.end());
-    DCHECK(!range_x.is_reversed());
-    if (range_x.is_empty())
+  for (size_t line_index = 0; line_index < lines().size(); ++line_index) {
+    const internal::Line& line = lines()[line_index];
+    if (line.segments.empty()) {
+      DCHECK_EQ(lines().size() - 1, line_index);

[msw, 2016/12/07 20:20:35]

ditto nit: check for lines.back() instead, if possible

[karandeepb, 2016/12/12 10:36:18]

Would require overloading ==.

       continue;

[msw, 2016/12/07 20:20:35]

nit: continue isn't necessary, given the segments iteration below

[karandeepb, 2016/12/12 10:36:19]

Done.

+    }
 
-    // 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();
+    float line_x = 0;
+    for (const internal::LineSegment& segment : line.segments) {
+      const internal::TextRunHarfBuzz& run = *run_list->runs()[segment.run];
+      const Range intersection = segment.char_range.Intersect(layout_range);
+      DCHECK(!intersection.is_reversed());
+      if (!intersection.is_empty()) {
+        float width =
+            SkScalarToFloat(run.GetGlyphWidthForCharRange(intersection));
+        float x;

[msw, 2016/12/07 20:20:35]

nit: explicitly init to 0 (or line_x and nix that term in conditional blocks)

[karandeepb, 2016/12/12 10:36:19]

Done.

+        if (run.is_rtl) {
+          x = line_x + SkScalarToFloat(run.GetGlyphWidthForCharRange(gfx::Range(
+                           intersection.end(), segment.char_range.end())));
+        } else {
+          x = line_x + SkScalarToFloat(run.GetGlyphWidthForCharRange(gfx::Range(
+                           segment.char_range.start(), intersection.start())));
+        }
+        int end_x = std::ceil(x + width);
+        int start_x = std::ceil(x);
+        gfx::Rect rect(start_x, 0, end_x - start_x, line.size.height());
+        rects.push_back(rect + GetLineOffset(line_index));
+      }
+      line_x += segment.width();
     }
-    bounds.push_back(range_x);
-  }
-  for (Range& bound : bounds) {
-    std::vector<Rect> current_rects = TextBoundsToViewBounds(bound);
-    rects.insert(rects.end(), current_rects.begin(), current_rects.end());
   }
   return rects;
 }
@@ -1229,23 +1263,41 @@ size_t RenderTextHarfBuzz::GetRunContainingCaret(
   return run_list->size();
 }
 
-size_t RenderTextHarfBuzz::GetRunContainingXCoord(float x,
-                                                  float* offset) const {
-  DCHECK(!update_display_run_list_);
-  const internal::TextRunList* run_list = GetRunList();
-  if (x < 0)
-    return run_list->size();
-  // Find the text run containing the argument point (assumed already offset).
-  float current_x = 0;
-  for (size_t i = 0; i < run_list->size(); ++i) {
-    size_t run = run_list->visual_to_logical(i);
-    current_x += run_list->runs()[run]->width;
-    if (x < current_x) {
-      *offset = x - (current_x - run_list->runs()[run]->width);
-      return run;
+int RenderTextHarfBuzz::GetLineContainingYCoord(float text_y) {
+  if (text_y < 0)
+    return -1;
+
+  for (size_t i = 0; i < lines().size(); i++) {
+    const internal::Line& line = lines()[i];
+
+    if (text_y <= line.size.height())
+      return i;
+    text_y -= line.size.height();
+  }
+
+  return lines().size();
+}
+
+int RenderTextHarfBuzz::GetLineSegmentContainingXCoord(
+    const internal::Line& line,
+    float line_x,
+    float* offset_relative_segment) {
+  DCHECK(offset_relative_segment);
+
+  *offset_relative_segment = 0;
+  if (line_x < 0)
+    return -1;
+  for (size_t i = 0; i < line.segments.size(); i++) {
+    const internal::LineSegment& segment = line.segments[i];
+
+    // segment.x_range is not used because it is text space.

[msw, 2016/12/07 20:20:35]

q: Isn't line_x also in text space?

[karandeepb, 2016/12/12 10:36:18]

Not really. line_x is relative to the beginning of text in current line. Text
space refers to bounds when the whole text is rendered on a single line.

[msw, 2016/12/13 03:04:55]

Acknowledged.

+    if (line_x < segment.width()) {
+      *offset_relative_segment = line_x;
+      return i;
     }
+    line_x -= segment.width();
   }
-  return run_list->size();
+  return line.segments.size();
 }
 
 SelectionModel RenderTextHarfBuzz::FirstSelectionModelInsideRun(