Windows implementation of multiline RenderText

ui/gfx/render_text_win.cc

 // Copyright (c) 2012 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_win.h"
 
 #include <algorithm>
 
 #include "base/i18n/break_iterator.h"
 #include "base/i18n/rtl.h"
@@ skipping 136 matching lines @@
   } else {
     result = ui::Range(run.logical_clusters[run_range.start()],
         run_range.end() < run.range.length() ?
             run.logical_clusters[run_range.end()] : run.glyph_count);
   }
   DCHECK(!result.is_reversed());
   DCHECK(ui::Range(0, run.glyph_count).Contains(result));
   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 is careful to not roll back to the line beginning.

[msw, 2013/08/15 02:44:17]

nit: careful or guaranteed? That may matter to callers.

[ckocagil, 2013/08/16 16:33:15]

Done.

+// Returns whether the run was broken.
+bool BreakRunAtWidth(const internal::TextRun& run,
+                     const BreakList<size_t>& breaks,
+                     size_t start_char,
+                     int available_width,
+                     bool empty_line,
+                     int* width,
+                     size_t* pos) {
+  DCHECK(run.range.Contains(ui::Range(start_char)));
+  BreakList<size_t>::const_iterator current_word = breaks.GetBreak(start_char);
+  BreakList<size_t>::const_iterator next_word = current_word + 1;
+  // x distance from |current_word|.
+  int current_word_x = 0;

[msw, 2013/08/15 02:44:17]

|word_width| or |current_word_width| seems clearer.

[ckocagil, 2013/08/16 16:33:15]

Done, it's |word_width| now.

+  // x distance from |start_char|.
+  int x = 0;

[msw, 2013/08/15 02:44:17]

just use |*width| instead of declaring |x|.

[ckocagil, 2013/08/16 16:33:15]

Done.

+
+  for (size_t i = start_char; i < run.range.end(); ++i) {

[msw, 2013/08/15 02:44:17]

1) I suspect you'll need to check IsCursorablePosition, use a char iterator,
supply a |char_breaks|, or similar to actually break at valid character indices.
This process should not split surrogate or diacritic code points from their base
code points.

2) Also, I suspect that if we support breaking by character, we'll need to
reshape the glyphs to prevent ligatures meant to join two characters now placed
on separate lines.

3) We'll eventually need to skip reserving width for whitespace characters at
the beginnings and ends of lines.

These could potentially be followup tasks, but they each deserve TODOs in the
code and either corresponding bugs or very clear comments in a meta bug.

[ckocagil, 2013/08/16 16:33:15]

TextRun::logical_clusters has cluster information, perhaps it can be used to
avoid breaking at surrogates/diacritics.
Wouldn't fixing 1) also fix this issue?
I'm adding TODOs here and comments to the bug.

+    if (next_word != breaks.breaks().end() && i >= next_word->first) {
+      current_word = next_word++;
+      current_word_x = 0;
+    }
+
+    ui::Range glyphs = CharRangeToGlyphRange(run, ui::Range(i, i + 1));
+    int char_width = 0;
+    for (size_t j = glyphs.start(); j < glyphs.end(); ++j)
+      char_width += run.advance_widths[j];
+
+    x += char_width;
+    current_word_x += char_width;
+
+    if (x > available_width) {
+      if (!empty_line || current_word_x < x) {
+        *width = x - current_word_x;
+        *pos = std::max(current_word->first, start_char);
+      } else if (char_width < x) {
+        *width = x - char_width;
+        *pos = i;
+      } else {
+        *width = x;
+        *pos = i + 1;
+      }
+
+      return true;
+    }
+  }
+
+  *width = x;
+  *pos = run.range.end();
+  return false;
+}
+
 }  // namespace
 
 namespace internal {
 
 TextRun::TextRun()
   : font_style(0),
     strike(false),
     diagonal_strike(false),
     underline(false),
     width(0),
@@ skipping 23 matching lines @@
       run->glyph_count,
       run->logical_clusters.get(),
       run->visible_attributes.get(),
       run->advance_widths.get(),
       &run->script_analysis,
       &x);
   DCHECK(SUCCEEDED(hr));
   return run->preceding_run_widths + x;
 }
 
