Don't split text runs on underline style change

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/char_iterator.h"
@@ skipping 274 matching lines @@
 }
 
 }  // namespace
 
 namespace internal {
 
 TextRun::TextRun()
   : font_style(0),
     strike(false),
     diagonal_strike(false),
-    underline(false),
     width(0),
     preceding_run_widths(0),
     glyph_count(0),
     script_cache(NULL) {
   memset(&script_analysis, 0, sizeof(script_analysis));
   memset(&abc_widths, 0, sizeof(abc_widths));
 }
 
 TextRun::~TextRun() {
   ScriptFreeCache(&script_cache);
@@ skipping 576 matching lines @@
           continue;
         DCHECK(glyph_range.Contains(colored_glyphs));
         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);
+
+        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;
     }
 
     renderer.EndDiagonalStrike();
   }
 
   UndoCompositionAndSelectionStyles();
 }
@@ skipping 30 matching lines @@
   if (!SUCCEEDED(hr) || script_items_count <= 0)
     return;
 
   // Temporarily apply composition underlines and selection colors.
   ApplyCompositionAndSelectionStyles();
 
   // Build the list of runs from the script items and ranged styles. Use an
   // 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;) {
     internal::TextRun* run = new internal::TextRun();
     run->range.set_start(run_break);
     run->font = font_list().GetPrimaryFont();
     run->font_style = (style.style(BOLD) ? Font::BOLD : 0) |
                       (style.style(ITALIC) ? Font::ITALIC : 0);
     DeriveFontIfNecessary(run->font.GetFontSize(), run->font.GetHeight(),
                           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.
     const size_t script_item_break = (script_item + 1)->iCharPos;
     run_break = std::min(script_item_break,
                          TextIndexToLayoutIndex(style.GetRange().end()));
 
     // Clamp run lengths to avoid exceeding the maximum supported glyph count.
     if ((run_break - run->range.start()) > max_run_length) {
       run_break = run->range.start() + max_run_length;
@@ skipping 310 matching lines @@
   size_t position = LayoutIndexToTextIndex(run->range.end());
   position = IndexOfAdjacentGrapheme(position, CURSOR_BACKWARD);
   return SelectionModel(position, CURSOR_FORWARD);
 }
 
 RenderText* RenderText::CreateNativeInstance() {
   return new RenderTextWin;
 }
 
 }  // namespace gfx