Replace StyleRange with BreakList; update RenderText, etc.

ui/gfx/render_text_linux.cc

Index: ui/gfx/render_text_linux.cc
diff --git a/ui/gfx/render_text_linux.cc b/ui/gfx/render_text_linux.cc
index 3d3cdefa62618e4a5441799c6830cccfa211e468..7afb9f54b40c8f7a01f670e50055acc7fc547d8f 100644
--- a/ui/gfx/render_text_linux.cc
+++ b/ui/gfx/render_text_linux.cc
@@ -325,26 +325,35 @@ void RenderTextLinux::EnsureLayout() {
 void RenderTextLinux::SetupPangoAttributes(PangoLayout* layout) {

[Alexei Svitkine (slow), 2013/01/29 22:07:25]

Is it possible to add a test for this function?

Something simple that does some styles and then queries the PangoAttributes that
got set on the pango layout?

[msw, 2013/01/31 01:48:54]

Done.

   PangoAttrList* attrs = pango_attr_list_new();
 
-  int default_font_style = font_list().GetFontStyle();
-  for (StyleRanges::const_iterator i = style_ranges().begin();
-       i < style_ranges().end(); ++i) {
-    // In Pango, different fonts means different runs, and it breaks Arabic
-    // shaping across run boundaries. So, set font only when it is different
-    // from the default font.
-    // TODO(xji): We'll eventually need to split up StyleRange into components
-    // (ColorRange, FontRange, etc.) so that we can combine adjacent ranges
-    // with the same Fonts (to avoid unnecessarily splitting up runs).
-    if (i->font_style != default_font_style) {
-      FontList derived_font_list = font_list().DeriveFontList(i->font_style);
+  // Splitting text runs to accommodate styling can break Arabic glyph shaping.
+  // Only split text runs as needed for bold and italic font styles changes.
+  BreakList<bool>::const_iterator bold = styles()[BOLD].breaks().begin();
+  BreakList<bool>::const_iterator italic = styles()[ITALIC].breaks().begin();
+  while (bold != styles()[BOLD].breaks().end() &&
+         italic != styles()[ITALIC].breaks().end()) {
+    const int style = (bold->second ? Font::BOLD : 0) |
+                      (italic->second ? Font::ITALIC : 0);
+    const size_t bold_end = TextIndexToLayoutIndex(std::min(text().length(),

[Alexei Svitkine (slow), 2013/01/29 22:07:25]

Can you add the std::min() logic to style.GetNextBreak() instead of repeating it
everywhere you use it?

e.g. you can pass text() as a param when constructing the iterator or make a
function in RenderText that returns it to avoid callers from having to pass all
the params themselves.

Or even alternatively, have the iterator include a pointer to the RenderText
instance, then it could even include the TextIndexToLayoutIndex() conversions.

[msw, 2013/01/31 01:48:54]

I revised BreakList to support a max value.
StyleIterator shouldn't know about RenderText index conversion.

+        styles()[BOLD].GetBreakEnd(bold)));
+    const size_t italic_end = TextIndexToLayoutIndex(std::min(text().length(),
+        styles()[ITALIC].GetBreakEnd(italic)));
+    const size_t end = std::min(bold_end, italic_end);
+    if (style != Font::NORMAL) {

[Alexei Svitkine (slow), 2013/01/29 22:07:25]

Shouldn't you compare it to the style of the font list like the previous code
was doing? e.g. if font_list is ITALIC and your new style is normal, this
wouldn't do the right thing.

[msw, 2013/01/31 01:48:54]

Done.

+      FontList derived_font_list = font_list().DeriveFontList(style);
       ScopedPangoFontDescription desc(pango_font_description_from_string(
           derived_font_list.GetFontDescriptionString().c_str()));
 
       PangoAttribute* pango_attr = pango_attr_font_desc_new(desc.get());
-      pango_attr->start_index = TextIndexToLayoutIndex(i->range.start());
-      pango_attr->end_index = TextIndexToLayoutIndex(i->range.end());
+      pango_attr->start_index =
+          TextIndexToLayoutIndex(std::max(bold->first, italic->first));
+      pango_attr->end_index = end;
       pango_attr_list_insert(attrs, pango_attr);
     }
+    bold += bold_end == end ? 1 : 0;
+    italic += italic_end == end ? 1 : 0;
   }
+  DCHECK(bold == styles()[BOLD].breaks().end());
+  DCHECK(italic == styles()[ITALIC].breaks().end());
 
   pango_layout_set_attributes(layout, attrs);
   pango_attr_list_unref(attrs);
@@ -363,20 +372,6 @@ void RenderTextLinux::DrawVisualText(Canvas* canvas) {
   std::vector<SkPoint> pos;
   std::vector<uint16> glyphs;
 
-  StyleRanges styles(style_ranges());
-  ApplyCompositionAndSelectionStyles(&styles);
-
-  // Pre-calculate UTF8 indices from UTF16 indices.
-  // TODO(asvitkine): Can we cache these?
-  std::vector<ui::Range> style_ranges_utf8;
-  style_ranges_utf8.reserve(styles.size());
-  size_t start_index = 0;
-  for (size_t i = 0; i < styles.size(); ++i) {
-    size_t end_index = TextIndexToLayoutIndex(styles[i].range.end());
-    style_ranges_utf8.push_back(ui::Range(start_index, end_index));
-    start_index = end_index;
-  }
-
   internal::SkiaTextRenderer renderer(canvas);
   ApplyFadeEffects(&renderer);
   ApplyTextShadows(&renderer);
@@ -391,27 +386,16 @@ void RenderTextLinux::DrawVisualText(Canvas* canvas) {
       render_params.antialiasing,
       use_subpixel_rendering && !background_is_transparent());
 
+  // Temporarily apply composition underlines and selection colors.
+  ApplyCompositionAndSelectionStyles();
+
+  internal::StyleIterator style(colors(), styles());
   for (GSList* it = current_line_->runs; it; it = it->next) {
     PangoLayoutRun* run = reinterpret_cast<PangoLayoutRun*>(it->data);
     int glyph_count = run->glyphs->num_glyphs;
     if (glyph_count == 0)
       continue;
 
-    size_t run_start = run->item->offset;
-    size_t first_glyph_byte_index = run_start + run->glyphs->log_clusters[0];
-    size_t style_increment = IsForwardMotion(CURSOR_RIGHT, run->item) ? 1 : -1;
-
-    // Find the initial style for this run.
-    // TODO(asvitkine): Can we avoid looping here, e.g. by caching this per run?
-    int style = -1;
-    for (size_t i = 0; i < style_ranges_utf8.size(); ++i) {
-      if (IndexInRange(style_ranges_utf8[i], first_glyph_byte_index)) {
-        style = i;
-        break;
-      }
-    }
-    DCHECK_GE(style, 0);
-
     ScopedPangoFontDescription desc(
         pango_font_describe(run->item->analysis.font));
 
@@ -429,50 +413,57 @@ void RenderTextLinux::DrawVisualText(Canvas* canvas) {
     for (int i = 0; i < glyph_count; ++i) {
       const PangoGlyphInfo& glyph = run->glyphs->glyphs[i];
       glyphs[i] = static_cast<uint16>(glyph.glyph);
-      // Use pango_units_to_double() rather than PANGO_PIXELS() here so that
-      // units won't get rounded to the pixel grid if we're using subpixel
-      // positioning.
+      // Use pango_units_to_double() rather than PANGO_PIXELS() here so units
+      // are not rounded to the pixel grid if subpixel positioning is enabled.
       pos[i].set(glyph_x + pango_units_to_double(glyph.geometry.x_offset),
                  y + pango_units_to_double(glyph.geometry.y_offset));
 
-      // If this glyph is beyond the current style, draw the glyphs so far and
-      // advance to the next style.
-      size_t glyph_byte_index = run_start + run->glyphs->log_clusters[i];
-      DCHECK_GE(style, 0);
-      DCHECK_LT(style, static_cast<int>(styles.size()));
-      if (!IndexInRange(style_ranges_utf8[style], glyph_byte_index)) {
+      // Find the end of the current ranged style. If this glyph is beyond the
+      // current style, draw the glyphs so far and advance to the next style.
+      size_t glyph_index = run->item->offset + run->glyphs->log_clusters[i];
+      const size_t style_end_index = TextIndexToLayoutIndex(
+          std::min(text().length(), style.GetNextBreak()));
+      if (glyph_index >= style_end_index) {
         // TODO(asvitkine): For cases like "fi", where "fi" is a single glyph
         //                  but can span multiple styles, Pango splits the
         //                  styles evenly over the glyph. We can do this too by
         //                  clipping and drawing the glyph several times.
-        renderer.SetForegroundColor(styles[style].foreground);
-        renderer.SetFontFamilyWithStyle(family_name, styles[style].font_style);
+        renderer.SetForegroundColor(style.color());
+        const int font_style = (style.style(BOLD) ? Font::BOLD : 0) |
+                               (style.style(ITALIC) ? Font::ITALIC : 0);
+        renderer.SetFontFamilyWithStyle(family_name, font_style);
         renderer.DrawPosText(&pos[start], &glyphs[start], i - start);
-        if (styles[style].underline)
+        if (style.style(UNDERLINE))
           SetPangoUnderlineMetrics(desc.get(), &renderer);
-        renderer.DrawDecorations(start_x, y, glyph_x - start_x, styles[style]);
+        renderer.DrawDecorations(start_x, y, glyph_x - start_x,
+            style.style(UNDERLINE), style.style(STRIKE),
+            style.style(DIAGONAL_STRIKE));
 
         start = i;
         start_x = glyph_x;
-        // Loop to find the next style, in case the glyph spans multiple styles.
-        do {
-          style += style_increment;
-        } while (style >= 0 && style < static_cast<int>(styles.size()) &&
-                 !IndexInRange(style_ranges_utf8[style], glyph_byte_index));
+        style.UpdatePosition(LayoutIndexToTextIndex(style_end_index));

[Alexei Svitkine (slow), 2013/01/29 22:07:25]

Are you sure that |style_end_index| is correct for the next glyph?

The previous code had the line:

  size_t style_increment = IsForwardMotion(CURSOR_RIGHT, run->item) ? 1 : -1;

and used this value to iterate through the style ranges. I believe the issue
that was trying to address was that some runs may be RTL, which presumably meant
that going forward one glyph and stepping outside the current style would mean
having to decrement the style index.

I'm not sure I understand how your new code handles it, if it does. Care to
clarify?

[msw, 2013/01/31 01:48:54]

You are right, my previous patch set was wrong; thanks for the catch! I re-wrote
this code and think the result is correct a bit cleaner; hopefully you agree.

       }
 
       glyph_x += pango_units_to_double(glyph.geometry.width);
     }
 
     // Draw the remaining glyphs.
-    renderer.SetForegroundColor(styles[style].foreground);
-    renderer.SetFontFamilyWithStyle(family_name, styles[style].font_style);
+    renderer.SetForegroundColor(style.color());
+    const int font_style = (style.style(BOLD) ? Font::BOLD : 0) |
+                           (style.style(ITALIC) ? Font::ITALIC : 0);
+    renderer.SetFontFamilyWithStyle(family_name, font_style);
     renderer.DrawPosText(&pos[start], &glyphs[start], glyph_count - start);
-    if (styles[style].underline)
+    if (style.style(UNDERLINE))
       SetPangoUnderlineMetrics(desc.get(), &renderer);
-    renderer.DrawDecorations(start_x, y, glyph_x - start_x, styles[style]);
+    renderer.DrawDecorations(start_x, y, glyph_x - start_x,
+        style.style(UNDERLINE), style.style(STRIKE),
+        style.style(DIAGONAL_STRIKE));
+
     x = glyph_x;
   }
+
+  // Undo the temporarily applied composition underlines and selection colors.
+  UndoCompositionAndSelectionStyles();
 }
 
 GSList* RenderTextLinux::GetRunContainingCaret(