Windows implementation of multiline RenderText

ui/gfx/render_text_win.cc

Index: ui/gfx/render_text_win.cc
diff --git a/ui/gfx/render_text_win.cc b/ui/gfx/render_text_win.cc
index ac018de3f446226f8e39276ee65f24ebf1e76478..f0ad9ceac25bdb40352318a6b13488caa8d56a8c 100644
--- a/ui/gfx/render_text_win.cc
+++ b/ui/gfx/render_text_win.cc
@@ -154,6 +154,64 @@ ui::Range CharRangeToGlyphRange(const internal::TextRun& run,
   return result;
 }
 
+// Starting from |start_char|, finds a suitable line break position at or before
+// |available_width| using word break info from |breaks|. If |empty_line| is
+// true, this function will not roll back to |start_char| and |*next_char| will
+// be greater than |start_char|.

[msw, 2013/09/06 23:47:45]

nit: add "(to avoid constructing empty lines)."

[ckocagil, 2013/09/11 14:59:49]

Done.

+// TODO(ckocagil): Do not break ligatures and diacritics.
+//                 TextRun::logical_clusters might help.
+// TODO(ckocagil): We might have to reshape after breaking at ligatures.
+//                 See whether resolving the TODO above resolves this too.
+// TODO(ckocagil): Do not reserve space for whitespace at the end of lines.

[msw, 2013/09/06 23:47:45]

nit: s/space/width/

[ckocagil, 2013/09/11 14:59:49]

Done.

