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..2f08d36499b5edfeed54118b655466fde5c2fabe 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| (to avoid constructing empty lines).
+// 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 width for whitespace at the end of lines.
+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)| to the current character.
+  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,209 @@ 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. |empty_baseline| and |empty_height| are the baseline and

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

I think these will have to be |min_baseline| and |min_height| (via the font
list's height and baseline, as you're already doing), and then clamp each Line's
values to be max(line_ascent_, min_baseline_), etc. This should hopefully fix
the behavior change I commented about in the unit tests. I'm glad we had those
tests around to catch this!

[ckocagil, 2013/09/11 20:31:16]

Wouldn't this cause every line to have the same size (i.e
font_list.GetHeight())?

The best solution here would be to leave it like this and let the omnibox use
the information from GetStringSize() and GetBaseline() to calculate offsets. But
that would perhaps require changes in the Omnibox code too, and this CL is
already big. So I'm forcing the Line to have font_list's height and baseline in
single line mode, PTAL.

[msw, 2013/09/11 21:26:50]

I'm not sure what the optimal behavior is, but it's probably desirable for every
line to use the font list height/baseline as a *minimum*. I suspect this new
multiline code has a feature/bug where lines dynamically change heights as the
fonts used on each line changes (ex/ "ab" vs "えあ"); that's considered a bug for
single-line textfields, because text can 'jump' while typing. This requires some
follow-up thought/work, but what you have seems fine for now. Perhaps add a
TODO(ckocagil): Determine optimal multi-line height behavior.

[ckocagil, 2013/09/12 02:46:49]

Done, added TODO.

+// height of an empty line.
+// TODO(ckocagil): Expose the interface of this class in the header and test
+//                 this class directly.
+class LineBreaker {
+ public:
+  LineBreaker(int max_width,
+              int empty_baseline,
+              int empty_height,
+              bool multiline,
+              const BreakList<size_t>* words,
+              const ScopedVector<TextRun>& runs)
+      : max_width_(max_width),
+        empty_baseline_(empty_baseline),
+        empty_height_(empty_height),
+        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_);
+    const TextRun* const run = runs_[run_index];
+    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_, current_char, max_width_ - line_x_,
+                      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();
+      if (line_ascent_ + line_descent_ == 0) {
+        line_ascent_ = empty_baseline_;
+        line_descent_ = empty_height_ - empty_baseline_;
+      }
+      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 int empty_baseline_;
+  const int empty_height_;
+  const bool multiline_;
+  const BreakList<size_t>* const words_;
+  const ScopedVector<TextRun>& runs_;
+
+  // Stores the resulting lines.
+  std::vector<Line> lines_;
+
+  // Text space and line space x coordinates of the next segment to be added.
+  int text_x_;
+  int line_x_;
+
+  // Size of the multiline 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_;
+
+  // The current RTL run segments, to be applied by |UpdateRTLSegmentRanges()|.
+  std::vector<SegmentHandle> rtl_segments_;
+
+  DISALLOW_COPY_AND_ASSIGN(LineBreaker);
+};
+
+// For segments in the same run, checks the continuity and order of |x_range|
+// and |char_range| fields.
+void CheckLineIntegrity(const std::vector<Line>& lines,

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

Move this up into the anonymous namespace, unless it's needed in internal.

[ckocagil, 2013/09/11 20:31:16]

This is no longer called from _unittests.cc anymore. Done.

+                        const ScopedVector<TextRun>& runs) {
+  size_t previous_segment_line = 0;
+  const LineSegment* previous_segment = NULL;
+
+  for (size_t i = 0; i < lines.size(); ++i) {
+    for (size_t j = 0; j < lines[i].segments.size(); ++j) {
+      const LineSegment* segment = &lines[i].segments[j];
+      TextRun* run = runs[segment->run];
+
+      if (!previous_segment) {
+        previous_segment = segment;
+      } else if (runs[previous_segment->run] != run) {
+        previous_segment = NULL;
+      } else {
+        DCHECK_EQ(previous_segment->char_range.end(),
+                  segment->char_range.start());
+        if (!run->script_analysis.fRTL) {

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

nit: remove braces.

[ckocagil, 2013/09/11 20:31:16]

These are necessary here. DCHECK_EQ expands to a naked if block, so our else
here sticks to that block instead of our block. Yes.

Macros.

+          DCHECK_EQ(previous_segment->x_range.end(), segment->x_range.start());
+        } else {
+          DCHECK_EQ(segment->x_range.end(), previous_segment->x_range.start());
+        }
+
+        previous_segment = segment;
+        previous_segment_line = i;
+      }
+    }
+  }
+}
+
 }  // namespace internal
 
 // static
