cc: Use Element Id to Record Animation Changes

cc: Use Element Id to Record Animation Changes

In LayerTreeHost and LayerTreeImpl, change key to the map that is used
to record animation changes from layer id to element id.

Also add set elements id for testings before building property trees for
testing so that the element id to node maps are always populated in
property trees.

BUG=702774
R=ajuma
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2758343002
Cr-Commit-Position: refs/heads/master@{#459530}
Committed: https://chromium.googlesource.com/chromium/src/+/1f4b78a69bd2994e484c7e5e7dc5743bead8a721

[weiliangc, 2017-03-20 17:23:20]

Description was changed from

==========
cc: Use Element Id to Record Animation Changes

In LayerTreeHost and LayerTreeImpl, change key to the map that is used
to record animation changes from layer id to element id.

Also add set elements id for testings before building property trees for
testing so that the element id to node maps are always populated in
property trees.

BUG=702774
R=ajuma
==========

to

==========
cc: Use Element Id to Record Animation Changes

In LayerTreeHost and LayerTreeImpl, change key to the map that is used
to record animation changes from layer id to element id.

Also add set elements id for testings before building property trees for
testing so that the element id to node maps are always populated in
property trees.

BUG=702774
R=ajuma
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[ajuma, 2017-03-20 21:40:38]

https://codereview.chromium.org/2758343002/diff/1/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2758343002/diff/1/cc/trees/layer_tree_impl.cc...
cc/trees/layer_tree_impl.cc:627: if (LayerImpl* layer = LayerById(id))
This logic is a bit roundabout in that (looking at the call stack that gets
here) we start with an element id, that gets converted to a node id, then we get
a layer id at the node, and then here we use that to find a layer to find the
element id again :)

Will this be fixed as part of removing the LayerTreeImpl* argument to
EffectTree::OnOpacityAnimated?

https://codereview.chromium.org/2758343002/diff/1/cc/trees/layer_tree_impl.cc...
cc/trees/layer_tree_impl.cc:728: for (; element_id_to_opacity !=
opacity_animations_map_.end();) {
Since this just has a condition now, it can be turned into a while loop. Same
comment for the loops below.

https://codereview.chromium.org/2758343002/diff/1/cc/trees/layer_tree_impl.cc...
cc/trees/layer_tree_impl.cc:734:
opacity_animations_map_.erase(element_id_to_opacity++);
Is the iterator guaranteed to still be valid after this?

[wkorman, 2017-03-20 21:59:56]

wkorman@chromium.org changed reviewers:
+ wkorman@chromium.org

[wkorman, 2017-03-20 21:59:57]

https://codereview.chromium.org/2758343002/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/2758343002/diff/1/cc/trees/layer_tree_host.cc...
cc/trees/layer_tree_host.cc:1350: state.potentially_animating[property];
Double checking, this case does not have the set_needs_update(true) we see in
OPACITY above. Are filters different in this respect?

https://codereview.chromium.org/2758343002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2758343002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:4127: for (int property =
TargetProperty::FIRST_TARGET_PROPERTY;
Much of this is near identical to the same in layer_tree_host.cc, can we share
code somehow? I see there are some differences for transform in particular (like
set_was_ever_ready..., and tree->set_needs_update_draw_properties). But bulk is
the same.

Even moving to three separate utility routines one for each case that live
somewhere affiliated with TargetProperty, PropertyTree, Animation, and we just
call them in each case here, then do impl-specific stuff remaining. Would make
maintaining easier.

We could create animation_property_utils.{h,cc} or somesuch. Just a thought.

https://codereview.chromium.org/2758343002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:4179: state.potentially_animating[property];
Same comment as in layer_tree_host.cc, no set_needs_update(true) here, is that
intended?

https://codereview.chromium.org/2758343002/diff/1/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/2758343002/diff/1/cc/trees/layer_tree_impl.h#...
cc/trees/layer_tree_impl.h:513: std::unordered_map<ElementId, float,
ElementIdHash> opacity_animations_map_;
Thoughts on naming these xx_to_yy? We did that for recently added
PropertyTrees::element_id_to_xxx_node_index for example. It can make the calling
code easier to follow since it's more obvious what we're mapping from/to.

So here would be something like element_id_to_opacity, element_id_to_transform,
element_id_to_filter_ops.

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

https://codereview.chromium.org/2758343002/diff/1/cc/trees/property_tree.h#ne...
cc/trees/property_tree.h:175: TransformNode* FindNodeFromElementId(ElementId
id);
Sometimes code guidelines prefer naming methods differently rather than relying
on varying return types. I'm actually not sure what Chromium style says here,
thought worth checkpoint for me to learn. Is this ok w/ the one below or should
be have Transform/Effect in the method name for more clarity?

I don't see anything about this explicitly in Chromium/Google C++ style docs.

[wkorman, 2017-03-20 22:01:03]

Forgot to add -- am not formal code reviewer here, comments were just while
reading in case helpful. :)