+struct LineSegmentWin : LineSegment {
+  const internal::TextRun* run;

[Alexei Svitkine (slow), 2013/08/14 12:59:43]

How about instead of storing a pointer to the run, store a run_index? Since this
is used only in the draw function of RenderTextWin, it should be able to get
back at the actual run pointer from there.

If you store only the run_index, then you can actually do it in LineSegment and
get rid of LineSegmentWin entirely and in fact no longer allocate LineSegments
on the heap - i.e. change ScopedVector<LineSegment> segments; to
std::vector<LineSegment>, which should be more efficient.

(However, you do now have to pass a run_index to LineBreaker::AddRun(), but I
think that's worth doing given the benefits above.)

[ckocagil, 2013/08/14 14:26:30]

I'm not sure about this, can you guess what kind of data LineSegment
implementations on other platforms will need to keep? I haven't checked how
RenderText works on other platforms yet. If you're confident that a run index
will be sufficient, I can make this change. Otherwise, I can add a TODO to
remove the subclass in the future if possible.

[ckocagil, 2013/08/16 16:33:15]

Done.

+};
+
+// Internal class to help break a text into lines.
+class LineBreaker {
+ public:
+  LineBreaker(int max_width, bool multiline, const BreakList<size_t>* words)

[msw, 2013/08/15 02:44:17]

nit: one param per line when the initialization list doesn't also fit.

[ckocagil, 2013/08/16 16:33:15]

Done.

+      : max_width_(max_width),
+        multiline_(multiline),

[msw, 2013/08/15 02:44:17]

Why is LineBreaker even used if multiline is false?
If needed, add a file-local static function that makes a single line:
internal::Line CreateLine(std::vector<internal::TextRun> runs)

[ckocagil, 2013/08/16 16:33:15]

I don't think we should create a new function for this case when implementing
the single-line mode here doesn't even cost a line (see the ternary operator in
AddRun). If needed, we should either change the class name to something like
LineGenerator and/or add a better description of |multiline|.

+        words_(words),
+        preceding_line_heights_(0),
+        pos_(0),
+        text_x_(0),
+        line_x_(0),
+        current_pos_(0),
+        current_width_(0) {
+    SkipLine();
+  }
+
+  // Breaks the given |runs| into |lines|. Should be called at most once for
+  // each instance. If |multiline| is false, doesn't do any breaking and fills

[msw, 2013/08/15 02:44:17]

nit: "each run instance", right? Better to be explicit there.

[ckocagil, 2013/08/16 16:33:15]

Oh, this comment doesn't even apply anymore. Removed.

+  // |lines| with a single Line.
+  void AddRun(const internal::TextRun& run) {

[msw, 2013/08/15 02:44:17]

Make this private, scrap Finalize and only expose a function like:
std::vector<internal::Line> CreateLines(std::vector<internal::TextRun> runs)
(or maybe std::vector<internal::TextRun*> to avoid copying)
Then ComputeLines just calls that with a visually ordered run vector.

[Alexei Svitkine (slow), 2013/08/15 14:51:05]

I disagree. I think it's wasteful to create a temporary vector ordered visually
to use the API. I think the current API is absolutely fine, it's still simple
enough and avoids doing extra work for no benefit.

[msw, 2013/08/15 15:42:09]

I'll concede here, LineBreaker isn't too complex for an internal API as-is.

+    ResetPos(run.range.start());
+    int width = run.width;
+    if (multiline_ && line_x_ + width > max_width_)
+      width = BreakRun(run);
+    // Remaining part of the run fits the line, add it as well.
+    Advance(run.range.end(), width);
+    CommitSegment(run);

[Alexei Svitkine (slow), 2013/08/14 12:59:43]

Advance() and CommitSegment() always get called together. Can you just merge
them?

[msw, 2013/08/15 02:44:17]

Agreed, I like "commit" for adding text, and this should also hopefully
eliminate |current_pos_| and |current_width_| members, which seem more suitable
as function-local variables for modifying |pos_|/|line_x_|/|text_x_| (the true
current committed values).

[ckocagil, 2013/08/16 16:33:15]

All done! Advance() and CommitSegment() are now merged into AddSegment(). There
is no |current_pos_|, |current_width|, ResetPos() and thus the "commit" logic is
gone. I like it, it is much simpler now.

+  }
+
+  // Finishes line breaking and outputs the results. Can be called at most once.
+  void Finalize(std::vector<internal::Line>* lines) {
+    DCHECK(!lines_.empty());
+    lines->swap(lines_);
+  }
+
+ private:
+  // Breaks a run into segments of at most |max_width| width, adds the segments,

[msw, 2013/08/15 02:44:17]

nit: |max_width_|

[ckocagil, 2013/08/16 16:33:15]

Done.

+  // returns the width of the final segment that fits the current line.
+  int BreakRun(const internal::TextRun& run) {
+    int width = 0;
+    size_t pos = 0;
+
+    // Break the run until it fits the current line.
+    while (BreakRunAtWidth(run, *words_, pos_, max_width_ - line_x_,

[msw, 2013/08/15 02:44:17]

This is de-referencing words_, which might technically be NULL, but should
actually be possible, given the logic to call BreakRun. I guess it's okay to not
DCHECK/CHECK before a potential NULL derefernce, but I wouldn't be against that
sort of an explicit check here.

[ckocagil, 2013/08/16 16:33:15]

Added DCHECK.

+                           line_x_ == 0, &width, &pos)) {
+      DCHECK(pos_ < run.range.end());

[msw, 2013/08/15 02:44:17]

nit: DCHECK_LT.

[ckocagil, 2013/08/16 16:33:15]

Done.

+      Advance(pos, width);
+      CommitSegment(run);
+      SkipLine();
+    }
+
+    return width;
+  }
+
+  // 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_pos| of RTL segments, segment widths are applied in reverse order.
+  // e.g. {[5, 10], [10, 40]} will become {[35, 40], [5, 35]}.
+  void PopRtl() {
+    if (rtl_segments_.empty())
+      return;
+    int x = rtl_segments_[0]->x_pos.start();
+    for (size_t i = rtl_segments_.size(); i > 0; --i) {
+      LineSegment* segment = rtl_segments_[i - 1];
+      segment->x_pos = ui::Range(x, x + segment->x_pos.length());
+      x += segment->x_pos.length();
+    }
+    rtl_segments_.clear();
+  }
+
+  void ResetPos(size_t new_pos) {

[msw, 2013/08/15 02:44:17]

This is called once, consider inlining it.

[ckocagil, 2013/08/16 16:33:15]

Done.

+    pos_ = new_pos;
+    current_pos_ = new_pos;
+    current_width_ = 0;
+  }
+
+  void Advance(size_t new_pos, int amount) {
+    current_pos_ = new_pos;
+    current_width_ += amount;
+  }
+
+  void SkipLine() {

[msw, 2013/08/15 02:44:17]

nit: AdvanceLine seems more appropriate.

[ckocagil, 2013/08/16 16:33:15]

Done.

+    if (!lines_.empty())
+      preceding_line_heights_ += lines_.back().height;
+    line_x_ = 0;
+    lines_.push_back(internal::Line());
+  }
+
+  void CommitSegment(const internal::TextRun& run) {
+    if (pos_ == current_pos_) {
+      DCHECK(current_width_ == 0);

[msw, 2013/08/15 02:44:17]

nit: DCHECK_EQ

[ckocagil, 2013/08/16 16:33:15]

Done.

+      return;
+    }
+    internal::LineSegmentWin* segment = new internal::LineSegmentWin;
+    segment->run = &run;
+    segment->char_pos = ui::Range(pos_, current_pos_);
+    segment->x_pos = ui::Range(text_x_, text_x_ + current_width_);
+    lines_.back().segments.push_back(segment);

[msw, 2013/08/15 02:44:17]

nit: cache lines_.back() as a local ptr |line|.

[ckocagil, 2013/08/16 16:33:15]

Done.

+    lines_.back().width += segment->x_pos.length();
+    lines_.back().height = std::max(lines_.back().height,
+                                    segment->run->font.GetHeight());
+    lines_.back().baseline = std::max(lines_.back().baseline,

[msw, 2013/08/15 02:44:17]

I think the line height and baseline might have to be calculated as done in
FontList::CacheCommonFontHeightAndBaseline, by calculating common ascent and
descent values, then determining the resulting height and baseline. That's to
handle cases where the ascents and descents mismatch while heights and/or
baselines might match. Alternately, each line could store a FontList of the
Fonts used to derive these values as needed.

[ckocagil, 2013/08/16 16:33:15]

Done. FontList looks like it can only contain fonts of the same style and size,
so I went with implementing that calculation here.

+                                      segment->run->font.GetBaseline());
+    lines_.back().preceding_heights = preceding_line_heights_;

[msw, 2013/08/15 02:44:17]

Must this be altered on each committed segment? Can't it be done in SkipLine,
allowing you to eliminate preceding_line_heights_ altogether?

[ckocagil, 2013/08/16 16:33:15]

Done.

+    if (run.script_analysis.fRTL) {
+      rtl_segments_.push_back(segment);
+      if (current_pos_ == run.range.end())
+        PopRtl();
+    }
+    pos_ = current_pos_;
+    text_x_ += current_width_;
+    line_x_ += current_width_;
+    current_width_ = 0;
+  }
+
+  int max_width_;

[msw, 2013/08/15 02:44:17]

nit: const

[ckocagil, 2013/08/16 16:33:15]

Done.

+  bool multiline_;

[msw, 2013/08/15 02:44:17]

nit: const

[ckocagil, 2013/08/16 16:33:15]

Done. Also done for |words_| below.

+  const BreakList<size_t>* words_;
+
+  // Stores the resulting lines.
+  std::vector<internal::Line> lines_;
+
+  // Position information of the last committed segment. |pos_| is the segment's
+  // end character position. |text_x_| is the text-space and |line_x_| is the

[msw, 2013/08/15 02:44:17]

nit: |text_x_| and |line_x_| are text-space and line-space x coordinates of
|pos_|.

[ckocagil, 2013/08/16 16:33:15]

Done.

+  // line-space x coordinate of |pos_|.
+  size_t pos_;
+  int text_x_;
+  int line_x_;
+  int preceding_line_heights_;
+
+  // The end character position and width of the current segment to be
+  // committed. The segment will have the character range [pos_, current_pos_)
+  // and width |current_width_|.
+  size_t current_pos_;

[msw, 2013/08/15 02:44:17]

Would |next_pos_| and |next_width_| be more appropriate?
Or perhaps make current_pos_ and pos_ a Range, etc?
(only if you have to keep these, I hope they can be removed)

[ckocagil, 2013/08/16 16:33:15]

They're gone.

+  int current_width_;
+
+  // Segments to be applied by |PopRtl()|.
+  std::vector<LineSegment*> rtl_segments_;
+
+  DISALLOW_COPY_AND_ASSIGN(LineBreaker);
+};
+
 }  // namespace internal
 
 // static
 HDC RenderTextWin::cached_hdc_ = NULL;
 
 // static
 std::map<std::string, Font> RenderTextWin::successful_substitute_fonts_;
 
 RenderTextWin::RenderTextWin()
     : RenderText(),
       common_baseline_(0),
       needs_layout_(false) {
   set_truncate_length(kMaxUniscribeTextLength);
 
   memset(&script_control_, 0, sizeof(script_control_));
   memset(&script_state_, 0, sizeof(script_state_));
 
   MoveCursorTo(EdgeSelectionModel(CURSOR_LEFT));
 }
 
 RenderTextWin::~RenderTextWin() {
 }
 
 Size RenderTextWin::GetStringSize() {
   EnsureLayout();
   return string_size_;
 }
 
