Don't paint text outside display area

Don't paint text outside display area

When rendering text, we don't have to render glyphs that fall outside of
the display area. This change stops processing glyps when we're outside
the display area, and stops processing further style ranges if those are
outside the display area. Note that the equivalent functionality is
already in place for Windows text rendering.

This fixes an issue where text rendering would become very slow when
rendering big strings.

BUG=413540
Committed: https://crrev.com/88328875c41317c728a20b78308218c176a6ee72
Cr-Commit-Position: refs/heads/master@{#295023}

[Arjan van Leeuwen, 2014-09-05 13:14:35]

arjanl@opera.com changed reviewers:
+ msw@chromium.org

[Arjan van Leeuwen, 2014-09-05 13:15:14]

This fixes a performance issue we had with very long URLs in the address bar. Is
this OK?

[msw, 2014-09-05 20:15:32]

This seems fine other than the |glyph_text_index| change.
It'd be even nicer if we could check for any rect intersection like
RenderTextWin (not just checking the glyph x position against the right edge of
the display rect), but perhaps that would require additional pre-computation of
segment/run bounds.

https://codereview.chromium.org/543073002/diff/1/ui/gfx/render_text_pango.cc
File ui/gfx/render_text_pango.cc (right):

https://codereview.chromium.org/543073002/diff/1/ui/gfx/render_text_pango.cc#...
ui/gfx/render_text_pango.cc:454: SkScalarTruncToInt(x) >=
display_rect().right())
Truncation doesn't seem correct here; why do we need this change?

[Arjan van Leeuwen, 2014-09-08 09:20:11]

https://codereview.chromium.org/543073002/diff/1/ui/gfx/render_text_pango.cc
File ui/gfx/render_text_pango.cc (right):

https://codereview.chromium.org/543073002/diff/1/ui/gfx/render_text_pango.cc#...
ui/gfx/render_text_pango.cc:454: SkScalarTruncToInt(x) >=
display_rect().right())
On 2014/09/05 20:15:32, msw wrote:
> Truncation doesn't seem correct here; why do we need this change?