[weiliangc, 2017-03-21 15:18:52]

https://codereview.chromium.org/2758343002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2758343002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:4127: for (int property =
TargetProperty::FIRST_TARGET_PROPERTY;
On 2017/03/20 at 21:59:57, wkorman wrote:
> Much of this is near identical to the same in layer_tree_host.cc, can we share
code somehow? I see there are some differences for transform in particular (like
set_was_ever_ready..., and tree->set_needs_update_draw_properties). But bulk is
the same.
> 
> Even moving to three separate utility routines one for each case that live
somewhere affiliated with TargetProperty, PropertyTree, Animation, and we just
call them in each case here, then do impl-specific stuff remaining. Would make
maintaining easier.
> 
> We could create animation_property_utils.{h,cc} or somesuch. Just a thought.

We could create template function in utils class that is called here? Do you
think it makes sense to put in same CL or do another CL for that?

https://codereview.chromium.org/2758343002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:4179: state.potentially_animating[property];
On 2017/03/20 at 21:59:57, wkorman wrote:
> Same comment as in layer_tree_host.cc, no set_needs_update(true) here, is that
intended?

It is because for opacity animation and transform animation the subtree could
care about whether they are inside the animation, but for filter animation the
change is only limited to the node. Should I add comment?

https://codereview.chromium.org/2758343002/diff/1/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2758343002/diff/1/cc/trees/layer_tree_impl.cc...
cc/trees/layer_tree_impl.cc:627: if (LayerImpl* layer = LayerById(id))
On 2017/03/20 at 21:40:38, ajuma wrote:
> This logic is a bit roundabout in that (looking at the call stack that gets
here) we start with an element id, that gets converted to a node id, then we get
a layer id at the node, and then here we use that to find a layer to find the
element id again :)
> 
> Will this be fixed as part of removing the LayerTreeImpl* argument to
EffectTree::OnOpacityAnimated?

Yeah.

https://codereview.chromium.org/2758343002/diff/1/cc/trees/layer_tree_impl.cc...
cc/trees/layer_tree_impl.cc:734:
opacity_animations_map_.erase(element_id_to_opacity++);
On 2017/03/20 at 21:40:38, ajuma wrote:
> Is the iterator guaranteed to still be valid after this?

Yes. It's post increment, so the current iterator is passed into erase, and the
iterator is incremented.

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

https://codereview.chromium.org/2758343002/diff/1/cc/trees/property_tree.h#ne...
cc/trees/property_tree.h:175: TransformNode* FindNodeFromElementId(ElementId
id);
On 2017/03/20 at 21:59:57, wkorman wrote:
> Sometimes code guidelines prefer naming methods differently rather than
relying on varying return types. I'm actually not sure what Chromium style says
here, thought worth checkpoint for me to learn. Is this ok w/ the one below or
should be have Transform/Effect in the method name for more clarity?
> 
> I don't see anything about this explicitly in Chromium/Google C++ style docs.

Since this is a member function on transform/effect tree, I omitted what node
this function is referring to. I could make FindNodeFromElementId a pure virtual
function on property tree and only override it for transform and effect tree?

[wkorman, 2017-03-21 19:44:23]