+void BreakRunAtWidth(const internal::TextRun& run,
+                     const BreakList<size_t>& breaks,
+                     size_t start_char,
+                     int available_width,
+                     bool empty_line,
+                     int* width,
+                     size_t* next_char) {
+  DCHECK(run.range.Contains(ui::Range(start_char, start_char + 1)));
+  BreakList<size_t>::const_iterator word = breaks.GetBreak(start_char);
+  BreakList<size_t>::const_iterator next_word = word + 1;
+  // Width from |std::max(word->first, start_char)|.

[msw, 2013/09/06 23:47:45]

nit: ... to the current character.

[ckocagil, 2013/09/11 14:59:49]

Done.

+  int word_width = 0;
+  *width = 0;
+
+  for (size_t i = start_char; i < run.range.end(); ++i) {
+    // |word| holds the word boundary at or before |i|, and |next_word| holds
+    // the word boundary right after |i|. Advance both |word| and |next_word|
+    // when |i| reaches |next_word|.
+    if (next_word != breaks.breaks().end() && i >= next_word->first) {
+      word = next_word++;
+      word_width = 0;
+    }
+
+    ui::Range glyph_range = CharRangeToGlyphRange(run, ui::Range(i, i + 1));
+    int char_width = 0;
+    for (size_t j = glyph_range.start(); j < glyph_range.end(); ++j)
+      char_width += run.advance_widths[j];
+
+    *width += char_width;
+    word_width += char_width;
+
+    if (*width > available_width) {
+      if (!empty_line || word_width < *width) {
+        *width -= word_width;
+        *next_char = std::max(word->first, start_char);
+      } else if (char_width < *width) {
+        *width -= char_width;
+        *next_char = i;
+      } else {
+        *next_char = i + 1;
+      }
+
+      return;
+    }
+  }
+
+  *next_char = run.range.end();
+}
+
 }  // namespace
 
 namespace internal {
@@ -197,6 +255,168 @@ int GetGlyphXBoundary(const internal::TextRun* run,
   return run->preceding_run_widths + x;
 }
 
+// Internal class to generate Line structures. If |multiline| is true, the text
+// is broken into lines at |words| boundaries such that each line is no longer
+// than |max_width|. If |multiline| is false, only outputs a single Line from
+// the given runs.
+// TODO(ckocagil): Expose the interface of this class in the header and test
+//                 this class directly.
+class LineBreaker {
+ public:
+  LineBreaker(int max_width,
+              bool multiline,
+              const BreakList<size_t>* words,
+              const ScopedVector<TextRun>& runs)
+      : max_width_(max_width),
+        multiline_(multiline),
+        words_(words),
+        runs_(runs),
+        text_x_(0),
+        line_x_(0),
+        line_ascent_(0),
+        line_descent_(0) {
+    AdvanceLine();
+  }
+
+  // Breaks the run at given |run_index| into Line structs.
+  void AddRun(int run_index) {
+    const TextRun* run = runs_[run_index];
+    if (multiline_ && line_x_ + run->width > max_width_)
+      BreakRun(run_index);
+    else
+      AddSegment(run_index, run->range, run->width);
+  }
+
+  // Finishes line breaking and outputs the results. Can be called at most once.
+  void Finalize(std::vector<Line>* lines, Size* size) {
+    DCHECK(!lines_.empty());
+    // Add an empty line to finish the line size calculation and remove it.
+    AdvanceLine();
+    lines_.pop_back();
+    *size = total_size_;
+    lines->swap(lines_);
+  }
+
+ private:
+  // A (line index, segment index) pair that specifies a segment in |lines_|.
+  typedef std::pair<size_t, size_t> SegmentHandle;
+
+  LineSegment* SegmentFromHandle(const SegmentHandle& handle) {
+    return &lines_[handle.first].segments[handle.second];
+  }
+
+  // Breaks a run into segments that fit in the last line in |lines_| and adds
+  // them. Adds a new Line to the back of |lines_| whenever a new segment can't
+  // be added without the Line's width exceeding |max_width_|.
+  void BreakRun(int run_index) {
+    DCHECK(words_);
+    TextRun* const run = runs_[run_index];

[msw, 2013/09/06 23:47:45]

nit: can this be const TextRun* const? Sorry if you already answered this.

[ckocagil, 2013/09/11 14:59:49]

It can be; done.

+    int width = 0;
+    size_t next_char = run->range.start();
+
+    // Break the run until it fits the current line.
+    while (next_char < run->range.end()) {
+      const size_t current_char = next_char;
+      BreakRunAtWidth(*run, *words_, next_char, max_width_ - line_x_,

[msw, 2013/09/06 23:47:45]

nit: send |current_char| instead of |next_char|, just for clarity.

[ckocagil, 2013/09/11 14:59:49]

Done.

+                      line_x_ == 0, &width, &next_char);
+      AddSegment(run_index, ui::Range(current_char, next_char), width);
+      if (next_char < run->range.end())
+        AdvanceLine();
+    }
+  }
+
+  // RTL runs are broken in logical order but displayed in visual order. To find
+  // the text-space coordinate (where it would fall in a single-line text)
+  // |x_range| of RTL segments, segment widths are applied in reverse order.
+  // e.g. {[5, 10], [10, 40]} will become {[35, 40], [5, 35]}.
+  void UpdateRTLSegmentRanges() {
+    if (rtl_segments_.empty())
+      return;
+    int x = SegmentFromHandle(rtl_segments_[0])->x_range.start();
+    for (size_t i = rtl_segments_.size(); i > 0; --i) {
+      LineSegment* segment = SegmentFromHandle(rtl_segments_[i - 1]);
+      const size_t segment_width = segment->x_range.length();
+      segment->x_range = ui::Range(x, x + segment_width);
+      x += segment_width;
+    }
+    rtl_segments_.clear();
+  }
+
+  // Finishes the size calculations of the last Line in |lines_|. Adds a new
+  // Line to the back of |lines_|.
+  void AdvanceLine() {
+    if (!lines_.empty()) {
+      Line* line = &lines_.back();
+      line->baseline = line_ascent_;
+      line->size.set_height(line_ascent_ + line_descent_);
+      line->preceding_heights = total_size_.height();
+      total_size_.set_height(total_size_.height() + line->size.height());
+      total_size_.set_width(std::max(total_size_.width(), line->size.width()));
+    }
+    line_x_ = 0;
+    line_ascent_ = 0;
+    line_descent_ = 0;
+    lines_.push_back(Line());
+  }
+
+  // Adds a new segment with the given properties to |lines_.back()|.
+  void AddSegment(int run_index, ui::Range char_range, int width) {
+    if (char_range.is_empty()) {
+      DCHECK_EQ(width, 0);
+      return;
+    }
+    const TextRun* run = runs_[run_index];
+    line_ascent_ = std::max(line_ascent_, run->font.GetBaseline());
+    line_descent_ = std::max(line_descent_,
+                             run->font.GetHeight() - run->font.GetBaseline());
+
+    LineSegment segment;
+    segment.run = run_index;
+    segment.char_range = char_range;
+    segment.x_range = ui::Range(text_x_, text_x_ + width);
+
+    Line* line = &lines_.back();
+    line->segments.push_back(segment);
+    line->size.set_width(line->size.width() + segment.x_range.length());
+    if (run->script_analysis.fRTL) {
+      rtl_segments_.push_back(SegmentHandle(lines_.size() - 1,
+                                            line->segments.size() - 1));
+      // If this is the last segment of an RTL run, reprocess the text-space x
+      // ranges of all segments from the run.
+      if (char_range.end() == run->range.end())
+        UpdateRTLSegmentRanges();
+    }
+    text_x_ += width;
+    line_x_ += width;
+  }
+
+  const int max_width_;
+  const bool multiline_;
+  const BreakList<size_t>* const words_;
+  const ScopedVector<TextRun>& runs_;
+
+  // Stores the resulting lines.
+  std::vector<Line> lines_;
+
+  // |text_x_| and |line_x_| are text-space and line-space x coordinates of the

[msw, 2013/09/06 23:47:45]

nit: remove "|text_x_| and |line_x_| are ".

[ckocagil, 2013/09/11 14:59:49]

Done.

+  // next segment to be added.
+  int text_x_;
+  int line_x_;
+
+  // Size of the multi-line text, not including the currently processed line.
+  Size total_size_;
+
+  // Ascent and descent values of the current line, |lines_.back()|.
+  int line_ascent_;
+  int line_descent_;
+
+  // Segments to be applied by |UpdateRTLSegmentRanges()|. Contains the segments

[msw, 2013/09/06 23:47:45]

nit: consider "// The current RTL run segments, to be applied by
|UpdateRTLSegmentRanges()|."

[ckocagil, 2013/09/11 14:59:49]

Done.

+  // of the current run, if the run is RTL.
+  std::vector<SegmentHandle> rtl_segments_;
+
+  DISALLOW_COPY_AND_ASSIGN(LineBreaker);
+};
+
 }  // namespace internal
 
 // static
