Replace StyleRange with BreakList; update RenderText, etc.

ui/gfx/render_text.h

Index: ui/gfx/render_text.h
diff --git a/ui/gfx/render_text.h b/ui/gfx/render_text.h
index dd86fc53dc40bf0d94621bbfe37adcb3b1371045..01f4b948b44218ef9afffb10a071a4b47d886dd4 100644
--- a/ui/gfx/render_text.h
+++ b/ui/gfx/render_text.h
@@ -18,6 +18,7 @@
 #include "third_party/skia/include/core/SkPaint.h"
 #include "third_party/skia/include/core/SkRect.h"
 #include "ui/base/range/range.h"
+#include "ui/gfx/break_list.h"
 #include "ui/gfx/font_list.h"
 #include "ui/gfx/point.h"
 #include "ui/gfx/rect.h"
@@ -37,7 +38,6 @@ namespace gfx {
 class Canvas;
 class Font;
 class RenderTextTest;
-struct StyleRange;
 
 namespace internal {
 
@@ -62,7 +62,14 @@ class SkiaTextRenderer {
   void DrawPosText(const SkPoint* pos,
                    const uint16* glyphs,
                    size_t glyph_count);
-  void DrawDecorations(int x, int y, int width, const StyleRange& style);
+  // Draw underline and strike-through text decorations.
+  // Based on |SkCanvas::DrawTextDecorations()| and constants from:
+  //   third_party/skia/src/core/SkTextFormatParams.h
+  void DrawDecorations(int x, int y, int width, bool underline, bool strike,
+                       bool diagonal_strike);
+  void DrawUnderline(int x, int y, int width);
+  void DrawStrike(int x, int y, int width) const;
+  void DrawDiagonalStrike(int x, int y, int width) const;
 
  private:
   SkCanvas* canvas_skia_;
@@ -78,21 +85,6 @@ class SkiaTextRenderer {
 
 }  // namespace internal
 
-// A visual style applicable to a range of text.
-struct UI_EXPORT StyleRange {
-  StyleRange();
-
-  SkColor foreground;
-  // A gfx::Font::FontStyle flag to specify bold and italic styles.
-  int font_style;
-  bool strike;
-  bool diagonal_strike;
-  bool underline;
-  ui::Range range;
-};
-
-typedef std::vector<StyleRange> StyleRanges;
-
 // RenderText represents an abstract model of styled text and its corresponding
 // visual layout. Support is built in for a cursor, a selection, simple styling,
 // complex scripts, and bi-directional text. Implementations provide mechanisms
@@ -157,9 +149,6 @@ class UI_EXPORT RenderText {
   bool clip_to_display_rect() const { return clip_to_display_rect_; }
   void set_clip_to_display_rect(bool clip) { clip_to_display_rect_ = clip; }
 
-  const StyleRange& default_style() const { return default_style_; }
-  void set_default_style(const StyleRange& style) { default_style_ = style; }
-
   // In an obscured (password) field, all text is drawn as asterisks or bullets.
   bool obscured() const { return obscured_; }
   void SetObscured(bool obscured);
@@ -227,11 +216,16 @@ class UI_EXPORT RenderText {
   const ui::Range& GetCompositionRange() const;
   void SetCompositionRange(const ui::Range& composition_range);
 
-  // Apply |style_range| to the internal style model.
-  void ApplyStyleRange(const StyleRange& style_range);
+  // Set the text color over the entire text or a logical character range.
+  // The |range| should be valid, non-reversed, and within [0, text().length()].
+  void SetColor(SkColor value);
+  void ApplyColor(SkColor value, const ui::Range& range);
 
-  // Apply |default_style_| over the entire text range.
-  void ApplyDefaultStyle();
+  // Set various text styles over the entire text or a logical character range.
+  // The respective |style| is applied if |value| is true, or removed if false.
+  // The |range| should be valid, non-reversed, and within [0, text().length()].
+  void SetStyle(TextStyle style, bool value);
+  void ApplyStyle(TextStyle style, bool value, const ui::Range& range);
 
   // Set the text directionality mode and get the text direction yielded.
   void SetDirectionalityMode(DirectionalityMode mode);
@@ -295,14 +289,15 @@ class UI_EXPORT RenderText {
  protected:
   RenderText();
 
+  const BreakList<SkColor>& colors() const { return colors_; }
+  const BreakList<bool>& styles(TextStyle s) const { return styles_[s]; }
+
   const Vector2d& GetUpdatedDisplayOffset();
 
   void set_cached_bounds_and_offset_valid(bool valid) {
     cached_bounds_and_offset_valid_ = valid;
   }
 
-  const StyleRanges& style_ranges() const { return style_ranges_; }
-
   // Get the selection model that visually neighbors |position| by |break_type|.
   // The returned value represents a cursor/caret position without a selection.
   SelectionModel GetAdjacentSelectionModel(const SelectionModel& current,
@@ -364,9 +359,20 @@ class UI_EXPORT RenderText {
   // Returns the text used for layout, which may be |obscured_text_|.
   const string16& GetLayoutText() const;
 
-  // Apply composition style (underline) to composition range and selection
-  // style (foreground) to selection range.
-  void ApplyCompositionAndSelectionStyles(StyleRanges* style_ranges);
+  // Apply (and undo) temporary composition underlines and selection colors.
+  void ApplyCompositionAndSelectionStyles();
+  void UndoCompositionAndSelectionStyles();
+
+  // Get the layout index of the next break from the given iterators.
+  size_t GetNextBreak(
+      const BreakList<SkColor>::const_iterator& color,
+      const BreakList<bool>::const_iterator style[NUM_TEXT_STYLES]) const;

[Alexei Svitkine (slow), 2013/01/25 13:06:56]

Can you make an inner class called StyleIterator that encapsulates this state?

e.g.

class StyleIterator {
 public:
  bool HasNext() const;
  void Advance();

  SkColor color() const;
  bool style(TextStyle) const;
  size_t position() const;

 private:
  BreakList<SkColor>::const_iterator color;
  BreakList<bool>::const_iterator style[NUM_TEXT_STYLES];
  size_t position;
};

Then iteration can be as simple as:

for (StyleIterator it = GetStyleIterator(); it.HasNext(); it.Advance()) {
  ...
}

[msw, 2013/01/25 13:33:32]

I'm going to pass for now, but will consider your suggestion in a follow-up
change, because:

1) Iteration is pretty simple as-is, and flexible enough to also account for
actual run breaks, like in RenderTextWin, where depending on the values of
|script_item_break| and GetNextBreak(), the style iterators might not need
advancing. That said, your example could be adapted to handle that case, but it
wouldn't be quite as simple.

2) I don't want to over-architect/polish the usage here, because I'm hoping to
change it more meaningfully soon. I plan to not break runs for color/adornment
changes, and to handle colors during Draw, and to ideally consolidate the
adornment drawing. These changes will alter the way style iteration is done,
making this helper class less useful or not useful at all, while another helper
design might arise.

So I don't think it's worth the effort to optimize this right now.

+
+  // Advance the given iterators (by one) that end at the supplied layout index.

[Alexei Svitkine (slow), 2013/01/28 20:52:30]

I find this wording a bit confusing - I see what you mean now by looking at the
code, but I think you can rephrase this to make it clearer.

Perhaps you can talk about what this really means instead of just what it does.
e.g. after calling this, the iterators will point to the styles for the
character at |layout_index|.

[msw, 2013/01/29 17:17:25]

Changed the comment, name, and impl in the spirit of your suggestion.
(uses the GetBreak function instead, a minor performance degradation)
(otherwise the comment/name would misrepresent actual functionality)

+  void AdvanceIterators(
+      size_t layout_index,
+      BreakList<SkColor>::const_iterator* color,
+      BreakList<bool>::const_iterator style[NUM_TEXT_STYLES]) const;

[Alexei Svitkine (slow), 2013/01/28 20:52:30]

I find the syntax of the last parameter confusing - since at first glance it
looks like the last parameter is being passed in by value (when it isn't).

Another reason I'd prefer to see a StyleIterator - that and the fact that its
init logic can be put in its constructor.

How about compromising on adding a StyleIterator as a struct so that its fields
are public, so low level access to them is still available for the particular
uses, but at least it makes using these two methods and the init code cleaner?

[msw, 2013/01/29 17:17:25]

Done; I dislike wasting effort polishing a temporary pattern, but I obliged.
This yielded something close to your earlier suggestion; a mild improvement.
(requires BreakList refs, needs index conversion, not much simpler, etc.)

 
   // Returns the text offset from the origin after applying text alignment and
   // display offset.
@@ -402,11 +408,9 @@ class UI_EXPORT RenderText {
 
  private:
   friend class RenderTextTest;
-
   FRIEND_TEST_ALL_PREFIXES(RenderTextTest, DefaultStyle);
-  FRIEND_TEST_ALL_PREFIXES(RenderTextTest, CustomDefaultStyle);
-  FRIEND_TEST_ALL_PREFIXES(RenderTextTest, ApplyStyleRange);
-  FRIEND_TEST_ALL_PREFIXES(RenderTextTest, StyleRangesAdjust);
+  FRIEND_TEST_ALL_PREFIXES(RenderTextTest, SetColorAndStyle);
+  FRIEND_TEST_ALL_PREFIXES(RenderTextTest, ApplyColorAndStyle);
   FRIEND_TEST_ALL_PREFIXES(RenderTextTest, ObscuredText);
   FRIEND_TEST_ALL_PREFIXES(RenderTextTest, GraphemePositions);
   FRIEND_TEST_ALL_PREFIXES(RenderTextTest, EdgeSelectionModels);
@@ -478,10 +482,10 @@ class UI_EXPORT RenderText {
   // Composition text range.
   ui::Range composition_range_;
 
-  // List of style ranges. Elements in the list never overlap each other.
-  StyleRanges style_ranges_;
-  // The default text style.
-  StyleRange default_style_;
+  // Color and style breaks, used to color and stylize ranges of text.
+  // TODO(msw): Expand to support cursor, selection, background, etc. colors.
+  BreakList<SkColor> colors_;
+  BreakList<bool> styles_[NUM_TEXT_STYLES];
 
   // A flag and the text to display for obscured (password) fields.
   // Asterisks are used instead of the actual text glyphs when true.