Replace StyleRange with BreakList; update RenderText, etc.

Replace StyleRange with BreakList; update RenderText, etc.

This is a functional rewrite with no observable behavior/appearance changes.
(it helps by merging adjacent equivalent styles, reducing artificial run breaks)
(it helps disambiguate font/adornment styles for application in layout/drawing)

Remove gfx::StyleRange and its use within gfx::RenderText[Win|Linux|Mac].
Add new BreakList class for managing [ranged] colors and styles; add/update tests.

Add gfx::TextStyle enum for bold, italic, underline, strike, and diagonal strike.
Split ApplyStyleRange into [Set|Apply]Color and [Set|Apply]Style.
Split ApplyDefaultStyle and |default_style_| into the first colors_ and styles_.
Split up SkiaTextRenderer::DrawDecorations for Underline/Strike/DiagonalStrike.
Update ApplyCompositionAndSelectionStyles, add UndoCompositionAndSelectionStyles.
Add temporary StyleIterator convenience class for RenderText subclass style iteration.

Update RenderText[Win|Linux|Mac], Textfield classes, and other users.
Simplify OmniboxResultView (nix bold font, and ClassificationData).
Rename gfx::Font::FontStyle::UNDERLINE (was UNDERLINED);

TODO(followup): Only break runs for bold/italic, color/adorn while drawing.
TODO(followup): Support more custom/ranged colors; merge TextStyle/FontStyle?

BUG=90426, 164047, 131660
TEST=No observable appearance/performance/behavior changes.
R=asvitkine@chromium.org,pkasting@chromium.org,sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180067

[msw, 2013-01-22 07:23:40]