@@ -222,7 +442,9 @@ RenderTextWin::~RenderTextWin() {
 
 Size RenderTextWin::GetStringSize() {
   EnsureLayout();
-  return string_size_;
+  // TODO(ckocagil): Always return |multiline_string_size| and remove
+  //                 |string_size_| if possible.

[msw, 2013/09/06 23:47:45]

nit: add some detail about why this should/can't be done.

[ckocagil, 2013/09/11 14:59:49]

I resolved this TODO by updating LineBreaker and the unit tests. I also changed
"Size string_size_" to "int string_width_".

[msw, 2013/09/11 17:32:37]

Awesome, but I have some comments about the next behavior.

+  return multiline() ? multiline_string_size_ : string_size_;
 }
 
 int RenderTextWin::GetBaseline() {
@@ -383,11 +605,12 @@ std::vector<Rect> RenderTextWin::GetSubstringBounds(const ui::Range& range) {
                          TextIndexToLayoutIndex(range.end()));
   DCHECK(ui::Range(0, GetLayoutText().length()).Contains(layout_range));
 
-  std::vector<Rect> bounds;
+  std::vector<Rect> rects;
   if (layout_range.is_empty())
-    return bounds;
+    return rects;
+  std::vector<ui::Range> bounds;
 
-  // Add a Rect for each run/selection intersection.
+  // 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::TextRun* run = runs_[visual_to_logical_[i]];
@@ -396,17 +619,23 @@ std::vector<Rect> RenderTextWin::GetSubstringBounds(const ui::Range& range) {
       DCHECK(!intersection.is_reversed());
       ui::Range range_x(GetGlyphXBoundary(run, intersection.start(), false),
                         GetGlyphXBoundary(run, intersection.end(), false));
-      Rect rect(range_x.GetMin(), 0, range_x.length(), run->font.GetHeight());
-      rect.set_origin(ToViewPoint(rect.origin()));
-      // Union this with the last rect if they're adjacent.
-      if (!bounds.empty() && rect.SharesEdgeWith(bounds.back())) {
-        rect.Union(bounds.back());
+      if (range_x.is_empty())
+        continue;
+      range_x = ui::Range(range_x.GetMin(), range_x.GetMax());
+      // Union this with the last range if they're adjacent.
+      DCHECK(bounds.empty() || bounds.back().GetMin() != range_x.GetMax());

[msw, 2013/09/06 23:47:45]

nit: Use DCHECK(bounds.empty() || bounds.back().GetMax() <= range_x.GetMin());

[ckocagil, 2013/09/11 14:59:49]

Done.

+      if (!bounds.empty() && bounds.back().GetMax() == range_x.GetMin()) {
+        range_x = ui::Range(bounds.back().GetMin(), range_x.GetMax());
         bounds.pop_back();
       }
-      bounds.push_back(rect);
+      bounds.push_back(range_x);
     }
   }
-  return bounds;
+  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 RenderTextWin::TextIndexToLayoutIndex(size_t index) const {
@@ -448,23 +677,35 @@ void RenderTextWin::ResetLayout() {
 }
 
 void RenderTextWin::EnsureLayout() {
-  if (!needs_layout_)
-    return;
-  // TODO(msw): Skip complex processing if ScriptIsComplex returns false.
-  ItemizeLogicalText();
-  if (!runs_.empty())
-    LayoutVisualText();
-  needs_layout_ = false;
+  if (needs_layout_) {
+    // TODO(msw): Skip complex processing if ScriptIsComplex returns false.
+    ItemizeLogicalText();
+    if (!runs_.empty())
+      LayoutVisualText();
+    needs_layout_ = false;
+    std::vector<internal::Line> lines;
+    set_lines(&lines);
+  }
+
+  // Compute lines if they're not valid. This is separate from the layout steps
+  // above to avoid text layout and shaping when we resize |display_rect_|.
+  if (lines().empty()) {
+    DCHECK(!needs_layout_);
+    std::vector<internal::Line> lines;
+    internal::LineBreaker line_breaker(display_rect().width() - 1, multiline(),
+                                       multiline() ? &GetLineBreaks() : NULL,
+                                       runs_);
+    for (size_t i = 0; i < runs_.size(); ++i)
+      line_breaker.AddRun(visual_to_logical_[i]);
+    line_breaker.Finalize(&lines, &multiline_string_size_);
+    DCHECK(!lines.empty());
+    set_lines(&lines);
+  }
 }
 
 void RenderTextWin::DrawVisualText(Canvas* canvas) {
   DCHECK(!needs_layout_);
-
-  // Skia will draw glyphs with respect to the baseline.
-  Vector2d offset(GetTextOffset() + Vector2d(0, common_baseline_));
-
-  SkScalar x = SkIntToScalar(offset.x());
-  SkScalar y = SkIntToScalar(offset.y());
+  DCHECK(!multiline() || !lines().empty());

[msw, 2013/09/06 23:47:45]

Shouldn't lines() be non-empty here, even in the single-line case?

[ckocagil, 2013/09/11 14:59:49]

Done; the "!multiline()" was a leftover from a previous iteration.

 
   std::vector<SkPoint> pos;
 
@@ -479,54 +720,84 @@ void RenderTextWin::DrawVisualText(Canvas* canvas) {
   renderer.SetFontSmoothingSettings(
       smoothing_enabled, cleartype_enabled && !background_is_transparent());
 
-  ApplyCompositionAndSelectionStyles();
-
-  for (size_t i = 0; i < runs_.size(); ++i) {
-    // Get the run specified by the visual-to-logical map.
-    internal::TextRun* run = runs_[visual_to_logical_[i]];
-
-    // Skip painting empty runs and runs outside the display rect area.
-    if ((run->glyph_count == 0) || (x >= display_rect().right()) ||
-        (x + run->width <= display_rect().x())) {
-      x += run->width;
+  ApplyCompositionAndSelectionStyles();
+
+  for (size_t i = 0; i < lines().size(); ++i) {
+    const internal::Line& line = lines()[i];
+    Vector2d line_offset = GetLineOffset(i);
+    Vector2d text_offset = line_offset + Vector2d(0, line.baseline);

[msw, 2013/09/06 23:47:45]

nit: declare |text_offset| and |preceding_segment_widths| at line 736

[ckocagil, 2013/09/11 14:59:49]

Done. (assuming you meant "before the loop")

+    int preceding_segment_widths = 0;
+
+    // Skip painting empty lines or lines outside the display rect area.
+    if (!display_rect().Intersects(Rect(PointAtOffsetFromOrigin(line_offset),
+                                        line.size)))
       continue;
-    }
+
+    for (size_t j = 0; j < line.segments.size(); ++j) {
+      const internal::LineSegment* segment = &line.segments[j];
+      const int segment_width = segment->x_range.length();
+      const internal::TextRun* run = runs_[segment->run];
+      DCHECK(!segment->char_range.is_empty());
+      DCHECK(run->range.Contains(segment->char_range));
+      ui::Range glyph_range = CharRangeToGlyphRange(*run, segment->char_range);
+      if (glyph_range.is_empty()) {

[msw, 2013/09/06 23:47:45]

Can you instead DCHECK(!glyph_range.is_empty())? Sorry if you already answered.

[ckocagil, 2013/09/11 14:59:49]

Done. We don't seem to hit that DCHECK with the strings I tried. Do you think we
should make it a CHECK to see whether it's hit on Canary?

[msw, 2013/09/11 17:32:37]

Leave it as a DCHECK for now; we safely handle it below.

+        DCHECK_EQ(segment_width, 0);
+        continue;
+      }
+      // Skip painting segments outside the display rect area.
+      if (!multiline()) {
+        const Rect segment_bounds(PointAtOffsetFromOrigin(line_offset) +
+                                      Vector2d(preceding_segment_widths, 0),
+                                  Size(segment_width, line.size.height()));
+        if (!display_rect().Intersects(segment_bounds)) {
+          preceding_segment_widths += segment_width;
+          continue;
+        }
+      }
 
-    // Based on WebCore::skiaDrawText. |pos| contains the positions of glyphs.
-    // An extra terminal |pos| entry is added to simplify width calculations.
-    pos.resize(run->glyph_count + 1);
-    SkScalar glyph_x = x;
-    for (int glyph = 0; glyph < run->glyph_count; glyph++) {
-      pos[glyph].set(glyph_x + run->offsets[glyph].du,
-                     y + run->offsets[glyph].dv);
-      glyph_x += SkIntToScalar(run->advance_widths[glyph]);
-    }
-    pos.back().set(glyph_x, y);
-
-    renderer.SetTextSize(run->font.GetFontSize());
-    renderer.SetFontFamilyWithStyle(run->font.GetFontName(), run->font_style);
-
-    for (BreakList<SkColor>::const_iterator it =
-             colors().GetBreak(run->range.start());
-         it != colors().breaks().end() && it->first < run->range.end();
-         ++it) {
-      const ui::Range glyph_range = CharRangeToGlyphRange(*run,
-          colors().GetRange(it).Intersect(run->range));
-      if (glyph_range.is_empty())
-        continue;
-      renderer.SetForegroundColor(it->second);
-      renderer.DrawPosText(&pos[glyph_range.start()],
-                           &run->glyphs[glyph_range.start()],
-                           glyph_range.length());
-      const SkScalar width = pos[glyph_range.end()].x() -
-          pos[glyph_range.start()].x();
-      renderer.DrawDecorations(pos[glyph_range.start()].x(), y,
-                               SkScalarCeilToInt(width), run->underline,
-                               run->strike, run->diagonal_strike);
-    }
+      // |pos| contains the positions of glyphs. An extra terminal |pos| entry
+      // is added to simplify width calculations.
+      int segment_x = preceding_segment_widths;
+      pos.resize(glyph_range.length() + 1);
+      for (size_t k = glyph_range.start(); k < glyph_range.end(); ++k) {
+        pos[k - glyph_range.start()].set(
+            SkIntToScalar(text_offset.x() + segment_x + run->offsets[k].du),

[msw, 2013/09/06 23:47:45]

nit: swap the order of |segment_x| and |run->offsets[k].du|

[ckocagil, 2013/09/11 14:59:49]

Done.

+            SkIntToScalar(text_offset.y() + run->offsets[k].dv));
+        segment_x += run->advance_widths[k];
+      }
+      pos.back().set(SkIntToScalar(text_offset.x() + segment_x),
+                     SkIntToScalar(text_offset.y()));
+
+      renderer.SetTextSize(run->font.GetFontSize());
+      renderer.SetFontFamilyWithStyle(run->font.GetFontName(), run->font_style);
+
+      for (BreakList<SkColor>::const_iterator it =
+               colors().GetBreak(run->range.start());

[msw, 2013/09/06 23:47:45]

Should run->range.start() be changed to segment->char_range.start()?

[ckocagil, 2013/09/11 14:59:49]

Yes. Done.

+           it != colors().breaks().end() && it->first < run->range.end();

[msw, 2013/09/06 23:47:45]

Should run->range.end() be changed to segment->char_range.end()?

[ckocagil, 2013/09/11 14:59:49]

Yes. Done.

+           ++it) {
+        const ui::Range intersection =
+            colors().GetRange(it).Intersect(segment->char_range);
+        const ui::Range colored_glyphs =
+            CharRangeToGlyphRange(*run, intersection);
+        DCHECK(glyph_range.Contains(colored_glyphs));
+        if (colored_glyphs.is_empty())

[msw, 2013/09/06 23:47:45]

With the above changes to use segment->char_range, can this be a DCHECK?

[ckocagil, 2013/09/11 14:59:49]

Done.

+          continue;
+        const SkPoint& start_pos =
+            pos[colored_glyphs.start() - glyph_range.start()];
+        const SkPoint& end_pos =
+            pos[colored_glyphs.end() - glyph_range.start()];
+
+        renderer.SetForegroundColor(it->second);
+        renderer.DrawPosText(&start_pos, &run->glyphs[colored_glyphs.start()],
+                             colored_glyphs.length());
+        renderer.DrawDecorations(start_pos.x(), text_offset.y(),
+                                 SkScalarCeilToInt(end_pos.x() - start_pos.x()),
+                                 run->underline, run->strike,
+                                 run->diagonal_strike);
+      }
 
-    DCHECK_EQ(glyph_x - x, run->width);
-    x = glyph_x;
+      preceding_segment_widths += segment_width;
+    }
   }
 
   UndoCompositionAndSelectionStyles();