cc: Stop pushing properties every activation for picture layers.

cc: Stop pushing properties every activation for picture layers.

Since we don't swap tilings anymore, we only need to push properties
when a property, or a tiling, or the raster source (invalidation) has
changed.

R=vmpstr
BUG=303943
Committed: https://crrev.com/dcaa923e704847eb6ba7003602ff70579aaaf47e
Cr-Commit-Position: refs/heads/master@{#313329}

[enne (OOO), 2015-01-23 20:56:06]

How is this tested?

[vmpstr, 2015-01-23 20:57:58]

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

https://codereview.chromium.org/874613003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:574: SetNeedsPushProperties();
Should this be invoked using PLTS UpdateTilingsToCurrentRasterSource?

https://codereview.chromium.org/874613003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:603: SetNeedsPushProperties();
Should this be invoked via PLTS constructor? Or is this push due to raster scale
being reset? If so, can you update the comment?

https://codereview.chromium.org/874613003/diff/1/cc/resources/picture_layer_t...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/874613003/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.h:37: // tiling was added or removed).
From the patch you have, this doesn't include any type of priority/rect changes,
should it?

[danakj, 2015-01-23 21:08:59]

On Fri, Jan 23, 2015 at 12:56 PM, <enne@chromium.org> wrote:

> How is this tested?
>


Ya... uh. I was thinking about what would be good tests here, but I'm not
even sure. I could perhaps have some "a new frame with invalidation gets to
the active tree"? Though that seems generally already tested.  All the
cases where the pending tree adds/removes a layer from the tree, I could do
some of those. Hm..

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

[danakj, 2015-01-23 21:11:36]

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

https://codereview.chromium.org/874613003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:574: SetNeedsPushProperties();
On 2015/01/23 20:57:57, vmpstr wrote:
> Should this be invoked using PLTS UpdateTilingsToCurrentRasterSource?

Only if it added a new tiling to the set.

https://codereview.chromium.org/874613003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:603: SetNeedsPushProperties();
On 2015/01/23 20:57:58, vmpstr wrote:
> Should this be invoked via PLTS constructor? Or is this push due to raster
scale
> being reset? If so, can you update the comment?

The constructor doesn't add any tilings, and the destructor doesn't call the
client. I could make one of those happen. The push is cuz of the tilings_ being
recreated/assigned.

https://codereview.chromium.org/874613003/diff/1/cc/resources/picture_layer_t...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/874613003/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.h:37: // tiling was added or removed).
On 2015/01/23 20:57:58, vmpstr wrote:
> From the patch you have, this doesn't include any type of priority/rect
changes,
> should it?

I don't think so.. live rect changes would come with a new rastersource if
they're changes from the pending tree.

The pending tree only has a single tiling, so it doesn't just change tiling
priority, it would remove/add a tiling.

[danakj, 2015-01-23 22:18:12]

Added a couple tests PTAL

[enne (OOO), 2015-01-23 22:25:28]

lgtm % vmpstr

[vmpstr, 2015-01-23 23:13:10]

lgtm

[danakj, 2015-01-23 23:14:08]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-01-23 23:15:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/874613003/20001

[commit-bot: I haz the power, 2015-01-24 00:08:46]

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

[commit-bot: I haz the power, 2015-01-24 00:08:47]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[danakj, 2015-01-24 00:17:17]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-01-24 00:17:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/874613003/20001

[commit-bot: I haz the power, 2015-01-24 00:18:34]

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

[commit-bot: I haz the power, 2015-01-24 00:18:35]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[danakj, 2015-01-24 00:19:48]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-01-24 00:20:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/874613003/20001

[commit-bot: I haz the power, 2015-01-24 02:02:04]

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

[commit-bot: I haz the power, 2015-01-24 02:02:05]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[danakj, 2015-01-27 17:42:18]

danakj@chromium.org changed reviewers:
+ enne@chromium.org

[danakj, 2015-01-27 17:42:19]

PTAL.

This fixes scrolling, it's also a much smaller patch now (+11 -45 loc from the
previous version).

This changes when we SNPP(). Instead of doing it when we add/remove a tiling
from the pending tree, we do it when we change the live tiles rect on a tiling
on the pending tree. Some reasoning:

- Removing a tiling on the pending tree doesn't matter for pushing. PushProps
doesn't cause the active tree to remove anything anyways.
- Adding a tiling on the pending tree is redundant for SNPP(), because
UpdateTiles does:
(1) Add tiling
(2) UpdateTilePriorities - sets the live tiles rect
So doing SNPP() on case (2) is enough.

- SNPP on SetLiveTilesRect also handles scrolling on the impl thread which
changes the live rect on the pending tree, if there are new tiles we need to
push to clone those tiles to the active tree. Otherwise we then SetLiveTilesRect
on the active tree too and try to add a new tile to the recycle tree, but it's
already there.

[vmpstr, 2015-01-27 19:11:31]

On 2015/01/27 17:42:19, danakj wrote:
> PTAL.
> 
> This fixes scrolling, it's also a much smaller patch now (+11 -45 loc from the
> previous version).
> 
> This changes when we SNPP(). Instead of doing it when we add/remove a tiling
> from the pending tree, we do it when we change the live tiles rect on a tiling
> on the pending tree. Some reasoning:
> 
> - Removing a tiling on the pending tree doesn't matter for pushing. PushProps
> doesn't cause the active tree to remove anything anyways.
> - Adding a tiling on the pending tree is redundant for SNPP(), because
> UpdateTiles does:
> (1) Add tiling
> (2) UpdateTilePriorities - sets the live tiles rect
> So doing SNPP() on case (2) is enough.
> 
> - SNPP on SetLiveTilesRect also handles scrolling on the impl thread which
> changes the live rect on the pending tree, if there are new tiles we need to
> push to clone those tiles to the active tree. Otherwise we then
SetLiveTilesRect
> on the active tree too and try to add a new tile to the recycle tree, but it's
> already there.

lgtm

[enne (OOO), 2015-01-27 19:15:15]

lgtm

I wasn't sure about adding or removing tilings not doing any notifying, but
adding a tiling and not adding new tiles seems like it shouldn't happen in
practice and wouldn't matter anyway (empty tiling might as well not exist).

[enne (OOO), 2015-01-27 19:15:23]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2015-01-27 19:16:42]

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

[vmpstr, 2015-01-27 19:17:16]

On 2015/01/27 19:15:15, enne_OOO_until_jan_26 wrote:
> lgtm
> 
> I wasn't sure about adding or removing tilings not doing any notifying, but
> adding a tiling and not adding new tiles seems like it shouldn't happen in
> practice and wouldn't matter anyway (empty tiling might as well not exist).

I was going to suggest "put a comment explaining why we don't need push
properties when we add a tiling" or something like that but I couldn't find a
good spot where that comment makes sense :\

[commit-bot: I haz the power, 2015-01-27 19:54:32]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-27 19:55:57]

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

[danakj, 2015-01-30 17:33:13]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/890963002/ by danakj@chromium.org.

The reason for reverting is: flaky tests
https://code.google.com/p/chromium/issues/detail?id=452965.