cc: Recreate small layer tiles when size changes

cc: Recreate small layer tiles when size changes

This is a follow-up bugfix to r180264, which added support for using
smaller tiles to save memory if the layer size was smaller than a tile
size.  Unfortunately, this only worked if the layer size never changed
as it cleared all the tiles and didn't recreate them.

This new patch recreates tiles for the entire layer after clearing the
old ones.  If the tile size is changing, it is only going to happen for
very small layers and small numbers of tiles, so this will never in
practice be an expensive operation.

R=danakj@chromium.org
BUG=173945
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181911

[danakj, 2013-02-12 01:31:20]

https://codereview.chromium.org/12221129/diff/1/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/12221129/diff/1/cc/picture_layer_tiling.cc#ne...
cc/picture_layer_tiling.cc:95:
CreateTilesFromLayerRect(gfx::Rect(layer_bounds_));
What if you set old_layer_bounds to empty here? Would that allow better reuse of
the code below instead of early out?

[enne (OOO), 2013-02-12 01:34:39]

https://codereview.chromium.org/12221129/diff/1/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/12221129/diff/1/cc/picture_layer_tiling.cc#ne...
cc/picture_layer_tiling.cc:95:
CreateTilesFromLayerRect(gfx::Rect(layer_bounds_));
On 2013/02/12 01:31:20, danakj wrote:
> What if you set old_layer_bounds to empty here? Would that allow better reuse
of
> the code below instead of early out?

Hmm.  I'm not sure the single line of reuse makes it worth the extra
complication.

[danakj, 2013-02-12 01:36:24]

https://codereview.chromium.org/12221129/diff/1/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/12221129/diff/1/cc/picture_layer_tiling.cc#ne...
cc/picture_layer_tiling.cc:95:
CreateTilesFromLayerRect(gfx::Rect(layer_bounds_));
On 2013/02/12 01:34:39, enne wrote:
> On 2013/02/12 01:31:20, danakj wrote:
> > What if you set old_layer_bounds to empty here? Would that allow better
reuse
> of
> > the code below instead of early out?
> 
> Hmm.  I'm not sure the single line of reuse makes it worth the extra
> complication.

For instance, if you did it that way, you'd also get an Invalidate over the
whole layer. Why do you want to skip that here?

[danakj, 2013-02-12 01:37:56]

https://codereview.chromium.org/12221129/diff/1/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/12221129/diff/1/cc/picture_layer_tiling.cc#ne...
cc/picture_layer_tiling.cc:95:
CreateTilesFromLayerRect(gfx::Rect(layer_bounds_));
On 2013/02/12 01:36:24, danakj wrote:
> On 2013/02/12 01:34:39, enne wrote:
> > On 2013/02/12 01:31:20, danakj wrote:
> > > What if you set old_layer_bounds to empty here? Would that allow better
> reuse
> > of
> > > the code below instead of early out?
> > 
> > Hmm.  I'm not sure the single line of reuse makes it worth the extra
> > complication.
> 
> For instance, if you did it that way, you'd also get an Invalidate over the
> whole layer. Why do you want to skip that here?

(Especially considering Justin's desire to record somehow based on the tile
size).

[enne (OOO), 2013-02-12 01:43:18]

On 2013/02/12 01:36:24, danakj wrote:
> https://codereview.chromium.org/12221129/diff/1/cc/picture_layer_tiling.cc
> File cc/picture_layer_tiling.cc (right):
> 
>
https://codereview.chromium.org/12221129/diff/1/cc/picture_layer_tiling.cc#ne...
> cc/picture_layer_tiling.cc:95:
> CreateTilesFromLayerRect(gfx::Rect(layer_bounds_));
> On 2013/02/12 01:34:39, enne wrote:
> > On 2013/02/12 01:31:20, danakj wrote:
> > > What if you set old_layer_bounds to empty here? Would that allow better
> reuse
> > of
> > > the code below instead of early out?
> > 
> > Hmm.  I'm not sure the single line of reuse makes it worth the extra
> > complication.
> 
> For instance, if you did it that way, you'd also get an Invalidate over the
> whole layer. Why do you want to skip that here?

You don't need it; the clear() call takes that of that for you.

Calling clear is much simpler than trying to handle the next loop (comparing
content bounds) with different old and new tile sizes.

[danakj, 2013-02-12 01:57:58]

On Mon, Feb 11, 2013 at 5:43 PM, <enne@chromium.org> wrote:

> On 2013/02/12 01:36:24, danakj wrote:
>
>> https://codereview.chromium.**org/12221129/diff/1/cc/**
>>
picture_layer_tiling.cc<https://codereview.chromium.org/12221129/diff/1/cc/picture_layer_tiling.cc>
>> File cc/picture_layer_tiling.cc (right):
>>
>
>
> https://codereview.chromium.**org/12221129/diff/1/cc/**
>
picture_layer_tiling.cc#**newcode95<https://codereview.chromium.org/12221129/diff/1/cc/picture_layer_tiling.cc#newcode95>
>
>> cc/picture_layer_tiling.cc:95:
>> CreateTilesFromLayerRect(gfx::**Rect(layer_bounds_));
>> On 2013/02/12 01:34:39, enne wrote:
>> > On 2013/02/12 01:31:20, danakj wrote:
>> > > What if you set old_layer_bounds to empty here? Would that allow
>> better
>> reuse
>> > of
>> > > the code below instead of early out?
>> >
>> > Hmm.  I'm not sure the single line of reuse makes it worth the extra
>> > complication.
>>
>
>  For instance, if you did it that way, you'd also get an Invalidate over
>> the
>> whole layer. Why do you want to skip that here?
>>
>
> You don't need it; the clear() call takes that of that for you.
>
> Calling clear is much simpler than trying to handle the next loop
> (comparing
> content bounds) with different old and new tile sizes.
>

Ok, it doesn't seem so likely that this will facilitate future bugs due to
skipping the normal code path. I think that layer_invalidation in
Invalidate() is not the droid I thought it was. LGTM

[commit-bot: I haz the power, 2013-02-12 02:01:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/12221129/1

[commit-bot: I haz the power, 2013-02-12 12:51:34]

Change committed as 181911