https://codereview.chromium.org/2758343002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2758343002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:4127: for (int property =
TargetProperty::FIRST_TARGET_PROPERTY;
On 2017/03/21 15:18:52, weiliangc wrote:
> On 2017/03/20 at 21:59:57, wkorman wrote:
> > Much of this is near identical to the same in layer_tree_host.cc, can we
share
> code somehow? I see there are some differences for transform in particular
(like
> set_was_ever_ready..., and tree->set_needs_update_draw_properties). But bulk
is
> the same.
> > 
> > Even moving to three separate utility routines one for each case that live
> somewhere affiliated with TargetProperty, PropertyTree, Animation, and we just
> call them in each case here, then do impl-specific stuff remaining. Would make
> maintaining easier.
> > 
> > We could create animation_property_utils.{h,cc} or somesuch. Just a thought.
> 
> We could create template function in utils class that is called here? Do you
> think it makes sense to put in same CL or do another CL for that?

Either ok IMO, could add a comment here and in other file to note the code
duplication and intent to move to be shared.

ajuma@ may have better idea on how to share the code, I also thought perhaps
layer_tree_host_common but that looks very reductionist/limited. But maybe
that's what it's for?

https://codereview.chromium.org/2758343002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:4179: state.potentially_animating[property];
On 2017/03/21 15:18:52, weiliangc wrote:
> On 2017/03/20 at 21:59:57, wkorman wrote:
> > Same comment as in layer_tree_host.cc, no set_needs_update(true) here, is
that
> intended?
> 
> It is because for opacity animation and transform animation the subtree could
> care about whether they are inside the animation, but for filter animation the
> change is only limited to the node. Should I add comment?

Yes, I think worth adding a comment since I wondered.

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

https://codereview.chromium.org/2758343002/diff/1/cc/trees/property_tree.h#ne...
cc/trees/property_tree.h:175: TransformNode* FindNodeFromElementId(ElementId
id);
On 2017/03/21 15:18:52, weiliangc wrote:
> On 2017/03/20 at 21:59:57, wkorman wrote:
> > Sometimes code guidelines prefer naming methods differently rather than
> relying on varying return types. I'm actually not sure what Chromium style
says
> here, thought worth checkpoint for me to learn. Is this ok w/ the one below or
> should be have Transform/Effect in the method name for more clarity?
> > 
> > I don't see anything about this explicitly in Chromium/Google C++ style
docs.
> 
> Since this is a member function on transform/effect tree, I omitted what node
> this function is referring to. I could make FindNodeFromElementId a pure
virtual
> function on property tree and only override it for transform and effect tree?

Ah, that's right, I think it's ok as is then.

[ajuma, 2017-03-21 19:53:28]

https://codereview.chromium.org/2758343002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2758343002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:4127: for (int property =
TargetProperty::FIRST_TARGET_PROPERTY;
On 2017/03/21 19:44:23, wkorman wrote:
> On 2017/03/21 15:18:52, weiliangc wrote:
> > On 2017/03/20 at 21:59:57, wkorman wrote:
> > > Much of this is near identical to the same in layer_tree_host.cc, can we
> share
> > code somehow? I see there are some differences for transform in particular
> (like
> > set_was_ever_ready..., and tree->set_needs_update_draw_properties). But bulk
> is
> > the same.
> > > 
> > > Even moving to three separate utility routines one for each case that live
> > somewhere affiliated with TargetProperty, PropertyTree, Animation, and we
just
> > call them in each case here, then do impl-specific stuff remaining. Would
make
> > maintaining easier.
> > > 
> > > We could create animation_property_utils.{h,cc} or somesuch. Just a
thought.
> > 
> > We could create template function in utils class that is called here? Do you
> > think it makes sense to put in same CL or do another CL for that?
> 
> Either ok IMO, could add a comment here and in other file to note the code
> duplication and intent to move to be shared.
> 
> ajuma@ may have better idea on how to share the code, I also thought perhaps
> layer_tree_host_common but that looks very reductionist/limited. But maybe
> that's what it's for?