Hey everyone, please take a look at the following (or as much as you'd like):
xji: ui/gfx/render_text*
asvitkine: ui/gfx/*
pkasting: chrome/browser/ui/views/omnibox/*
sky: ash/*, ui/app_list/*, ui/views/*
Thanks!

[Alexei Svitkine (slow), 2013-01-22 15:00:12]

Can you split this into several smaller CLs so that it's easier to review?

For example, the renaming of UNDERLINED to UNDERLINE doesn't need to be in this
CL.

[sky, 2013-01-22 15:46:55]

LGTM

[msw, 2013-01-22 18:26:41]

On 2013/01/22 15:00:12, Alexei Svitkine wrote:
> Can you split this into several smaller CLs so that it's easier to review?
> 
> For example, the renaming of UNDERLINED to UNDERLINE doesn't need to be in
this
> CL.

I could split UNDERLINED, OmniboxResultView, and DrawDecorations refactorings.
However, they're pretty trivial, so I won't bother splitting them unless they're
blocking you.

[Alexei Svitkine (slow), 2013-01-22 18:29:46]

On 2013/01/22 18:26:41, msw wrote:
> On 2013/01/22 15:00:12, Alexei Svitkine wrote:
> > Can you split this into several smaller CLs so that it's easier to review?
> > 
> > For example, the renaming of UNDERLINED to UNDERLINE doesn't need to be in
> this
> > CL.
> 
> I could split UNDERLINED, OmniboxResultView, and DrawDecorations refactorings.
> However, they're pretty trivial, so I won't bother splitting them unless
they're
> blocking you.

I guess it's okay for this CL - I won't make you do more work trying to split
this, but in the future please split such changes into smaller CLs.

[msw, 2013-01-22 18:37:45]

On 2013/01/22 18:29:46, Alexei Svitkine wrote:
> I guess it's okay for this CL - I won't make you do more work trying to split
> this, but in the future please split such changes into smaller CLs.

Thanks, I'll take more care keeping my changes minimal, this CL is a bit big.

[Alexei Svitkine (slow), 2013-01-22 19:20:20]

Sorry, I haven't yet managed to get through the entire CL since it's so large,
but here's some initial comments so as not to keep you waiting.

https://codereview.chromium.org/11535014/diff/46003/ui/app_list/views/search_...
File ui/app_list/views/search_result_view.cc (right):

https://codereview.chromium.org/11535014/diff/46003/ui/app_list/views/search_...
ui/app_list/views/search_result_view.cc:80: if (it->styles &
app_list::SearchResult::Tag::DIM)
Can you make the last two if statements an "if else". (and switch the order, so
to ensure DIM continues to take precedence).

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/canvas_skia.cc#new...
ui/gfx/canvas_skia.cc:284: render_text->ApplyStyle(UNDERLINE, true, range);
Only do this if range.IsValid(). See comment for StripAcceleratorChars().

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/canvas_skia.cc#new...
ui/gfx/canvas_skia.cc:320: render_text->ApplyStyle(UNDERLINE, true, range);
Only do this if range.IsValid(). See comment for StripAcceleratorChars().

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:759: void RenderText::ApplyCompositionStyle(StyleBreaks*
underlines) {
Is this really specific to underline style?

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:775: // overtype mode. RenderTextWin should apply colors
DrawVisualText(), as done
I think you're missing the word "in":

" RenderTextWin should apply colors *in* DrawVisualText()"

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:780: ui::Range(cursor, IndexOfAdjacentGrapheme(cursor,
CURSOR_FORWARD)));
Nit: Maybe extract IndexOfAdjacentGrapheme(cursor, CURSOR_FORWARD) into a
variable to make this more readable?

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:889: CHECK(ui::Range(0, text_.length()).Contains(range));
Shouldn't all of these be DCHECK()'s?

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:88: // Each |break| applies to logical text range:
[break.first, (break+1).first)
The comment isn't clear about what the last entry applies to. Is there always a
terminator entry or is the range of the last break assumed to be the end of the
logical text. Please document.

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:92: typedef std::vector<StyleBreak> StyleBreaks;
Do we really need StyleBreaks and ColorBreaks typedefs?

I find using std::vector<ColorBreak> more readable.

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:229: // Set various text styles over the entire text or a
logical character range.
Same comment as textfield, please document |value| so that it's obvious how this
works.

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:298: // Get the end of the supplied break (the next break's
start or text length).
From reading the comment, I still have no idea what this does. Please document
this better.

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:300: size_t GetBreakEnd(const std::vector<std::pair<size_t,
T> >& breaks,
Can you make the template param the vector itself?

i.e.

size_t GetBreakEnd(const T& breaks, const typename T::const_iteratio& i) {
  ...
}

This will make it more readable, I think.

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:374: void ApplyCompositionStyle(StyleBreaks* underlines);
Is this specific to underline? If not, change the comments / names.

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:426: void SetBreaks(std::vector<std::pair<size_t, T> >*
breaks,
Is there a better name for this than SetBreaks()?

ApplyBreakOverRange()? or just ApplyBreak()?

Since you're modifying |breaks|, it should be the last parameter.

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text_unitte...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text_unitte...
ui/gfx/render_text_unittest.cc:1023: render_text->ApplyStyle(gfx::BOLD, false,
ui::Range(0, 5));
This should be NORMAL, not BOLD.

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text_win.cc...
ui/gfx/render_text_win.cc:534: const StyleBreaks*
adjusted_styles[NUM_TEXT_STYLES];
I don't get why you need this. Can you explain?

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text_win.cc...
ui/gfx/render_text_win.cc:539: adjusted_styles[UNDERLINE] =
&adjusted_underlines;
The code in the diff doesn't seem to be special-casing underline, why is the new
code doing this?

https://codereview.chromium.org/11535014/diff/46003/ui/views/controls/link.cc
File ui/views/controls/link.cc (right):

https://codereview.chromium.org/11535014/diff/46003/ui/views/controls/link.cc...
ui/views/controls/link.cc:208: if ((enabled() && underline_) !=
(font().GetStyle() & gfx::Font::UNDERLINE)) {
This is not correct.

The result of (font().GetStyle() & gfx::Font::UNDERLINE) will either be 0 or
gfx::Font::UNDERLINE.

The result of (enabled() && underline_) will either be true (1) or false (0).

Your new logic will only work if 1 == gfx::Font::UNDERLINE, which is a bad
assumption to make (and false, I think).

You can do this instead:

const bool is_underline = (font().GetStyle() & gfx::Font::UNDERLINE) ? true :
false;
if ((enabled() && underline_) != is_underline) {
  ...
}

It also sounds like this should have a test, since I assume the logic breakage
wasn't caught by any test...

https://codereview.chromium.org/11535014/diff/46003/ui/views/controls/textfie...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/11535014/diff/46003/ui/views/controls/textfie...
ui/views/controls/textfield/textfield.h:218: void SetStyle(gfx::TextStyle style,
bool value);
Please document the |value| parameter better. It wasn't immediately obvious to
me  that |value| is used to say whether the style is added or removed.

[Alexei Svitkine (slow), 2013-01-22 19:41:03]

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text_mac.cc
File ui/gfx/render_text_mac.cc (right):

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text_mac.cc...
ui/gfx/render_text_mac.cc:217: end = std::min(end, style_end[i]);
I find requiring this logic to RenderText subclasses suboptimal.

Can you make a function in the base RenderText class that computes a merged list
of style breaks instead? This way, the logic in the subclasses will remain
simple by iterating over this computed list...

[msw, 2013-01-22 22:27:24]

Alexei, please take another look; thanks!

https://codereview.chromium.org/11535014/diff/46003/ui/app_list/views/search_...
File ui/app_list/views/search_result_view.cc (right):

https://codereview.chromium.org/11535014/diff/46003/ui/app_list/views/search_...
ui/app_list/views/search_result_view.cc:80: if (it->styles &
app_list::SearchResult::Tag::DIM)
On 2013/01/22 19:20:21, Alexei Svitkine wrote:
> Can you make the last two if statements an "if else". (and switch the order,
so
> to ensure DIM continues to take precedence).

Done.

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/canvas_skia.cc
File ui/gfx/canvas_skia.cc (right):

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/canvas_skia.cc#new...
ui/gfx/canvas_skia.cc:284: render_text->ApplyStyle(UNDERLINE, true, range);
On 2013/01/22 19:20:21, Alexei Svitkine wrote:
> Only do this if range.IsValid(). See comment for StripAcceleratorChars().

Done.

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/canvas_skia.cc#new...
ui/gfx/canvas_skia.cc:320: render_text->ApplyStyle(UNDERLINE, true, range);
On 2013/01/22 19:20:21, Alexei Svitkine wrote:
> Only do this if range.IsValid(). See comment for StripAcceleratorChars().

Done.

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:759: void RenderText::ApplyCompositionStyle(StyleBreaks*
underlines) {
On 2013/01/22 19:20:21, Alexei Svitkine wrote:
> Is this really specific to underline style?

Yes.

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:775: // overtype mode. RenderTextWin should apply colors
DrawVisualText(), as done
On 2013/01/22 19:20:21, Alexei Svitkine wrote:
> I think you're missing the word "in":
> 
> " RenderTextWin should apply colors *in* DrawVisualText()"

Done.

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:780: ui::Range(cursor, IndexOfAdjacentGrapheme(cursor,
CURSOR_FORWARD)));
On 2013/01/22 19:20:21, Alexei Svitkine wrote:
> Nit: Maybe extract IndexOfAdjacentGrapheme(cursor, CURSOR_FORWARD) into a
> variable to make this more readable?

Done.

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:889: CHECK(ui::Range(0, text_.length()).Contains(range));
On 2013/01/22 19:20:21, Alexei Svitkine wrote:
> Shouldn't all of these be DCHECK()'s?

Done (they weren't before, but making them so seems okay).

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:88: // Each |break| applies to logical text range:
[break.first, (break+1).first)
On 2013/01/22 19:20:21, Alexei Svitkine wrote:
> The comment isn't clear about what the last entry applies to. Is there always
a
> terminator entry or is the range of the last break assumed to be the end of
the
> logical text. Please document.

Done.

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:92: typedef std::vector<StyleBreak> StyleBreaks;
On 2013/01/22 19:20:21, Alexei Svitkine wrote:
> Do we really need StyleBreaks and ColorBreaks typedefs?
> 
> I find using std::vector<ColorBreak> more readable.

Done.

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:229: // Set various text styles over the entire text or a
logical character range.
On 2013/01/22 19:20:21, Alexei Svitkine wrote:
> Same comment as textfield, please document |value| so that it's obvious how
this
> works.

Done; but I hope usage is obvious, even ApplyStyle(UNDERLINE, true, range).

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:298: // Get the end of the supplied break (the next break's
start or text length).
On 2013/01/22 19:20:21, Alexei Svitkine wrote:
> From reading the comment, I still have no idea what this does. Please document
> this better.

Done.

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:300: size_t GetBreakEnd(const std::vector<std::pair<size_t,
T> >& breaks,
On 2013/01/22 19:20:21, Alexei Svitkine wrote:
> Can you make the template param the vector itself?
> 
> i.e.
> 
> size_t GetBreakEnd(const T& breaks, const typename T::const_iteratio& i) {
>   ...
> }
> 
> This will make it more readable, I think.

Done.

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:374: void ApplyCompositionStyle(StyleBreaks* underlines);
On 2013/01/22 19:20:21, Alexei Svitkine wrote:
> Is this specific to underline? If not, change the comments / names.

It is specific to underlines, left as-is.

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:426: void SetBreaks(std::vector<std::pair<size_t, T> >*
breaks,
On 2013/01/22 19:20:21, Alexei Svitkine wrote:
> Is there a better name for this than SetBreaks()?
> 
> ApplyBreakOverRange()? or just ApplyBreak()?
> 
> Since you're modifying |breaks|, it should be the last parameter.

Done.

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text_mac.cc
File ui/gfx/render_text_mac.cc (right):

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text_mac.cc...
ui/gfx/render_text_mac.cc:217: end = std::min(end, style_end[i]);
On 2013/01/22 19:41:03, Alexei Svitkine wrote:
> I find requiring this logic to RenderText subclasses suboptimal.
> 
> Can you make a function in the base RenderText class that computes a merged
list
> of style breaks instead? This way, the logic in the subclasses will remain
> simple by iterating over this computed list...

I agree, and have a TODO in the CL description for this:
TODO(followup): Make style iteration more convenient for Win/Linux/Mac impls. 
I don't see a great solution now; iterating over a combined list is a good idea,
but would need some thought (temp style ownership?, need separate iterators or
to repeatedly find each color/style break for each index?, etc.). Also
Win(+Mac?) code will soon only break for BOLD/ITALIC, and I hope to combine some
of the adornment drawing into render_text.cc if possible. I'd prefer to address
this in a followup CL, what do you think?

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text_unitte...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text_unitte...
ui/gfx/render_text_unittest.cc:1023: render_text->ApplyStyle(gfx::BOLD, false,
ui::Range(0, 5));
On 2013/01/22 19:20:21, Alexei Svitkine wrote:
> This should be NORMAL, not BOLD.

No, BOLD is the style that being changed to false.

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text_win.cc...
ui/gfx/render_text_win.cc:534: const StyleBreaks*
adjusted_styles[NUM_TEXT_STYLES];
On 2013/01/22 19:20:21, Alexei Svitkine wrote:
> I don't get why you need this. Can you explain?

Similar to the previous code, where |styles| was a local copy to which we
applied the transient composition and selection styles, I'm doing the same with
the underline and color break lists.

https://codereview.chromium.org/11535014/diff/46003/ui/gfx/render_text_win.cc...
ui/gfx/render_text_win.cc:539: adjusted_styles[UNDERLINE] =
&adjusted_underlines;
On 2013/01/22 19:20:21, Alexei Svitkine wrote:
> The code in the diff doesn't seem to be special-casing underline, why is the
new
> code doing this?

The prior code applied the transient composition underlines and selection colors
locally by copying/modifying all the style ranges together as |styles|. Here,
I'm only copying/modifying the |colors_| and |styles_[UNDERLINE]| vectors to
make their needed temporary local adjustments.

https://codereview.chromium.org/11535014/diff/46003/ui/views/controls/link.cc
File ui/views/controls/link.cc (right):

https://codereview.chromium.org/11535014/diff/46003/ui/views/controls/link.cc...
ui/views/controls/link.cc:208: if ((enabled() && underline_) !=
(font().GetStyle() & gfx::Font::UNDERLINE)) {
On 2013/01/22 19:20:21, Alexei Svitkine wrote:
> This is not correct.
> 
> The result of (font().GetStyle() & gfx::Font::UNDERLINE) will either be 0 or
> gfx::Font::UNDERLINE.
> 
> The result of (enabled() && underline_) will either be true (1) or false (0).
> 
> Your new logic will only work if 1 == gfx::Font::UNDERLINE, which is a bad
> assumption to make (and false, I think).
> 
> You can do this instead:
> 
> const bool is_underline = (font().GetStyle() & gfx::Font::UNDERLINE) ? true :
> false;
> if ((enabled() && underline_) != is_underline) {
>   ...
> }
> 
> It also sounds like this should have a test, since I assume the logic breakage
> wasn't caught by any test...

Adjusted, thanks for the catch (worked trivially in my use).

https://codereview.chromium.org/11535014/diff/46003/ui/views/controls/textfie...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/11535014/diff/46003/ui/views/controls/textfie...
ui/views/controls/textfield/textfield.h:218: void SetStyle(gfx::TextStyle style,
bool value);
On 2013/01/22 19:20:21, Alexei Svitkine wrote:
> Please document the |value| parameter better. It wasn't immediately obvious to
> me  that |value| is used to say whether the style is added or removed.

Done.

[Alexei Svitkine (slow), 2013-01-23 16:52:54]

Here's another round of comments. Still haven't gotten through the entire CL, so
expect more. :\


The more I stare at all the code dealing with breaks scattered in different
places (anon namespace, header, impl, etc.), the more I'm thinking that maybe
the following approach would be better:

Have a CompositionBreaks class, templated on T, in its own header file. Its only
member would be a vector of pair<size_T, T>. Move all the utility methods that
are currently scattered throughout your CL dealing with breaks into this class
(in the header, since it's a template).

Then, it's all self-contained, potentially re-usable, and more importantly the
contract of the class can be clearly stated in a comment about it and any
methods on it would enforce that contract (e.g. things like ApplyBreak()).

Then, you can also easily unit test it, since these functions are no longer
scattered in places that are hard to test (i.e. anon namespaces.)

WDYT?

(Obviously, if you go with the above approach, many of my comments below may no
longer apply.)

https://codereview.chromium.org/11535014/diff/45060/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/11535014/diff/45060/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:44: // Get the break at or preceeding |index|; |breaks|
must not be empty.
Please mention what units the |index| is. The way the comment currently reads
sounds like it just returns breaks->at(index).

https://codereview.chromium.org/11535014/diff/45060/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:46: typename std::vector<std::pair<size_t, T> >::iterator
GetBreak(
Can you make this templated on the vector too, to make it more readable?

Also, |breaks| should either be const or be the last param.

https://codereview.chromium.org/11535014/diff/45060/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:70: void ResizeBreaks(std::vector<std::pair<size_t, T> >*
breaks, size_t length) {
Add a comment. Also, |breaks| should be last in params list, since you're
changing it.

https://codereview.chromium.org/11535014/diff/45060/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:326: // Adjust any ranged styles and colors to accomodate
a new text size.
Nit: "text size" -> "text length" in comment, to avoid confusion with font size.

https://codereview.chromium.org/11535014/diff/45060/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/11535014/diff/45060/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:89: // The first break is always at 0; the last break
applies through the text end.
Maybe move this part of the comment to the actual declaration of the vectors,
since these typedefs only deal with a single break.

https://codereview.chromium.org/11535014/diff/45060/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:90: typedef std::pair<size_t, SkColor> ColorBreak;
Can these be inside RenderText (and possibly private/protected)? Or will
something else be using them too?

https://codereview.chromium.org/11535014/diff/45060/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:373: void ApplyCompositionStyle(std::vector<StyleBreak>*
underlines);
Please rename the method if this is specific to underline only.

https://codereview.chromium.org/11535014/diff/45060/ui/gfx/text_constants.h
File ui/gfx/text_constants.h (right):

https://codereview.chromium.org/11535014/diff/45060/ui/gfx/text_constants.h#n...
ui/gfx/text_constants.h:45: enum TextStyle {
Have you considered making these values powers of 2?

Then, for places that need to keep track of a set of styles, a flags field can
be used. (Obviously, similar to the Font constants).

This would make it so you don't need to keep a set of bools around to track
these when dealing with e.g. runs in RenderText impls.

(Of course when the param is a TextStyle, it should only be one flag, whereas an
int should be used for a set of flags, which we can document here for clarity.)

https://codereview.chromium.org/11535014/diff/45060/ui/views/controls/textfie...
File ui/views/controls/textfield/native_textfield_views.cc (right):

https://codereview.chromium.org/11535014/diff/45060/ui/views/controls/textfie...
ui/views/controls/textfield/native_textfield_views.cc:444:
GetRenderText()->SetColor(textfield_->GetTextColor());
Nit: Just call SetColor(textfield_->GetTextColor()); - which already does a
SchedulePaint().

[msw, 2013-01-25 09:10:02]

Please take another look; a lot has changed with the BreakList rewrite; thanks.

https://codereview.chromium.org/11535014/diff/45060/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/11535014/diff/45060/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:44: // Get the break at or preceeding |index|; |breaks|
must not be empty.
On 2013/01/23 16:52:54, Alexei Svitkine wrote:
> Please mention what units the |index| is. The way the comment currently reads
> sounds like it just returns breaks->at(index).

Replaced 'index' terminology with 'position' and mentions of ranged space in the
new BreakList.

https://codereview.chromium.org/11535014/diff/45060/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:46: typename std::vector<std::pair<size_t, T> >::iterator
GetBreak(
On 2013/01/23 16:52:54, Alexei Svitkine wrote:
> Can you make this templated on the vector too, to make it more readable?
> 
> Also, |breaks| should either be const or be the last param.

Moot.

https://codereview.chromium.org/11535014/diff/45060/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:70: void ResizeBreaks(std::vector<std::pair<size_t, T> >*
breaks, size_t length) {
On 2013/01/23 16:52:54, Alexei Svitkine wrote:
> Add a comment. Also, |breaks| should be last in params list, since you're
> changing it.

Moot.

https://codereview.chromium.org/11535014/diff/45060/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:326: // Adjust any ranged styles and colors to accomodate
a new text size.
On 2013/01/23 16:52:54, Alexei Svitkine wrote:
> Nit: "text size" -> "text length" in comment, to avoid confusion with font
size.

Done.

https://codereview.chromium.org/11535014/diff/45060/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/11535014/diff/45060/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:89: // The first break is always at 0; the last break
applies through the text end.
On 2013/01/23 16:52:54, Alexei Svitkine wrote:
> Maybe move this part of the comment to the actual declaration of the vectors,
> since these typedefs only deal with a single break.

Moot.

https://codereview.chromium.org/11535014/diff/45060/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:90: typedef std::pair<size_t, SkColor> ColorBreak;
On 2013/01/23 16:52:54, Alexei Svitkine wrote:
> Can these be inside RenderText (and possibly private/protected)? Or will
> something else be using them too?

Moot.

https://codereview.chromium.org/11535014/diff/45060/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:373: void ApplyCompositionStyle(std::vector<StyleBreak>*
underlines);
On 2013/01/23 16:52:54, Alexei Svitkine wrote:
> Please rename the method if this is specific to underline only.

Moot.

https://codereview.chromium.org/11535014/diff/45060/ui/gfx/text_constants.h
File ui/gfx/text_constants.h (right):

https://codereview.chromium.org/11535014/diff/45060/ui/gfx/text_constants.h#n...
ui/gfx/text_constants.h:45: enum TextStyle {
On 2013/01/23 16:52:54, Alexei Svitkine wrote:
> Have you considered making these values powers of 2?
> 
> Then, for places that need to keep track of a set of styles, a flags field can
> be used. (Obviously, similar to the Font constants).
> 
> This would make it so you don't need to keep a set of bools around to track
> these when dealing with e.g. runs in RenderText impls.
> 
> (Of course when the param is a TextStyle, it should only be one flag, whereas
an
> int should be used for a set of flags, which we can document here for
clarity.)

As we discussed offline, this would be nice, but makes indexing by style
difficult/costly; I'm open to further suggestions.

https://codereview.chromium.org/11535014/diff/45060/ui/views/controls/textfie...
File ui/views/controls/textfield/native_textfield_views.cc (right):

https://codereview.chromium.org/11535014/diff/45060/ui/views/controls/textfie...
ui/views/controls/textfield/native_textfield_views.cc:444:
GetRenderText()->SetColor(textfield_->GetTextColor());
On 2013/01/23 16:52:54, Alexei Svitkine wrote:
> Nit: Just call SetColor(textfield_->GetTextColor()); - which already does a
> SchedulePaint().

Done.

[Alexei Svitkine (slow), 2013-01-25 13:06:55]

I have an offsite today, so I won't get a chance to review this more until
Monday, but here's one suggestion I have:

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:369: const BreakList<bool>::const_iterator
style[NUM_TEXT_STYLES]) const;
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]

Thanks for the thoughts, I'll wait to hear back as you have time.

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:369: const BreakList<bool>::const_iterator
style[NUM_TEXT_STYLES]) const;
On 2013/01/25 13:06:56, Alexei Svitkine wrote:
> 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;
> &nbsp; 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()) {
>   ...
> }

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.

[Peter Kasting, 2013-01-28 00:47:38]

c/b/ui/views/omnibox LGTM

https://codereview.chromium.org/11535014/diff/52007/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/11535014/diff/52007/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:115:
font_height_(font.DeriveFont(0, gfx::BOLD).GetHeight()),
How do you know this height will not be smaller than the normal font height?

Nit: Why cache this in a member instead of doing what the code used to do?

https://codereview.chromium.org/11535014/diff/52007/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:435: if
((*j)->GetStringSize().width() > remaining_width) {
Nit: Should you pull the string width out into a temp?

https://codereview.chromium.org/11535014/diff/52007/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:466: gfx::Rect rect(left,
top, width, (*j)->GetStringSize().height());
Nit: Seems like you shouldn't call GetStringSize() twice just to pull the width
and height off.

https://codereview.chromium.org/11535014/diff/52007/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:529: if
((((*j)->GetFont().GetStyle() & gfx::BOLD) != 0 ) &&
Nit: Extra space

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

Here's another volley of comments.

Believe or not, I still haven't looked through the entire CL. :\

https://codereview.chromium.org/11535014/diff/52007/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/11535014/diff/52007/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:501: remaining_width +=
(*j)->GetStringSize().width();
The comment reads "We also add this classification's width (sans ellipsis)
back".

Your new code adds the width *with* ellipsis.

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/break_list.h
File ui/gfx/break_list.h (right):

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/break_list.h#newco...
ui/gfx/break_list.h:26: template <class T>
Nit: change "class" to "typename", since its used with primitives and not
classes.

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/break_list.h#newco...
ui/gfx/break_list.h:30: typedef typename std::vector<std::pair<size_t, T>
>::const_iterator
Nit: How about typedefing std::pair<size_t, T> to "Break" to make it so it's
just std::vector<Break> to make the code below more readable.

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/break_list.h#newco...
ui/gfx/break_list.h:37: const std::vector<std::pair<size_t, T> >& list() const {
return list_; }
Nit: Instead of naming it |list|, how about |breaks|?

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/break_list.h#newco...
ui/gfx/break_list.h:57: void SaveList();
I'd prefer if this functionality was outside of this class. Remove
DISALLOW_COPY_AND_ASSIGN(), since this class just wraps a vector of primitives,
which allows copy/assignment and let RenderText do the backup and restore.

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/break_list.h#newco...
ui/gfx/break_list.h:62: bool EqualsListForTest(const
std::vector<std::pair<size_t, T> >& list) const;
I think the convention is to suffix it with "ForTesting" rather than "ForTest".

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:516: bool invalidate = (style == BOLD) || (style ==
UNDERLINE);
Don't you mean (style == BOLD) || (style == ITALIC)?

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:739: *color += colors_.GetBreakEnd(*color) <= text_index ?
1 : 0;
Can you add a DCHECK() that the new |color| has a break end >= |text_index|.

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:741: style[i] += styles_[i].GetBreakEnd(style[i]) <=
text_index ? 1 : 0;
Also, DCHECK() here, same as my comment above.

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:755: const ui::Range range(selection().GetMin(),
selection().GetMax());
Is there a reason you can't just pass selection() to ApplyValue()?

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:766: void RenderText::UndoCompositionAndSelectionStyles()
{
Can you add a bool to keep track of whether the selection/composition styles
were applied and DCHECK() here and in ApplyCompositionAndSelectionStyles() that
we're in the right state each time?

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:371: // Advance the given iterators (by one) that end at
the supplied layout index.
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|.

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:375: BreakList<bool>::const_iterator
style[NUM_TEXT_STYLES]) const;
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?

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text_mac.cc
File ui/gfx/render_text_mac.cc (right):

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text_mac.cc...
ui/gfx/render_text_mac.cc:198: attributes_.reset(CFArrayCreateMutable(NULL,
max_style_count,
Please put the "Note:" comment above the current first line in the comment.

The previous parameter here was based on the fact that the loop below can add
at-most 3 entries to |attributes_| per each iteration of the loop.

The new breaks approach makes this hard to reserve accurately, so I suggest
passing 0 to it, which per the docs makes it so there's no limit on the
capacity.

https://developer.apple.com/library/mac/#documentation/CoreFoundation/Referen...

(And change the comment to just say "Clear the attributes storage." after the
Note: comment.)

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text_mac.cc...
ui/gfx/render_text_mac.cc:216: kCTUnderlineStyleAttributeName, underline_value);
Nit:

This is actually against the style guide, see:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls

We used to have some text on the Chromium style guide page that allowed this,
but it's no longer there. Please change the formatting back here. (The other
allowed style is one param on each line).

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text_win.cc...
ui/gfx/render_text_win.cc:559: script_item += script_item_break == run_break ? 1
: 0;
Nit: I'm not sure I like this style that you use throughout the CL. Sure, it
fits 2 lines onto one and perhaps it doesn't matter to compilers we use in terms
of the code that's generated, but I find it's more clear to use:

if (run_break == script_item_break)
  ++script_item;

[msw, 2013-01-29 17:17:24]

Comments addressed, fixing Mac RenderTextTest.StringSizeSanity crashes.
Please take another look in the mean time, I won't delete patch sets <= 10.

https://codereview.chromium.org/11535014/diff/52007/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/11535014/diff/52007/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:115:
font_height_(font.DeriveFont(0, gfx::BOLD).GetHeight()),
On 2013/01/28 00:47:38, Peter Kasting wrote:
> How do you know this height will not be smaller than the normal font height?
> 
> Nit: Why cache this in a member instead of doing what the code used to do?

Changed to take max; deriving fonts is supposedly costly.

https://codereview.chromium.org/11535014/diff/52007/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:435: if
((*j)->GetStringSize().width() > remaining_width) {
On 2013/01/28 00:47:38, Peter Kasting wrote:
> Nit: Should you pull the string width out into a temp?

Done.

https://codereview.chromium.org/11535014/diff/52007/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:466: gfx::Rect rect(left,
top, width, (*j)->GetStringSize().height());
On 2013/01/28 00:47:38, Peter Kasting wrote:
> Nit: Seems like you shouldn't call GetStringSize() twice just to pull the
width
> and height off.

Done.

https://codereview.chromium.org/11535014/diff/52007/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:501: remaining_width +=
(*j)->GetStringSize().width();
On 2013/01/28 20:52:30, Alexei Svitkine wrote:
> The comment reads "We also add this classification's width (sans ellipsis)
> back".
> 
> Your new code adds the width *with* ellipsis.

Done. Thanks for the catch!

https://codereview.chromium.org/11535014/diff/52007/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:529: if
((((*j)->GetFont().GetStyle() & gfx::BOLD) != 0 ) &&
On 2013/01/28 00:47:38, Peter Kasting wrote:
> Nit: Extra space

Done.

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/break_list.h
File ui/gfx/break_list.h (right):

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/break_list.h#newco...
ui/gfx/break_list.h:26: template <class T>
On 2013/01/28 20:52:30, Alexei Svitkine wrote:
> Nit: change "class" to "typename", since its used with primitives and not
> classes.

Done.

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/break_list.h#newco...
ui/gfx/break_list.h:30: typedef typename std::vector<std::pair<size_t, T>
>::const_iterator
On 2013/01/28 20:52:30, Alexei Svitkine wrote:
> Nit: How about typedefing std::pair<size_t, T> to "Break" to make it so it's
> just std::vector<Break> to make the code below more readable.

Done.

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/break_list.h#newco...
ui/gfx/break_list.h:37: const std::vector<std::pair<size_t, T> >& list() const {
return list_; }
On 2013/01/28 20:52:30, Alexei Svitkine wrote:
> Nit: Instead of naming it |list|, how about |breaks|?

Done.

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/break_list.h#newco...
ui/gfx/break_list.h:57: void SaveList();
On 2013/01/28 20:52:30, Alexei Svitkine wrote:
> I'd prefer if this functionality was outside of this class. Remove
> DISALLOW_COPY_AND_ASSIGN(), since this class just wraps a vector of
primitives,
> which allows copy/assignment and let RenderText do the backup and restore.

Done.

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/break_list.h#newco...
ui/gfx/break_list.h:62: bool EqualsListForTest(const
std::vector<std::pair<size_t, T> >& list) const;
On 2013/01/28 20:52:30, Alexei Svitkine wrote:
> I think the convention is to suffix it with "ForTesting" rather than
"ForTest".

Done.

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:516: bool invalidate = (style == BOLD) || (style ==
UNDERLINE);
On 2013/01/28 20:52:30, Alexei Svitkine wrote:
> Don't you mean (style == BOLD) || (style == ITALIC)?

Done; thanks for the catch!

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:739: *color += colors_.GetBreakEnd(*color) <= text_index ?
1 : 0;
On 2013/01/28 20:52:30, Alexei Svitkine wrote:
> Can you add a DCHECK() that the new |color| has a break end >= |text_index|.

Moot, now BreakList::GetBreak(), as per your comment/functionality comment.

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:741: style[i] += styles_[i].GetBreakEnd(style[i]) <=
text_index ? 1 : 0;
On 2013/01/28 20:52:30, Alexei Svitkine wrote:
> Also, DCHECK() here, same as my comment above.

Moot, now BreakList::GetBreak(), as per your comment/functionality comment.

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:755: const ui::Range range(selection().GetMin(),
selection().GetMax());
On 2013/01/28 20:52:30, Alexei Svitkine wrote:
> Is there a reason you can't just pass selection() to ApplyValue()?

ApplyValue (like ApplyStyleRangeImpl) won't correctly handle reversed ranges.

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:766: void RenderText::UndoCompositionAndSelectionStyles()
{
On 2013/01/28 20:52:30, Alexei Svitkine wrote:
> Can you add a bool to keep track of whether the selection/composition styles
> were applied and DCHECK() here and in ApplyCompositionAndSelectionStyles()
that
> we're in the right state each time?

Done.

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:371: // Advance the given iterators (by one) that end at
the supplied layout index.
On 2013/01/28 20:52:30, Alexei Svitkine wrote:
> 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|.

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)

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:375: BreakList<bool>::const_iterator
style[NUM_TEXT_STYLES]) const;
On 2013/01/28 20:52:30, Alexei Svitkine wrote:
> 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?

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.)

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text_mac.cc
File ui/gfx/render_text_mac.cc (right):

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text_mac.cc...
ui/gfx/render_text_mac.cc:198: attributes_.reset(CFArrayCreateMutable(NULL,
max_style_count,
On 2013/01/28 20:52:30, Alexei Svitkine wrote:
> Please put the "Note:" comment above the current first line in the comment.
> 
> The previous parameter here was based on the fact that the loop below can add
> at-most 3 entries to |attributes_| per each iteration of the loop.
> 
> The new breaks approach makes this hard to reserve accurately, so I suggest
> passing 0 to it, which per the docs makes it so there's no limit on the
> capacity.
> 
>
https://developer.apple.com/library/mac/#documentation/CoreFoundation/Referen...
> 
> (And change the comment to just say "Clear the attributes storage." after the
> Note: comment.)

Done (but potentially reverting in the subsequent patch set...). I'm not sure
what's causing RenderTextTest.StringSizeSanity to crash on Mac, but I'll take a
look once I get in; I can't ping/remote into my Mac box :(.

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text_mac.cc...
ui/gfx/render_text_mac.cc:216: kCTUnderlineStyleAttributeName, underline_value);
On 2013/01/28 20:52:30, Alexei Svitkine wrote:
> Nit:
> 
> This is actually against the style guide, see:
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls
> 
> We used to have some text on the Chromium style guide page that allowed this,
> but it's no longer there. Please change the formatting back here. (The other
> allowed style is one param on each line).

Done.

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://codereview.chromium.org/11535014/diff/52007/ui/gfx/render_text_win.cc...
ui/gfx/render_text_win.cc:559: script_item += script_item_break == run_break ? 1
: 0;
On 2013/01/28 20:52:30, Alexei Svitkine wrote:
> Nit: I'm not sure I like this style that you use throughout the CL. Sure, it
> fits 2 lines onto one and perhaps it doesn't matter to compilers we use in
terms
> of the code that's generated, but I find it's more clear to use:
> 
> if (run_break == script_item_break)
>   ++script_item;

Done here. I prefer my style, but I'll oblige for instances you find egregious.

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

https://codereview.chromium.org/11535014/diff/55006/ui/gfx/break_list.h
File ui/gfx/break_list.h (right):

https://codereview.chromium.org/11535014/diff/55006/ui/gfx/break_list.h#newco...
ui/gfx/break_list.h:119: typename std::vector<std::pair<size_t, T> >::iterator
BreakList<T>::GetBreak(
Nit: std::vector<Break>?

https://codereview.chromium.org/11535014/diff/55006/ui/gfx/break_list.h#newco...
ui/gfx/break_list.h:139: const std::vector<std::pair<size_t, T> >& breaks) const
{
Nit: std::vector<Break>?

https://codereview.chromium.org/11535014/diff/67006/ui/gfx/render_text_linux.cc
File ui/gfx/render_text_linux.cc (right):

https://codereview.chromium.org/11535014/diff/67006/ui/gfx/render_text_linux....
ui/gfx/render_text_linux.cc:325: void
RenderTextLinux::SetupPangoAttributes(PangoLayout* layout) {
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?

https://codereview.chromium.org/11535014/diff/67006/ui/gfx/render_text_linux....
ui/gfx/render_text_linux.cc:336: const size_t bold_end =
TextIndexToLayoutIndex(std::min(text().length(),
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.

https://codereview.chromium.org/11535014/diff/67006/ui/gfx/render_text_linux....
ui/gfx/render_text_linux.cc:341: if (style != Font::NORMAL) {
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.

https://codereview.chromium.org/11535014/diff/67006/ui/gfx/render_text_linux....
ui/gfx/render_text_linux.cc:444:
style.UpdatePosition(LayoutIndexToTextIndex(style_end_index));
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]

Comment addressed, please take another look. Alexei, I can't thank you enough
for your patience with me and your thorough review technique. Sorry if any of my
responses are anything but polite and thankful. We'll get through this CL,
buddy!

https://codereview.chromium.org/11535014/diff/55006/ui/gfx/break_list.h
File ui/gfx/break_list.h (right):

https://codereview.chromium.org/11535014/diff/55006/ui/gfx/break_list.h#newco...
ui/gfx/break_list.h:119: typename std::vector<std::pair<size_t, T> >::iterator
BreakList<T>::GetBreak(
On 2013/01/29 22:07:25, Alexei Svitkine wrote:
> Nit: std::vector<Break>?

Actually, it seems like the compiler requires:
"typename std::vector<typename BreakList<T>::Break>::iterator"
which is longer and not much clearer than
"typename std::vector<std::pair<size_t, T> >::iterator"
so I'll leave it as-is if you agree.

https://codereview.chromium.org/11535014/diff/55006/ui/gfx/break_list.h#newco...
ui/gfx/break_list.h:139: const std::vector<std::pair<size_t, T> >& breaks) const
{
On 2013/01/29 22:07:25, Alexei Svitkine wrote:
> Nit: std::vector<Break>?

Done.

https://codereview.chromium.org/11535014/diff/67006/ui/gfx/render_text_linux.cc
File ui/gfx/render_text_linux.cc (right):

https://codereview.chromium.org/11535014/diff/67006/ui/gfx/render_text_linux....
ui/gfx/render_text_linux.cc:325: void
RenderTextLinux::SetupPangoAttributes(PangoLayout* layout) {
On 2013/01/29 22:07:25, Alexei Svitkine wrote:
> 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?

Done.

https://codereview.chromium.org/11535014/diff/67006/ui/gfx/render_text_linux....
ui/gfx/render_text_linux.cc:336: const size_t bold_end =
TextIndexToLayoutIndex(std::min(text().length(),
On 2013/01/29 22:07:25, Alexei Svitkine wrote:
> 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.

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

https://codereview.chromium.org/11535014/diff/67006/ui/gfx/render_text_linux....
ui/gfx/render_text_linux.cc:341: if (style != Font::NORMAL) {
On 2013/01/29 22:07:25, Alexei Svitkine wrote:
> 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.

Done.

https://codereview.chromium.org/11535014/diff/67006/ui/gfx/render_text_linux....
ui/gfx/render_text_linux.cc:444:
style.UpdatePosition(LayoutIndexToTextIndex(style_end_index));
On 2013/01/29 22:07:25, Alexei Svitkine wrote:
> 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?

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.

[Alexei Svitkine (slow), 2013-01-31 16:33:34]

A few more comments your way. Getting close. :)

https://codereview.chromium.org/11535014/diff/68064/ash/system/user/tray_user.cc
File ash/system/user/tray_user.cc (right):

https://codereview.chromium.org/11535014/diff/68064/ash/system/user/tray_user...
ash/system/user/tray_user.cc:286: display_name.Intersect(line_range));
Nit: Align.

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:518: DCHECK(ui::Range(0,
text().length()).Contains(range));
Remove this DCHECK() since there's one in BreakList already.

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:546: DCHECK(ui::Range(0,
text().length()).Contains(range));
Remove this DCHECK() since BreakList has one already.

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:772: const ui::Range range(selection().GetMin(),
selection().GetMax());
Add a comment here explaining you're doing this because the range may be
reversed.

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:869: text_rect, left_part, right_part,
colors_.breaks().front().second);
|colors_.breaks().front().second| is quite ugly. Perhaps add a convenience
method on the BreakList or use GetBreak(0) (and possibly add a special case for
0 that avoids iterating the list backwards)?

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:501: // Color and style breaks, used to color and stylize
ranges of text.
Mention that these BreakLists use text indexes (and not layout indexes).

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text_linux.cc
File ui/gfx/render_text_linux.cc (right):

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text_linux....
ui/gfx/render_text_linux.cc:407: int glyph_end = 0;
Can you add a comment explaining what glyph_start and glyph_end mean here?

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text_linux....
ui/gfx/render_text_linux.cc:409: style.UpdatePosition(LayoutIndexToTextIndex(
You seem to use the following pattern a lot in this code:

LayoutIndexToTextIndex(run->item->offset +
run->glyphs->log_clusters[glyph_index])

Can you make a helper function that takes a run and a glyph_index and returns
the text index? e.g. GetGlyphTextIndex(run, glyph_index).

(It would have to be a member of RenderTextLinux since it has to use
LayoutIndexToTextIndex().)

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text_linux....
ui/gfx/render_text_linux.cc:422: const size_t i = (glyph_end == glyph_count) ?
style.GetRange().end() :
Nit: Change |i| to |glyph_text_index|?

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text_linux....
ui/gfx/render_text_linux.cc:422: const size_t i = (glyph_end == glyph_count) ?
style.GetRange().end() :
Cache result of style.GetRange() since you're using it multiple times and it's
not a simple getter.

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text_linux....
ui/gfx/render_text_linux.cc:425: if (i < style.GetRange().start() || i >=
style.GetRange().end()) {
Can you use !IndexInRange(style.GetRange(), i) like the old code did?

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text_mac.cc
File ui/gfx/render_text_mac.cc (right):

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text_mac.cc...
ui/gfx/render_text_mac.cc:195: end =
TextIndexToLayoutIndex(style.GetRange().end());
Again, this seems to be comparing a LayoutIndex with text_length which is a
TextIndex.

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text_win.cc...
ui/gfx/render_text_win.cc:553: TextIndexToLayoutIndex(style.GetRange().end()));
Is this really right?

Reading the text/layout index functions, it seems (correct me if I'm wrong):

- the LayoutIndex is a code point index (i.e. if there's a utf16 surrogate pair,
it counts as one code point)
- the TextIndex is just an index on the utf8 string

Give the above, you're assigning a LayoutIndex here to |run_break|, but the loop
compares |run_break| to |text_length| which is the length of the text, i.e.
utf16 length (assume the non-obscured case). So this seems wrong.

[msw, 2013-01-31 20:27:39]

Please take another look; thanks!

https://codereview.chromium.org/11535014/diff/68064/ash/system/user/tray_user.cc
File ash/system/user/tray_user.cc (right):

https://codereview.chromium.org/11535014/diff/68064/ash/system/user/tray_user...
ash/system/user/tray_user.cc:286: display_name.Intersect(line_range));
On 2013/01/31 16:33:34, Alexei Svitkine wrote:
> Nit: Align.

Done.

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:518: DCHECK(ui::Range(0,
text().length()).Contains(range));
On 2013/01/31 16:33:34, Alexei Svitkine wrote:
> Remove this DCHECK() since there's one in BreakList already.

Done.

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:546: DCHECK(ui::Range(0,
text().length()).Contains(range));
On 2013/01/31 16:33:34, Alexei Svitkine wrote:
> Remove this DCHECK() since BreakList has one already.

Done.

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:772: const ui::Range range(selection().GetMin(),
selection().GetMax());
On 2013/01/31 16:33:34, Alexei Svitkine wrote:
> Add a comment here explaining you're doing this because the range may be
> reversed.

Done.

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:869: text_rect, left_part, right_part,
colors_.breaks().front().second);
On 2013/01/31 16:33:34, Alexei Svitkine wrote:
> |colors_.breaks().front().second| is quite ugly. Perhaps add a convenience
> method on the BreakList or use GetBreak(0) (and possibly add a special case
for
> 0 that avoids iterating the list backwards)?

I suspect this use is incorrect anyway, but was copying the "default color" use.
I added a TODO to find the text color in the relevant area(s) being faded.

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:501: // Color and style breaks, used to color and stylize
ranges of text.
On 2013/01/31 16:33:34, Alexei Svitkine wrote:
> Mention that these BreakLists use text indexes (and not layout indexes).

Done.

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text_linux.cc
File ui/gfx/render_text_linux.cc (right):

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text_linux....
ui/gfx/render_text_linux.cc:407: int glyph_end = 0;
On 2013/01/31 16:33:34, Alexei Svitkine wrote:
> Can you add a comment explaining what glyph_start and glyph_end mean here?

Done.

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text_linux....
ui/gfx/render_text_linux.cc:409: style.UpdatePosition(LayoutIndexToTextIndex(
On 2013/01/31 16:33:34, Alexei Svitkine wrote:
> You seem to use the following pattern a lot in this code:
> 
> LayoutIndexToTextIndex(run->item->offset +
> run->glyphs->log_clusters[glyph_index])
> 
> Can you make a helper function that takes a run and a glyph_index and returns
> the text index? e.g. GetGlyphTextIndex(run, glyph_index).
> 
> (It would have to be a member of RenderTextLinux since it has to use
> LayoutIndexToTextIndex().)

Done. It's only used 3 times, but I obliged.

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text_linux....
ui/gfx/render_text_linux.cc:422: const size_t i = (glyph_end == glyph_count) ?
style.GetRange().end() :
On 2013/01/31 16:33:34, Alexei Svitkine wrote:
> Nit: Change |i| to |glyph_text_index|?

Done.

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text_linux....
ui/gfx/render_text_linux.cc:422: const size_t i = (glyph_end == glyph_count) ?
style.GetRange().end() :
On 2013/01/31 16:33:34, Alexei Svitkine wrote:
> Nit: Change |i| to |glyph_text_index|?

Done.

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text_linux....
ui/gfx/render_text_linux.cc:425: if (i < style.GetRange().start() || i >=
style.GetRange().end()) {
On 2013/01/31 16:33:34, Alexei Svitkine wrote:
> Can you use !IndexInRange(style.GetRange(), i) like the old code did?

Done.

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text_mac.cc
File ui/gfx/render_text_mac.cc (right):

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text_mac.cc...
ui/gfx/render_text_mac.cc:195: end =
TextIndexToLayoutIndex(style.GetRange().end());
On 2013/01/31 16:33:34, Alexei Svitkine wrote:
> Again, this seems to be comparing a LayoutIndex with text_length which is a
> TextIndex.

Done.

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://codereview.chromium.org/11535014/diff/68064/ui/gfx/render_text_win.cc...
ui/gfx/render_text_win.cc:553: TextIndexToLayoutIndex(style.GetRange().end()));
On 2013/01/31 16:33:34, Alexei Svitkine wrote:
> Is this really right?
> 
> Reading the text/layout index functions, it seems (correct me if I'm wrong):
> 
> - the LayoutIndex is a code point index (i.e. if there's a utf16 surrogate
pair,
> it counts as one code point)
> - the TextIndex is just an index on the utf8 string
> 
> Give the above, you're assigning a LayoutIndex here to |run_break|, but the
loop
> compares |run_break| to |text_length| which is the length of the text, i.e.
> utf16 length (assume the non-obscured case). So this seems wrong.

The old/new code seems wrong as you say; I'll use the layout text length.
I really dislike that obscured text complicates all this code so badly.

[Alexei Svitkine (slow), 2013-01-31 21:25:06]

LGTM with a few last comments. Thanks for addressing all the other ones!

https://codereview.chromium.org/11535014/diff/69016/ui/gfx/break_list.h
File ui/gfx/break_list.h (right):

https://codereview.chromium.org/11535014/diff/69016/ui/gfx/break_list.h#newco...
ui/gfx/break_list.h:21: // A solitary break at position 0 applies to the entire
space [0, max_).
Can you document what happens if max_ is not specified?

If it's required, document it and mention what the two constructors set it to
(in the comments for the constructors).

https://codereview.chromium.org/11535014/diff/69016/ui/gfx/break_list.h#newco...
ui/gfx/break_list.h:69: BreakList<T>::BreakList() : breaks_(1, Break(0, T())),
max_(0) {
Nit: Remove spurious indent.

https://codereview.chromium.org/11535014/diff/69016/ui/gfx/render_text_linux.cc
File ui/gfx/render_text_linux.cc (right):

https://codereview.chromium.org/11535014/diff/69016/ui/gfx/render_text_linux....
ui/gfx/render_text_linux.cc:407: // Track the glyph index and x-coordinate
ranges for each styled range.
I was looking for a comment explaining the differences between |glyph_start| and
|glyph_end| here, given the confusing code below that uses |glyph_end| to index
into |glyphs|.

I guess I'd prefer different names, e.g.:

glyph_end -> glyph_index (this is the current glyph being processed)
glyph_start -> style_start_glyph_index (corresponding to the start of the
current style)

[msw, 2013-02-01 00:08:08]

Done; I'll land with green tryjobs unless anyone has more feedback.
Thanks everyone, especially Alexei, this has been an adventure!

https://codereview.chromium.org/11535014/diff/69016/ui/gfx/break_list.h
File ui/gfx/break_list.h (right):

https://codereview.chromium.org/11535014/diff/69016/ui/gfx/break_list.h#newco...
ui/gfx/break_list.h:21: // A solitary break at position 0 applies to the entire
space [0, max_).
On 2013/01/31 21:25:07, Alexei Svitkine wrote:
> Can you document what happens if max_ is not specified?
> 
> If it's required, document it and mention what the two constructors set it to
> (in the comments for the constructors).

Done.

https://codereview.chromium.org/11535014/diff/69016/ui/gfx/break_list.h#newco...
ui/gfx/break_list.h:69: BreakList<T>::BreakList() : breaks_(1, Break(0, T())),
max_(0) {
On 2013/01/31 21:25:07, Alexei Svitkine wrote:
> Nit: Remove spurious indent.

Done.

https://codereview.chromium.org/11535014/diff/69016/ui/gfx/render_text_linux.cc
File ui/gfx/render_text_linux.cc (right):

https://codereview.chromium.org/11535014/diff/69016/ui/gfx/render_text_linux....
ui/gfx/render_text_linux.cc:407: // Track the glyph index and x-coordinate
ranges for each styled range.
On 2013/01/31 21:25:07, Alexei Svitkine wrote:
> I was looking for a comment explaining the differences between |glyph_start|
and
> |glyph_end| here, given the confusing code below that uses |glyph_end| to
index
> into |glyphs|.
> 
> I guess I'd prefer different names, e.g.:
> 
> glyph_end -> glyph_index (this is the current glyph being processed)
> glyph_start -> style_start_glyph_index (corresponding to the start of the
> current style)

Done; I also renamed x_end to match; hopefully this is clearer.

[commit-bot: I haz the power, 2013-02-01 01:30:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/11535014/57050

[commit-bot: I haz the power, 2013-02-01 04:42:21]

Change committed as 180067