cc: Compute animation scale on demand

cc:: Compute animation scale on demand

As we are cleaning up transform tree logic, we need to remove animation
scale computation and data from transform tree. And instead, we compute
it when we need it.

However, computing animation scale may reuse animation scale data which
belongs to other transform nodes, so we need a PropertyTreesCachedData
to store this information, and invalidate it every time we update the
transform tree.

BUG=623564
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/f468675e51ff897ceee05906de62d19b62b238e9
Cr-Commit-Position: refs/heads/master@{#403354}

[sunxd, 2016-06-28 16:04:20]

Description was changed from

==========
cc:: Compute animation scale on demand

As we are cleaning up transform tree logic, we need to remove animation
scale computation and data from transform tree. And instead, we compute
it when we need it.

However, computing animation scale may reuse animation scale data which
belongs to other transform nodes, so we need a PropertyTreesCachedData
to store this information, and invalidate it every time we update the
transform tree.

BUG=623564
==========

to

==========
cc:: Compute animation scale on demand

As we are cleaning up transform tree logic, we need to remove animation
scale computation and data from transform tree. And instead, we compute
it when we need it.

However, computing animation scale may reuse animation scale data which
belongs to other transform nodes, so we need a PropertyTreesCachedData
to store this information, and invalidate it every time we update the
transform tree.

BUG=623564
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[sunxd, 2016-06-28 20:57:00]

Description was changed from

==========
cc:: Compute animation scale on demand

As we are cleaning up transform tree logic, we need to remove animation
scale computation and data from transform tree. And instead, we compute
it when we need it.

However, computing animation scale may reuse animation scale data which
belongs to other transform nodes, so we need a PropertyTreesCachedData
to store this information, and invalidate it every time we update the
transform tree.

BUG=623564
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc:: Compute animation scale on demand

As we are cleaning up transform tree logic, we need to remove animation
scale computation and data from transform tree. And instead, we compute
it when we need it.

However, computing animation scale may reuse animation scale data which
belongs to other transform nodes, so we need a PropertyTreesCachedData
to store this information, and invalidate it every time we update the
transform tree.

BUG=623564
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[sunxd, 2016-06-28 20:57:01]

[email protected] changed reviewers:
+ [email protected], [email protected], [email protected]

[sunxd, 2016-06-28 20:59:05]

I'm still thinking about adding unit tests for the cached data structure. My
current idea is to add a tree synchronizer test to check if the cached data is
refreshed when pushing trees from main to impl; and also add a property tree
unittest that checks if animation scale is correctly computed and reused.

[sunxd, 2016-06-28 21:32:06]

Patchset #6 (id:100001) has been deleted

[jaydasika, 2016-06-28 23:08:41]

Overall looks good. The tests you suggested also sound good.

https://codereview.chromium.org/2105673003/diff/120001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (left):

https://codereview.chromium.org/2105673003/diff/120001/cc/trees/property_tree...
cc/trees/property_tree.cc:1082: bool failed_at_ancestor =
Can we move comments to GetAnimationScales ?

https://codereview.chromium.org/2105673003/diff/120001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2105673003/diff/120001/cc/trees/property_tree...
cc/trees/property_tree.cc:718: property_trees()->UpdateCachedNumber();
Should be invalidate the cache as soon as we set_needs_update(true) or when we
actually do the update ?

https://codereview.chromium.org/2105673003/diff/120001/cc/trees/property_tree...
cc/trees/property_tree.cc:2434: 
Add cached_data_ to PropertyTrees::operator==

https://codereview.chromium.org/2105673003/diff/120001/cc/trees/property_tree.h
File cc/trees/property_tree.h (left):

https://codereview.chromium.org/2105673003/diff/120001/cc/trees/property_tree...
cc/trees/property_tree.h:158: // The maximum scale that that node's |local|
transform will have during
Can we retain the comments ? (move them inside AnimationScaleData, maybe)

https://codereview.chromium.org/2105673003/diff/120001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/2105673003/diff/120001/cc/trees/property_tree...
cc/trees/property_tree.h:785: int property_tree_update_number;
This is unused.

https://codereview.chromium.org/2105673003/diff/120001/cc/trees/property_tree...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2105673003/diff/120001/cc/trees/property_tree...
cc/trees/property_tree_builder.cc:1385: property_trees->ResetCachedData();
Should this also be in the Layer version of the function ? Or maybe, put it
inside BuildPropertyTreesTopLevelInternal so that all clearing code is in one
place ?

[ajuma, 2016-06-29 15:04:38]

https://codereview.chromium.org/2105673003/diff/160001/cc/trees/draw_property...
File cc/trees/draw_property_utils.cc (left):

https://codereview.chromium.org/2105673003/diff/160001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:1221:
layer->draw_properties().starting_animation_contents_scale = 0.f;
Please make sure the new logic also uses scale 0 when
layer_transforms_should_scale_layer_contents is false (and please add a test for
this if we don't already have one).

https://codereview.chromium.org/2105673003/diff/160001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2105673003/diff/160001/cc/trees/property_tree...
cc/trees/property_tree.cc:2343: node->data.to_screen_has_scale_animation = true;
If to_screen_has_scale_animation is only used for computing animation scale, I
think it should also be moved out of TransformNodeData and into the struct
that's caching animation scale stuff (and if it's used outside of computing
animation scale, then its value shouldn't be changed during the computation of
animation scale).

https://codereview.chromium.org/2105673003/diff/160001/cc/trees/property_tree...
cc/trees/property_tree.cc:2365: layer_tree_impl->MaximumTargetScale(
Please leave a TODO to make these calls take an id instead of a LayerImpl*.

https://codereview.chromium.org/2105673003/diff/160001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/2105673003/diff/160001/cc/trees/property_tree...
cc/trees/property_tree.h:739: int property_tree_update_number;
This needs to be initialized.

[sunxd, 2016-06-30 15:29:44]

https://codereview.chromium.org/2105673003/diff/120001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (left):

https://codereview.chromium.org/2105673003/diff/120001/cc/trees/property_tree...
cc/trees/property_tree.cc:1082: bool failed_at_ancestor =
On 2016/06/28 23:08:40, jaydasika wrote:
> Can we move comments to GetAnimationScales ?

Done.

https://codereview.chromium.org/2105673003/diff/120001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2105673003/diff/120001/cc/trees/property_tree...
cc/trees/property_tree.cc:718: property_trees()->UpdateCachedNumber();
On 2016/06/28 23:08:40, jaydasika wrote:
> Should be invalidate the cache as soon as we set_needs_update(true) or when we
> actually do the update ?

Hmm, that depends on whether we get animation scale after setting needs_update
and before updating transforms. I prefer doing this if we don't. As updating
cached number is equivalent to setting the flag.

https://codereview.chromium.org/2105673003/diff/120001/cc/trees/property_tree...
cc/trees/property_tree.cc:2434: 
On 2016/06/28 23:08:40, jaydasika wrote:
> Add cached_data_ to PropertyTrees::operator==

I think cached_data_ should be considered separately. It stores transient data
so that the way of accessing the cached data can affect the actual data stored.

Also for test, I think we can have specific test that compares the cached_data_
instead of compare them along with other member variables of property trees.

https://codereview.chromium.org/2105673003/diff/120001/cc/trees/property_tree.h
File cc/trees/property_tree.h (left):

https://codereview.chromium.org/2105673003/diff/120001/cc/trees/property_tree...
cc/trees/property_tree.h:158: // The maximum scale that that node's |local|
transform will have during
On 2016/06/28 23:08:40, jaydasika wrote:
> Can we retain the comments ? (move them inside AnimationScaleData, maybe)

Done.

https://codereview.chromium.org/2105673003/diff/120001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/2105673003/diff/120001/cc/trees/property_tree...
cc/trees/property_tree.h:785: int property_tree_update_number;
On 2016/06/28 23:08:40, jaydasika wrote:
> This is unused.

Done.

https://codereview.chromium.org/2105673003/diff/120001/cc/trees/property_tree...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2105673003/diff/120001/cc/trees/property_tree...
cc/trees/property_tree_builder.cc:1385: property_trees->ResetCachedData();
On 2016/06/28 23:08:40, jaydasika wrote:
> Should this also be in the Layer version of the function ? Or maybe, put it
> inside BuildPropertyTreesTopLevelInternal so that all clearing code is in one
> place ?

The only place where animation scales get used is in PictureLayerImpl in
production code, and ResetCachedData is called when pushing property trees from
main to impl. This line here is only for testing. Maybe when we decide to move
other stuff from property trees to the cached data structure, we need to reset
the cache on main thread, but I would still stick to this currently.

https://codereview.chromium.org/2105673003/diff/160001/cc/trees/draw_property...
File cc/trees/draw_property_utils.cc (left):

https://codereview.chromium.org/2105673003/diff/160001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:1221:
layer->draw_properties().starting_animation_contents_scale = 0.f;
On 2016/06/29 15:04:37, ajuma wrote:
> Please make sure the new logic also uses scale 0 when
> layer_transforms_should_scale_layer_contents is false (and please add a test
for
> this if we don't already have one).

Done.

https://codereview.chromium.org/2105673003/diff/160001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2105673003/diff/160001/cc/trees/property_tree...
cc/trees/property_tree.cc:2343: node->data.to_screen_has_scale_animation = true;
On 2016/06/29 15:04:37, ajuma wrote:
> If to_screen_has_scale_animation is only used for computing animation scale, I
> think it should also be moved out of TransformNodeData and into the struct
> that's caching animation scale stuff (and if it's used outside of computing
> animation scale, then its value shouldn't be changed during the computation of
> animation scale).

Done.

https://codereview.chromium.org/2105673003/diff/160001/cc/trees/property_tree...
cc/trees/property_tree.cc:2365: layer_tree_impl->MaximumTargetScale(
On 2016/06/29 15:04:37, ajuma wrote:
> Please leave a TODO to make these calls take an id instead of a LayerImpl*.

Done.

https://codereview.chromium.org/2105673003/diff/160001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/2105673003/diff/160001/cc/trees/property_tree...
cc/trees/property_tree.h:739: int property_tree_update_number;
On 2016/06/29 15:04:38, ajuma wrote:
> This needs to be initialized.

It is initialized in PropertyTreesCachedData's ctor and
PropertyTrees::ResetCachedData (called by PropertyTrees::operator=).

[sunxd, 2016-06-30 18:36:09]

I discussed with Ali about the some of the comments and made corresponding
changes.

[jaydasika, 2016-06-30 19:56:50]

lgtm % ajuma

https://codereview.chromium.org/2105673003/diff/240001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/2105673003/diff/240001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common_unittest.cc:8003: //
root->SetDrawsContent(true);
Remove commented code.

[ajuma, 2016-06-30 21:57:37]

lgtm too

https://codereview.chromium.org/2105673003/diff/240001/cc/trees/tree_synchron...
File cc/trees/tree_synchronizer_unittest.cc (right):

https://codereview.chromium.org/2105673003/diff/240001/cc/trees/tree_synchron...
cc/trees/tree_synchronizer_unittest.cc:688: // refreshed when pushing layer
trees
Style nit: Please write comments as sentences (capitalize the first letter, add
a period at the end). Same for the other comments you added.

[sunxd, 2016-06-30 22:59:12]

The CQ bit was checked by [email protected]

[sunxd, 2016-06-30 22:59:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
[email protected], [email protected]
Link to the patchset: https://codereview.chromium.org/2105673003/#ps260001
(title: "Nit changes")

[commit-bot: I haz the power, 2016-06-30 22:59:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 23:56:36]

Description was changed from

==========
cc:: Compute animation scale on demand

As we are cleaning up transform tree logic, we need to remove animation
scale computation and data from transform tree. And instead, we compute
it when we need it.

However, computing animation scale may reuse animation scale data which
belongs to other transform nodes, so we need a PropertyTreesCachedData
to store this information, and invalidate it every time we update the
transform tree.

BUG=623564
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc:: Compute animation scale on demand

As we are cleaning up transform tree logic, we need to remove animation
scale computation and data from transform tree. And instead, we compute
it when we need it.

However, computing animation scale may reuse animation scale data which
belongs to other transform nodes, so we need a PropertyTreesCachedData
to store this information, and invalidate it every time we update the
transform tree.

BUG=623564
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-06-30 23:56:40]

Committed patchset #13 (id:260001)

[commit-bot: I haz the power, 2016-06-30 23:57:12]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 23:59:04]

Description was changed from

==========
cc:: Compute animation scale on demand

As we are cleaning up transform tree logic, we need to remove animation
scale computation and data from transform tree. And instead, we compute
it when we need it.

However, computing animation scale may reuse animation scale data which
belongs to other transform nodes, so we need a PropertyTreesCachedData
to store this information, and invalidate it every time we update the
transform tree.

BUG=623564
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc:: Compute animation scale on demand

As we are cleaning up transform tree logic, we need to remove animation
scale computation and data from transform tree. And instead, we compute
it when we need it.

However, computing animation scale may reuse animation scale data which
belongs to other transform nodes, so we need a PropertyTreesCachedData
to store this information, and invalidate it every time we update the
transform tree.

BUG=623564
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/f468675e51ff897ceee05906de62d19b62b238e9
Cr-Commit-Position: refs/heads/master@{#403354}
==========

[commit-bot: I haz the power, 2016-06-30 23:59:05]

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