@@ -207,7 +468,6 @@ std::map<std::string, Font> RenderTextWin::successful_substitute_fonts_;
 
 RenderTextWin::RenderTextWin()
     : RenderText(),
-      common_baseline_(0),
       needs_layout_(false) {
   set_truncate_length(kMaxUniscribeTextLength);
 
@@ -222,12 +482,12 @@ RenderTextWin::~RenderTextWin() {
 
 Size RenderTextWin::GetStringSize() {
   EnsureLayout();
-  return string_size_;
+  return multiline_string_size_;
 }
 
 int RenderTextWin::GetBaseline() {
   EnsureLayout();
-  return common_baseline_;
+  return lines()[0].baseline;
 }
 
 SelectionModel RenderTextWin::FindCursorPosition(const Point& point) {
@@ -369,7 +629,7 @@ ui::Range RenderTextWin::GetGlyphBounds(size_t 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 ui::Range(string_size_.width());
+    return ui::Range(string_width_);
   internal::TextRun* run = runs_[run_index];
   const size_t layout_index = TextIndexToLayoutIndex(index);
   return ui::Range(GetGlyphXBoundary(run, layout_index, false),
@@ -383,11 +643,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 +657,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().GetMax() <= range_x.GetMin());
+      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 +715,40 @@ 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,
+                                       font_list().GetBaseline(),
+                                       font_list().GetHeight(), 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);
+#ifndef NDEBUG
+    CheckLineIntegrity(this->lines(), runs_);

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

nit: do this on |lines| before calling set_lines(&lines).

[ckocagil, 2013/09/11 20:31:16]

Done.

+#endif
+  }
 }
 
 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(!lines().empty());
 
   std::vector<SkPoint> pos;
 
@@ -479,54 +763,82 @@ void RenderTextWin::DrawVisualText(Canvas* canvas) {
   renderer.SetFontSmoothingSettings(
       smoothing_enabled, cleartype_enabled && !background_is_transparent());
 
-  ApplyCompositionAndSelectionStyles();
+  ApplyCompositionAndSelectionStyles();
+
+  for (size_t i = 0; i < lines().size(); ++i) {
+    const internal::Line& line = lines()[i];
+    const Vector2d line_offset = GetLineOffset(i);
 
-  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;
+    // Skip painting empty lines or lines outside the display rect area.
+    if (!display_rect().Intersects(Rect(PointAtOffsetFromOrigin(line_offset),
+                                        line.size)))
       continue;
-    }
+
+    const Vector2d text_offset = line_offset + Vector2d(0, line.baseline);
+    int preceding_segment_widths = 0;
+
+    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);
+      DCHECK(!glyph_range.is_empty());
+      // 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() + run->offsets[k].du + segment_x),
+            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(segment->char_range.start());
+           it != colors().breaks().end() &&
+               it->first < segment->char_range.end();
+           ++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));
+        DCHECK(!colored_glyphs.is_empty());
+        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();
@@ -534,10 +846,7 @@ void RenderTextWin::DrawVisualText(Canvas* canvas) {
 
 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());
-  common_baseline_ = font_list().GetBaseline();
+  string_width_ = 0;

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

Clear multiline_string_size_ here, this seems like the best place.

[ckocagil, 2013/09/11 20:31:16]

Done.

 
   // Set Uniscribe's base text direction.
   script_state_.uBidiLevel =
@@ -643,8 +952,6 @@ void RenderTextWin::LayoutVisualText() {
       DCHECK(SUCCEEDED(hr));
     }
   }
-  string_size_.set_height(ascent + descent);
-  common_baseline_ = ascent;
 
   // Build the array of bidirectional embedding levels.
   scoped_ptr<BYTE[]> levels(new BYTE[runs_.size()]);
@@ -669,7 +976,7 @@ void RenderTextWin::LayoutVisualText() {
     run->width = abc.abcA + abc.abcB + abc.abcC;
     preceding_run_widths += run->width;
   }
-  string_size_.set_width(preceding_run_widths);
+  string_width_ = preceding_run_widths;
 }
 
 void RenderTextWin::LayoutTextRun(internal::TextRun* run) {