+Size RenderTextWin::GetMultilineTextSize() {
+  EnsureLayout();
+  if (!multiline())
+    return Size(display_rect().width(), string_size_.height());
+  return Size(display_rect().width(),

[msw, 2013/08/15 02:44:17]

Should this actually be dislpay_rect().width() if the multi-line text actually
wraps earlier, yielding a smaller actual width? Similar question for the
single-line case above.

[ckocagil, 2013/08/16 16:33:15]

Done.

+              lines().back().preceding_heights + lines().back().height);
+}
+
 int RenderTextWin::GetBaseline() {
   EnsureLayout();
   return common_baseline_;
 }
 
 SelectionModel RenderTextWin::FindCursorPosition(const Point& point) {
   if (text().empty())
     return SelectionModel();
 
   EnsureLayout();
@@ skipping 138 matching lines @@
                    GetGlyphXBoundary(run, layout_index, true));
 }
 
 std::vector<Rect> RenderTextWin::GetSubstringBounds(const ui::Range& range) {
   DCHECK(!needs_layout_);
   DCHECK(ui::Range(0, text().length()).Contains(range));
   ui::Range layout_range(TextIndexToLayoutIndex(range.start()),
                          TextIndexToLayoutIndex(range.end()));
   DCHECK(ui::Range(0, GetLayoutText().length()).Contains(layout_range));
 
-  std::vector<Rect> bounds;
+  std::vector<ui::Range> bounds;

[msw, 2013/08/15 02:44:17]

nit: declare |bounds| after the early return.

[ckocagil, 2013/08/16 16:33:15]

Done.

+  std::vector<Rect> rects;
   if (layout_range.is_empty())
-    return bounds;
+    return rects;
 
-  // 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]];
     ui::Range intersection = run->range.Intersect(layout_range);
     if (intersection.IsValid()) {
       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());
+      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 {
   DCHECK_LE(index, text().length());
   ptrdiff_t i = obscured() ? ui::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);
 }
 