This is what actually stops it from rendering glyphs outside the area (the
change above only activates if the start of the line is already outside the
display area, so that we don't have to paint the current line at all).

We set the index to style_range.end() if we've either reached the end of the
line (glyph_index == glyph_count, as before) or the current and subsequent
characters will fall out of the display area (new).

The value is truncated because if for example we have a unit that starts at
(double) 1.7, and (int) display_rect.right() is 2, we still probably need to
draw the glyph (after all it's part of the interval 1-2), so rounding it up
would be wrong but truncation means there's no risk of accidentally not painting
a glyph that needed painting.

[msw, 2014-09-08 19:10:30]

https://codereview.chromium.org/543073002/diff/1/ui/gfx/render_text_pango.cc
File ui/gfx/render_text_pango.cc (right):

https://codereview.chromium.org/543073002/diff/1/ui/gfx/render_text_pango.cc#...
ui/gfx/render_text_pango.cc:413: // Don't do anything if we're outside the
display area
nit: "Skip painting runs outside the display area."

https://codereview.chromium.org/543073002/diff/1/ui/gfx/render_text_pango.cc#...
ui/gfx/render_text_pango.cc:454: SkScalarTruncToInt(x) >=
display_rect().right())
On 2014/09/08 09:20:11, Arjan van Leeuwen wrote:
> On 2014/09/05 20:15:32, msw wrote:
> > Truncation doesn't seem correct here; why do we need this change?
> 
> This is what actually stops it from rendering glyphs outside the area (the
> change above only activates if the start of the line is already outside the
> display area, so that we don't have to paint the current line at all).
> 
> We set the index to style_range.end() if we've either reached the end of the
> line (glyph_index == glyph_count, as before) or the current and subsequent
> characters will fall out of the display area (new).
> 
> The value is truncated because if for example we have a unit that starts at
> (double) 1.7, and (int) display_rect.right() is 2, we still probably need to
> draw the glyph (after all it's part of the interval 1-2), so rounding it up
> would be wrong but truncation means there's no risk of accidentally not
painting
> a glyph that needed painting.

Ah, your change below to the while loop on [new] line 479/480 doesn't suffice
for early termination alone, because it would leave the last range of styled
glyphs unpainted, right? I think that's what I didn't catch on my first review.

Still, modifying |glyph_text_index| seems wrong, can you instead modify the if
statement below to if (!IndexInRange(style_range, glyph_text_index) ||
SkScalarTruncToInt(x) >= display_rect().right())?

Add a comment to explain the early termination either way.

[Arjan van Leeuwen, 2014-09-09 07:03:59]

https://codereview.chromium.org/543073002/diff/1/ui/gfx/render_text_pango.cc
File ui/gfx/render_text_pango.cc (right):

https://codereview.chromium.org/543073002/diff/1/ui/gfx/render_text_pango.cc#...
ui/gfx/render_text_pango.cc:454: SkScalarTruncToInt(x) >=
display_rect().right())
On 2014/09/08 19:10:30, msw wrote:
> On 2014/09/08 09:20:11, Arjan van Leeuwen wrote:
> > On 2014/09/05 20:15:32, msw wrote:
> > > Truncation doesn't seem correct here; why do we need this change?
> > 
> > This is what actually stops it from rendering glyphs outside the area (the
> > change above only activates if the start of the line is already outside the
> > display area, so that we don't have to paint the current line at all).
> > 
> > We set the index to style_range.end() if we've either reached the end of the
> > line (glyph_index == glyph_count, as before) or the current and subsequent
> > characters will fall out of the display area (new).
> > 
> > The value is truncated because if for example we have a unit that starts at
> > (double) 1.7, and (int) display_rect.right() is 2, we still probably need to
> > draw the glyph (after all it's part of the interval 1-2), so rounding it up
> > would be wrong but truncation means there's no risk of accidentally not
> painting
> > a glyph that needed painting.
> 
> Ah, your change below to the while loop on [new] line 479/480 doesn't suffice
> for early termination alone, because it would leave the last range of styled
> glyphs unpainted, right? I think that's what I didn't catch on my first
review.
> 
> Still, modifying |glyph_text_index| seems wrong, can you instead modify the if
> statement below to if (!IndexInRange(style_range, glyph_text_index) ||
> SkScalarTruncToInt(x) >= display_rect().right())?
> 
> Add a comment to explain the early termination either way.

Right. Added some comments now.

[msw, 2014-09-09 18:20:22]

Great, LGTM, thanks!

[Alexei Svitkine (slow), 2014-09-09 19:26:30]

Deferring to msw's review % my nit below.

Is there a crbug.com on file for the issue this is fixing?

https://codereview.chromium.org/543073002/diff/20001/ui/gfx/render_text_pango.cc
File ui/gfx/render_text_pango.cc (right):

https://codereview.chromium.org/543073002/diff/20001/ui/gfx/render_text_pango...
ui/gfx/render_text_pango.cc:480: // Terminates loop when the end of the range
has been reached or the next
Nit: Indent comment 2 more.

[Arjan van Leeuwen, 2014-09-10 06:36:10]

On 2014/09/09 19:26:30, Alexei Svitkine wrote:
> Deferring to msw's review % my nit below.
> 
> Is there a http://crbug.com on file for the issue this is fixing?

I don't know about any crbug.com for this, and couldn't find one with a quick
search.

[Alexei Svitkine (slow), 2014-09-10 13:43:44]

Could you file one then and then associate with the CL?

On Wed, Sep 10, 2014 at 2:36 AM, <arjanl@opera.com> wrote:

> https://codereview.chromium.org/543073002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Arjan van Leeuwen, 2014-09-12 06:28:29]

On 2014/09/10 13:43:44, Alexei Svitkine wrote:
> Could you file one then and then associate with the CL?

Done: http://crbug.com/413540 .

[Alexei Svitkine (slow), 2014-09-12 15:32:32]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2014-09-12 15:32:33]

Thanks, LGTM!

[Arjan van Leeuwen, 2014-09-13 14:36:04]

The CQ bit was checked by arjanl@opera.com

[commit-bot: I haz the power, 2014-09-13 14:36:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/543073002/40001

[commit-bot: I haz the power, 2014-09-13 16:36:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-13 16:36:54]

Exceeded time limit waiting for builds to trigger.

[Arjan van Leeuwen, 2014-09-16 06:33:59]

The CQ bit was checked by arjanl@opera.com

[commit-bot: I haz the power, 2014-09-16 06:34:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/543073002/40001

[commit-bot: I haz the power, 2014-09-16 07:15:24]

Committed patchset #3 (id:40001) as 29d646dd9a03638be85ece44929d7f5fde12d4f8

[commit-bot: I haz the power, 2014-09-16 07:16:43]

Patchset 3 (id:??) landed as
https://crrev.com/88328875c41317c728a20b78308218c176a6ee72
Cr-Commit-Position: refs/heads/master@{#295023}