cc: Remove tiles from recycle tree that were deleted on active.

cc: Remove tiles from recycle tree that were deleted on active.

This patch removes tiles from the recycle tree that were deleted from
the active tree as a result of a shifting live tiles rect. It is
important to do this, since if the active tree then would recreate
the deleted tile (ie, live tiles rect shift back), we have to ensure
that this tile will be shared when the next pending tree is created.
If we don't do it, we can run into a situation in which we will
continuously raster the same tile.

The patch does the following:
- Adds a way to get a recycled tree twin tiling.
- During LiveTilesRect tile deletion, deletes tiles from the same
  location from the recycle twin (if one exists).

R=danakj, enne

Committed: https://crrev.com/7fceb77977afd9af22215eb9cd28ab667567668e
Cr-Commit-Position: refs/heads/master@{#291949}

[vmpstr, 2014-08-22 15:16:23]

Hi,

This is the approach I was thinking of taking. Please take a look. The thing
that is blatantly missing are the tests. I will add these if the approach seems
legit. 

Thanks.

[enne (OOO), 2014-08-22 19:56:55]

This seems legit to me.

[enne (OOO), 2014-08-22 21:36:09]

https://codereview.chromium.org/502453003/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/502453003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:620: for (size_t i = 0; i <
recycled_twin_layer_->tilings_->num_tilings(); ++i) {
I think you're looking for
recycled_twin_layer_->tilings_->TilingAtScale(tiling->contents_scale)?

[vmpstr, 2014-08-22 22:47:30]

Added some tests.

https://codereview.chromium.org/502453003/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/502453003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:620: for (size_t i = 0; i <
recycled_twin_layer_->tilings_->num_tilings(); ++i) {
On 2014/08/22 21:36:09, enne wrote:
> I think you're looking for
> recycled_twin_layer_->tilings_->TilingAtScale(tiling->contents_scale)?

Ah, that's easier.

[enne (OOO), 2014-08-22 23:01:05]

https://codereview.chromium.org/502453003/diff/40001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/502453003/diff/40001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling_unittest.cc:1951: TEST(PictureLayerTilingTest,
RecycledTilesCleared) {
For forgetful future ennes, could you leave a comment about what this test is
attempt to test?

[vmpstr, 2014-08-25 18:41:00]

PTAL

https://codereview.chromium.org/502453003/diff/40001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/502453003/diff/40001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling_unittest.cc:1951: TEST(PictureLayerTilingTest,
RecycledTilesCleared) {
On 2014/08/22 23:01:05, enne wrote:
> For forgetful future ennes, could you leave a comment about what this test is
> attempt to test?

Done.

[enne (OOO), 2014-08-25 20:00:29]

lgtm

[vmpstr, 2014-08-25 21:23:55]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2014-08-25 21:25:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vmpstr@chromium.org/502453003/60001

[vmpstr, 2014-08-25 21:37:52]

The CQ bit was unchecked by vmpstr@chromium.org

[vmpstr, 2014-08-26 00:05:22]

PTAL. I removed maintaining of the recycle tree here and added a todo. See
crbug.com/407418. Basically, maintaining recycle tree twin is a bit more
involved than maintaining a regular twin, since it can go away without notifying
the active tree (and leaving a dangling recycled twin pointer). There should be
a way to maintain it, but I would like to do that as a follow-up. This is mostly
because it would be a higher risk change that is a bit too risky to merge (and
this  patch will have to be merged).

[enne (OOO), 2014-08-26 16:59:59]

lgtm.  Maintaining pointers to the recycle tree seems like it could be dodgy.

PS Why is LayerTreeImpl::LayerById not const? This isn't your fault, but it
seems wrong.

[vmpstr, 2014-08-26 17:03:33]

On 2014/08/26 16:59:59, enne wrote:
> lgtm.  Maintaining pointers to the recycle tree seems like it could be dodgy.
> 
> PS Why is LayerTreeImpl::LayerById not const? This isn't your fault, but it
> seems wrong.

It returns a non const pointer, so I think by convention we don't want the
function to be const.

[vmpstr, 2014-08-26 17:03:41]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2014-08-26 17:04:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vmpstr@chromium.org/502453003/80001

[danakj, 2014-08-26 17:22:08]

On Tue, Aug 26, 2014 at 1:03 PM, <vmpstr@chromium.org> wrote:

> On 2014/08/26 16:59:59, enne wrote:
>
>> lgtm.  Maintaining pointers to the recycle tree seems like it could be
>> dodgy.
>>
>
>  PS Why is LayerTreeImpl::LayerById not const? This isn't your fault, but
>> it
>> seems wrong.
>>
>
> It returns a non const pointer, so I think by convention we don't want the
> function to be const.
>

Hm, I thought the rule is no const method returning non-const pointers. Or
I'm misremembering.

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

[vmpstr, 2014-08-26 18:01:40]

On 2014/08/26 17:22:08, danakj wrote:
> On Tue, Aug 26, 2014 at 1:03 PM, <mailto:vmpstr@chromium.org> wrote:
> 
> > On 2014/08/26 16:59:59, enne wrote:
> >
> >> lgtm.  Maintaining pointers to the recycle tree seems like it could be
> >> dodgy.
> >>
> >
> >  PS Why is LayerTreeImpl::LayerById not const? This isn't your fault, but
> >> it
> >> seems wrong.
> >>
> >
> > It returns a non const pointer, so I think by convention we don't want the
> > function to be const.
> >
> 
> Hm, I thought the rule is no const method returning non-const pointers. Or
> I'm misremembering.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Perhaps it's my own convention that this supports :) I'm not sure. Typically
getting a non-const pointer to member out of a const object is not too good? But
I have seen both approaches in cc

[danakj, 2014-08-26 18:04:57]

On Tue, Aug 26, 2014 at 1:21 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Tue, Aug 26, 2014 at 1:03 PM, <vmpstr@chromium.org> wrote:
>
>> On 2014/08/26 16:59:59, enne wrote:
>>
>>> lgtm.  Maintaining pointers to the recycle tree seems like it could be
>>> dodgy.
>>>
>>
>>  PS Why is LayerTreeImpl::LayerById not const? This isn't your fault, but
>>> it
>>> seems wrong.
>>>
>>
>> It returns a non const pointer, so I think by convention we don't want the
>> function to be const.
>>
>
> Hm, I thought the rule is no const method returning non-const pointers. Or
> I'm misremembering.
>

Oh, I remember right. But I just don't read right. You said a it returns
non-const, I read const. Wee.

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

[enne (OOO), 2014-08-26 18:11:55]

Ah, ok, quite right.  :)

[commit-bot: I haz the power, 2014-08-26 18:52:13]

Committed patchset #5 (80001) as b1733f3e4f8411dfa234908062538bc47010acbe

[commit-bot: I haz the power, 2014-09-10 02:44:22]

Patchset 5 (id:??) landed as
https://crrev.com/7fceb77977afd9af22215eb9cd28ab667567668e
Cr-Commit-Position: refs/heads/master@{#291949}