Perhaps draw_property_utils or property_tree might be good places for this.
layer_tree_host_common was where the old CalcDrawProps used to live; recently
we've mostly been removing stuff from there rather than adding new things to it.

Doing this in another CL (and leaving a comment for now) sgtm.

[weiliangc, 2017-03-21 19:59:39]

The CQ bit was checked by weiliangc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 20:00:19]

Dry run: 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, 2017-03-21 20:04:39]

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

[commit-bot: I haz the power, 2017-03-21 20:04:45]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[weiliangc, 2017-03-21 21:02:15]

The CQ bit was checked by weiliangc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 21:03:13]

Dry run: 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, 2017-03-21 21:44:16]

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

[commit-bot: I haz the power, 2017-03-21 21:44:17]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[weiliangc, 2017-03-21 22:12:18]

The test failure seems to be trying to disable animation using element id, which
the layer for that element id doesn't exist.

[ajuma, 2017-03-21 22:59:35]

On 2017/03/21 22:12:18, weiliangc wrote:
> The test failure seems to be trying to disable animation using element id,
which
> the layer for that element id doesn't exist.

How about changing the DCHECKs in LayerTreeHost::ElementIsAnimatingChanged so
that they only check for needs_rebuild when potentially_animating or
currently_animating is true?

[weiliangc, 2017-03-22 17:27:00]

The CQ bit was checked by weiliangc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 17:27:27]

Dry run: 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, 2017-03-22 18:30:10]

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

[commit-bot: I haz the power, 2017-03-22 18:30:11]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[weiliangc, 2017-03-22 19:08:33]

PTAL

[ajuma, 2017-03-22 19:28:22]

Looks good aside from one test failure to sort out:

https://codereview.chromium.org/2758343002/diff/100001/cc/trees/layer_tree_im...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2758343002/diff/100001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:1190: SetElementIdsForTesting();
It looks like this is conflicting with
CompositorMutableStateTest.MutableStateMutableProperties that wants to set its
own element ids but also needs to build properties for testing. Perhaps
SetElementIdForTesting can be changed to only set element ids on layers that
don't already have one.

[weiliangc, 2017-03-24 14:48:58]

The CQ bit was checked by weiliangc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 14:49:27]

Dry run: 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, 2017-03-24 17:13:15]

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

[commit-bot: I haz the power, 2017-03-24 17:13:16]

Dry run: This issue passed the CQ dry run.

[weiliangc, 2017-03-24 18:35:22]

Passing tests now :D
PTAL

[ajuma, 2017-03-24 20:03:16]

On 2017/03/24 18:35:22, weiliangc wrote:
> Passing tests now :D
> PTAL

Thanks! lgtm

[weiliangc, 2017-03-24 20:10:25]

The CQ bit was checked by weiliangc@chromium.org

[commit-bot: I haz the power, 2017-03-24 20:11:01]

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, 2017-03-24 20:16:47]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490386225983770,
"parent_rev": "14d16f0ea00e7716032e6452ee8d04d3a54d9e59", "commit_rev":
"1f4b78a69bd2994e484c7e5e7dc5743bead8a721"}

[commit-bot: I haz the power, 2017-03-24 20:17:38]

Description was changed from

==========
cc: Use Element Id to Record Animation Changes

In LayerTreeHost and LayerTreeImpl, change key to the map that is used
to record animation changes from layer id to element id.

Also add set elements id for testings before building property trees for
testing so that the element id to node maps are always populated in
property trees.

BUG=702774
R=ajuma
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Use Element Id to Record Animation Changes

In LayerTreeHost and LayerTreeImpl, change key to the map that is used
to record animation changes from layer id to element id.

Also add set elements id for testings before building property trees for
testing so that the element id to node maps are always populated in
property trees.

BUG=702774
R=ajuma
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2758343002
Cr-Commit-Position: refs/heads/master@{#459530}
Committed:
https://chromium.googlesource.com/chromium/src/+/1f4b78a69bd2994e484c7e5e7dc5...
==========

[commit-bot: I haz the power, 2017-03-24 20:17:39]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/1f4b78a69bd2994e484c7e5e7dc5...