Don't split text runs on underline style change

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 f79577fde0aa9314cd61ddd7b5f209ee71ae1ae4..3950ff37e44a4c0c1009fee6ec365c7fcef9d51c 100644
--- a/ui/gfx/render_text_win.cc
+++ b/ui/gfx/render_text_win.cc
@@ -292,7 +292,6 @@ TextRun::TextRun()
   : font_style(0),
     strike(false),
     diagonal_strike(false),
-    underline(false),
     width(0),
     preceding_run_widths(0),
     glyph_count(0),
@@ -889,10 +888,33 @@ void RenderTextWin::DrawVisualText(Canvas* canvas) {
         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);
+
+        const BreakList<bool>& underline_breaks = styles()[UNDERLINE];
+        for (size_t position = intersection.start();
+                position < intersection.end(); ) {

[msw, 2014/07/08 17:20:53]

nit: the indent here should match the inside of the parentheses above.

[Tomasz Moniuszko, 2014/07/16 10:35:45]

Done.

+          const BreakList<bool>::const_iterator underline =
+              underline_breaks.GetBreak(position);
+          const Range underline_range =
+              underline_breaks.GetRange(underline).Intersect(intersection);
+          Range decorated_glyphs = CharRangeToGlyphRange(*run, underline_range);
+          if (decorated_glyphs.is_empty())

[msw, 2014/07/08 17:20:53]

nit: add {}

[msw, 2014/07/08 17:20:53]

Why do we get an empty glyph range for something that should be valid?

[Tomasz Moniuszko, 2014/07/16 10:35:45]

Done.

[Tomasz Moniuszko, 2014/07/16 10:35:45]

We can get empty glyph range for non-empty character range when script is
complex.

Example from ScriptShape() documentation
(http://msdn.microsoft.com/en-us/library/windows/desktop/dd368564(v=vs.85).aspx):

Character array, where c<n>u<m> means cluster n, Unicode code point m:
| c1u1 | c2u1 | c3u1 c3u2 c3u3 | c4u1 c4u2 |
Glyph array, where c<n>g<m> means cluster n, glyph m:
| c1g1 | c2g1 c2g2 c2g3 | c3g1 | c4g1 c4g2 c4g3 |
Cluster array, that is, the offset (in glyphs) to the cluster containing the
character:
| 0 | 1 | 4 4 4 | 5 5 |

With this example CharRangeToGlyphRange() would return glyph range [4, 4)
(empty) for character range [3, 4) (non-empty).

[msw, 2014/07/16 19:52:38]

I suspect that RenderTextWin::CharRangeToGlyphRange is wrong and needs a fix,
like Cem's http://crrev.com/275754 for RenderTextHarfBuzz. WDYT?

+            if (run->script_analysis.fRTL)
+              decorated_glyphs.set_start(decorated_glyphs.end() - 1);

[msw, 2014/07/08 17:20:53]

Hmm, adjusting the range by one glyph seems odd, what's going on here?

[Tomasz Moniuszko, 2014/07/16 10:35:45]

Yes, I agree. The issue is that BreakList contains some underline range (in this
case the range consists of single underlined character which is a keyboard
accelerator) but we get an empty glyph range for this range. BreakList indicates
that we should underline something but glyph range is empty so there's nothing
to underline. I adjust the glyph range so one glyph gets underlined.

+            else
+              decorated_glyphs.set_end(decorated_glyphs.start() + 1);
+          const SkPoint& decorations_start_pos =
+              pos[decorated_glyphs.start() - glyph_range.start()];
+          const SkPoint& decorations_end_pos =
+              pos[decorated_glyphs.end() - glyph_range.start()];
+          const int decorations_width = SkScalarCeilToInt(
+              decorations_end_pos.x() - decorations_start_pos.x());
+          renderer.DrawDecorations(decorations_start_pos.x(), text_offset.y(),
+                                   decorations_width,
+                                   underline->second,
+                                   run->strike,
+                                   run->diagonal_strike);
+          position = underline_range.end();
+        }
       }
 
       preceding_segment_widths += segment_width;
@@ -943,7 +965,11 @@ void RenderTextWin::ItemizeLogicalText() {
   // empty color BreakList to avoid breaking runs at color boundaries.
   BreakList<SkColor> empty_colors;
   empty_colors.SetMax(layout_text_length);
-  internal::StyleIterator style(empty_colors, styles());
+  std::vector<BreakList<bool> > breaking_styles = styles();

[msw, 2014/07/08 17:20:53]

nit: add a comment like the one above for colors (or extend that to mention the
reason for modifying the underline BreakList).

+  size_t underline_max = breaking_styles[UNDERLINE].max();
+  breaking_styles[UNDERLINE].SetValue(false);
+  breaking_styles[UNDERLINE].SetMax(underline_max);
+  internal::StyleIterator style(empty_colors, breaking_styles);
   SCRIPT_ITEM* script_item = &script_items[0];
   const size_t max_run_length = kMaxGlyphs / 2;
   for (size_t run_break = 0; run_break < layout_text_length;) {
@@ -956,7 +982,6 @@ void RenderTextWin::ItemizeLogicalText() {
                           run->font_style, &run->font);
     run->strike = style.style(STRIKE);
     run->diagonal_strike = style.style(DIAGONAL_STRIKE);
-    run->underline = style.style(UNDERLINE);
     run->script_analysis = script_item->a;
 
     // Find the next break and advance the iterators as needed.