@@ skipping 21 matching lines @@
          position < LayoutIndexToTextIndex(GetLayoutText().length()) &&
          GetGlyphBounds(position) != GetGlyphBounds(position - 1);
 }
 
 void RenderTextWin::ResetLayout() {
   // Layout is performed lazily as needed for drawing/metrics.
   needs_layout_ = true;
 }
 
 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())
+    ComputeLines();
+}
+
+void RenderTextWin::ComputeLines() {

[msw, 2013/08/15 02:44:17]

Inline this in EnsureLayout

[ckocagil, 2013/08/16 16:33:15]

Done.

+  DCHECK(!needs_layout_);
+  std::vector<internal::Line> lines;
+  internal::LineBreaker line_breaker(display_rect().width(), multiline(),
+                                     multiline() ? &GetLineBreaks() : 0);

[msw, 2013/08/15 02:44:17]

Use NULL, not 0. Moreso, avoid LineBreaker for single-line scenarios.

[ckocagil, 2013/08/16 16:33:15]

Replaced 0 with NULL.

I'm using LineBreaker for single-line to have a single drawing path. Why are you
suggesting this? If it's for performance, LineBreaker doesn't do any text
inspection (or actual line breaking) in the single-line case, so I'm not sure if
it's worth having two paths for the performance gain here.

[msw, 2013/08/16 19:05:02]

It's just weird to create and use a LineBreaker to not breaks lines.
I guess it's fine with a comment or renamed LineMaker, LineGenerator, etc.

[ckocagil, 2013/08/20 21:25:07]

Expanded the comment above the definition of LineBreaker. Looks better now?

+  for (size_t i = 0; i < runs_.size(); ++i)
+    line_breaker.AddRun(*runs_[visual_to_logical_[i]]);
+  line_breaker.Finalize(&lines);
+  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());
 
   std::vector<SkPoint> pos;
 
   internal::SkiaTextRenderer renderer(canvas);
   ApplyFadeEffects(&renderer);
   ApplyTextShadows(&renderer);
 
   bool smoothing_enabled;
   bool cleartype_enabled;
   GetCachedFontSmoothingSettings(&smoothing_enabled, &cleartype_enabled);
   // Note that |cleartype_enabled| corresponds to Skia's |enable_lcd_text|.
   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]];
