Make painting app list items more efficient by caching the labels.

Make painting app list items more efficient by caching the labels.


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

[benwells, 2013-03-12 04:58:11]

lgtm with a couple of nits.

https://codereview.chromium.org/12771021/diff/1/ui/app_list/views/app_list_it...
File ui/app_list/views/app_list_item_view.cc (right):

https://codereview.chromium.org/12771021/diff/1/ui/app_list/views/app_list_it...
ui/app_list/views/app_list_item_view.cc:81: title_->Invalidate();
nit: is this needed? CachedLabel initializes needs_repaint_ to true.

https://codereview.chromium.org/12771021/diff/1/ui/app_list/views/cached_label.h
File ui/app_list/views/cached_label.h (right):

https://codereview.chromium.org/12771021/diff/1/ui/app_list/views/cached_labe...
ui/app_list/views/cached_label.h:1: // Copyright (c) 2013 The Chromium Authors.
All rights reserved.
super important nit: no (c)

[xiyuan, 2013-03-12 05:12:01]

Do you have any numbers to prove/indicate that the text drawing is the bottle
neck?

Add some TRACE_EVENT and chrome:trace might tell us how much time spend in text
drawing and icon drawing etc. It might give us more insight.

If we need to make this change, let's make it windows-only. On ChromeOS, layer
is on. Moving a view does not repaint so we don't want the caching there.

[koz (OOO until 15th September), 2013-03-13 00:29:47]

On 2013/03/12 05:12:01, xiyuan wrote:
> Do you have any numbers to prove/indicate that the text drawing is the bottle
> neck?
> 
> Add some TRACE_EVENT and chrome:trace might tell us how much time spend in
text
> drawing and icon drawing etc. It might give us more insight.
> 
> If we need to make this change, let's make it windows-only. On ChromeOS, layer
> is on. Moving a view does not repaint so we don't want the caching there.

I used TRACE_EVENT to come to the conclusion that text rendering was much slower
than painting the background or the icon. I don't have the actual traces
anymore, but I could reproduce them if you're interested.

It's also noticeably faster with this change, and animations are smooth more
often than not now. There still remains some jankiness and obviously the first
paint isn't helped any by this change, but we might be able to improve that by
improving text rendering speed in general.

[koz (OOO until 15th September), 2013-03-13 00:30:36]

In the latest patch set it's win-only, too.

https://codereview.chromium.org/12771021/diff/1/ui/app_list/views/app_list_it...
File ui/app_list/views/app_list_item_view.cc (right):

https://codereview.chromium.org/12771021/diff/1/ui/app_list/views/app_list_it...
ui/app_list/views/app_list_item_view.cc:81: title_->Invalidate();
On 2013/03/12 04:58:11, benwells wrote:
> nit: is this needed? CachedLabel initializes needs_repaint_ to true.

Not needed, but needs_repaint_ could just as easily initialise to false. I also
think this adds clarity.

https://codereview.chromium.org/12771021/diff/1/ui/app_list/views/cached_label.h
File ui/app_list/views/cached_label.h (right):

https://codereview.chromium.org/12771021/diff/1/ui/app_list/views/cached_labe...
ui/app_list/views/cached_label.h:1: // Copyright (c) 2013 The Chromium Authors.
All rights reserved.
On 2013/03/12 04:58:11, benwells wrote:
> super important nit: no (c)

Done.

[xiyuan, 2013-03-13 04:14:50]

Thank you for investigating and confirming the bottle neck. 

Both my comments are optional. So LGTM

https://codereview.chromium.org/12771021/diff/7001/ui/app_list/views/cached_l...
File ui/app_list/views/cached_label.cc (right):

https://codereview.chromium.org/12771021/diff/7001/ui/app_list/views/cached_l...
ui/app_list/views/cached_label.cc:29: SkColorSetARGB(0x0, 0, 0, 0),
SkXfermode::kSrc_Mode);
Transparency might break ClearType painting and have artifacts. Suggest to paint
on opaque app list background #f5f5f5.

https://codereview.chromium.org/12771021/diff/7001/ui/app_list/views/cached_l...
ui/app_list/views/cached_label.cc:30: Label::OnPaint(canvas.get());
Are you using a SkBitmap to save some allocation? If not, think we can get rid
of SkBitmap etc (line 18-26) and simplify as:

gfx::Canvas canvas(size(), ui::SCALE_FACTOR_100P,
                   true /* is_opaque */);
canvas->FillRect(GetLocalBounds(), SkColorSetRGB(0xF5, 0xF5, 0xF5));
Label::OnPaint(&canvas);
image_ = gfx::ImageSkia(canvas.ExtractImageRep());

[koz (OOO until 15th September), 2013-03-13 04:43:28]

Thanks for the review, Xiyuan!

https://codereview.chromium.org/12771021/diff/7001/ui/app_list/views/cached_l...
File ui/app_list/views/cached_label.cc (right):

https://codereview.chromium.org/12771021/diff/7001/ui/app_list/views/cached_l...
ui/app_list/views/cached_label.cc:29: SkColorSetARGB(0x0, 0, 0, 0),
SkXfermode::kSrc_Mode);
On 2013/03/13 04:14:50, xiyuan wrote:
> Transparency might break ClearType painting and have artifacts. Suggest to
paint
> on opaque app list background #f5f5f5.

The difficulty I found with opaque backgrounds is that you need to make
everything beneath it opaque as well but currently the colour is determined by
various semi-transparent tones, some of which have the potential to animate
(though we have set the animation time to zero now).

I'm hoping this will suffice as a temporary fix until win/aura is the default.

https://codereview.chromium.org/12771021/diff/7001/ui/app_list/views/cached_l...
ui/app_list/views/cached_label.cc:30: Label::OnPaint(canvas.get());
On 2013/03/13 04:14:50, xiyuan wrote:
> Are you using a SkBitmap to save some allocation? If not, think we can get rid
> of SkBitmap etc (line 18-26) and simplify as:
> 
> gfx::Canvas canvas(size(), ui::SCALE_FACTOR_100P,
>                    true /* is_opaque */);
> canvas->FillRect(GetLocalBounds(), SkColorSetRGB(0xF5, 0xF5, 0xF5));
> Label::OnPaint(&canvas);
> image_ = gfx::ImageSkia(canvas.ExtractImageRep());

Everything I do here is out of ignorance of a better way :-) Yours is much
nicer! I've removed the SkBitmap.

[commit-bot: I haz the power, 2013-03-13 04:48:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/12771021/13001

[commit-bot: I haz the power, 2013-03-13 09:57:46]

Change committed as 187822