+  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/08/15 02:44:17]

Hmm, I'm not clear on why we add the baseline to text_offset.

[ckocagil, 2013/08/16 16:33:15]

AFAICT Skia vertically aligns (0, 0) with the baseline, so not adding the
baseline would draw the ascent above 0 and the ascent wouldn't be visible.

+    int preceding_segment_widths = 0;
 
-    // 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;
+    // Skip painting empty lines or lines outside the display rect area.
+    if (!display_rect().Intersects(Rect(PointAtOffsetFromOrigin(line_offset),
+                                        Size(line.width, line.height))))
       continue;
+
+    for (size_t j = 0; j < line.segments.size(); ++j) {
+      const internal::LineSegmentWin* segment =
+          static_cast<internal::LineSegmentWin*>(line.segments[j]);
+      const int segment_width = segment->x_pos.length();
+      const internal::TextRun* run = segment->run;
+      DCHECK(!segment->char_pos.is_empty());
+      DCHECK(run->range.Contains(segment->char_pos));
+      ui::Range glyphs = CharRangeToGlyphRange(*run, segment->char_pos);
+      if (glyphs.is_empty()) {
+        DCHECK(segment_width == 0);
+        continue;
+      }
+      // Skip painting segments outside the display rect area.

[msw, 2013/08/15 02:44:17]

This should only happen for visible lines if the line actually extended beyond
the display rect width, which should only happen for the single-line case (since
we don't allow multi-line lines to extend beyond the display-rect width at the
moment). So I think you can wrap |segment_bounds| and this check in a
!multiline() check for performance. (if you think it's better to support the
multiline case where lines may extend beyond the display_rect width, then this
might be okay as-is)

[ckocagil, 2013/08/16 16:33:15]

Done.

+      Rect segment_bounds(PointAtOffsetFromOrigin(line_offset) +

[msw, 2013/08/15 02:44:17]

nit: const

[ckocagil, 2013/08/16 16:33:15]

Done.

+                              Vector2d(preceding_segment_widths, 0),
+                          Size(segment_width, line.height));
+      if (!display_rect().Intersects(segment_bounds)) {
+        preceding_segment_widths += segment_width;
+        continue;
+      }
+
+      int segment_x = 0;
+      pos.resize(glyphs.length());

[msw, 2013/08/15 02:44:17]

Does it help to keep using a terminal pos entry?

[ckocagil, 2013/08/16 16:33:15]

I don't see a place where it would.

[msw, 2013/08/16 19:05:02]

At the SkScalar width = (colored_glyphs.end() < glyphs.end() ? ... : ...)
I think an earlier patch set added the terminal value for this case.

[ckocagil, 2013/08/20 00:09:06]

Done.

+      for (size_t g = glyphs.start(); g < glyphs.end(); ++g) {
+        pos[g - glyphs.start()].set(
+            SkIntToScalar(text_offset.x() + preceding_segment_widths +
+                          segment_x + run->offsets[g].du),
+            SkIntToScalar(text_offset.y() + run->offsets[g].dv));
+        segment_x += run->advance_widths[g];
+      }
+
+      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) {
+        ui::Range intersection = colors().GetRange(it).Intersect(

[msw, 2013/08/15 02:44:17]

nit: const

[ckocagil, 2013/08/16 16:33:15]

Done.

+            segment->char_pos);
+        ui::Range colored_glyphs = CharRangeToGlyphRange(*run, intersection);

[msw, 2013/08/15 02:44:17]

nit: const

[ckocagil, 2013/08/16 16:33:15]

Done.

+        DCHECK(glyphs.Contains(colored_glyphs));
+        if (colored_glyphs.is_empty())
+          continue;
+        renderer.SetForegroundColor(it->second);
+        renderer.DrawPosText(&pos[colored_glyphs.start() - glyphs.start()],
+                             &run->glyphs[colored_glyphs.start()],
+                             colored_glyphs.length());
+        SkScalar width = (colored_glyphs.end() < glyphs.end()

[msw, 2013/08/15 02:44:17]

nit: const

[ckocagil, 2013/08/16 16:33:15]

Done.

+                          ? pos[colored_glyphs.end() - glyphs.start()].x()

[msw, 2013/08/15 02:44:17]

nit: '?' goes on the line above, ':' goes at the end of this line, and both this
line and the line below should be indented 4 more spaces than the ternary's
conditional, I think :P

[ckocagil, 2013/08/16 16:33:15]

Done.

+                          : pos[0].x() + SkIntToScalar(segment_width))
+            - pos[colored_glyphs.start() - glyphs.start()].x();
+        renderer.DrawDecorations(
+            pos[colored_glyphs.start() - glyphs.start()].x(), text_offset.y(),
+            SkScalarCeilToInt(width), run->underline, run->strike,
+            run->diagonal_strike);
+      }
+
+      preceding_segment_widths += segment_width;
     }
-
-    // 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);
-    }
-
-    DCHECK_EQ(glyph_x - x, run->width);
-    x = glyph_x;
   }
 
   UndoCompositionAndSelectionStyles();
 }
 
 void RenderTextWin::ItemizeLogicalText() {
   runs_.clear();
   // Make |string_size_|'s height and |common_baseline_| tall enough to draw
   // often-used characters which are rendered with fonts in the font list.
   string_size_ = Size(0, font_list().GetHeight());
@@ skipping 357 matching lines @@
   size_t position = LayoutIndexToTextIndex(run->range.end());
   position = IndexOfAdjacentGrapheme(position, CURSOR_BACKWARD);
   return SelectionModel(position, CURSOR_FORWARD);
 }
 
 RenderText* RenderText::CreateInstance() {
   return new RenderTextWin;
 }
 
 }  // namespace gfx