The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1f4b78a69bd2994e484c7e5e7dc5743bead8a721

commit 1f4b78a69bd2994e484c7e5e7dc5743bead8a721
Author: weiliangc <weiliangc@chromium.org>
Date: Fri Mar 24 20:16:42 2017

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}

[modify] https://crrev.com/1f4b78a69bd2994e484c7e5e7dc5743bead8a721/cc/layers/layer.cc
[modify] https://crrev.com/1f4b78a69bd2994e484c7e5e7dc5743bead8a721/cc/layers/layer.h
[modify] https://crrev.com/1f4b78a69bd2994e484c7e5e7dc5743bead8a721/cc/layers/layer_impl.cc
[modify] https://crrev.com/1f4b78a69bd2994e484c7e5e7dc5743bead8a721/cc/layers/layer_impl.h
[modify] https://crrev.com/1f4b78a69bd2994e484c7e5e7dc5743bead8a721/cc/test/layer_tree_test.cc
[modify] https://crrev.com/1f4b78a69bd2994e484c7e5e7dc5743bead8a721/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/1f4b78a69bd2994e484c7e5e7dc5743bead8a721/cc/trees/layer_tree_host_common_unittest.cc
[modify] https://crrev.com/1f4b78a69bd2994e484c7e5e7dc5743bead8a721/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/1f4b78a69bd2994e484c7e5e7dc5743bead8a721/cc/trees/layer_tree_host_unittest.cc
[modify] https://crrev.com/1f4b78a69bd2994e484c7e5e7dc5743bead8a721/cc/trees/layer_tree_impl.cc
[modify] https://crrev.com/1f4b78a69bd2994e484c7e5e7dc5743bead8a721/cc/trees/layer_tree_impl.h
[modify] https://crrev.com/1f4b78a69bd2994e484c7e5e7dc5743bead8a721/cc/trees/property_tree.cc
[modify] https://crrev.com/1f4b78a69bd2994e484c7e5e7dc5743bead8a721/cc/trees/property_tree.h

Does this bug include work to revise:

EffectTree::OnOpacityAnimated
EffectTree::OnFilterAnimated
TransformTree::OnTransformAnimated

to avoid the layer id -> element id lookup in LayerTreeImpl::AddTo{Transform,Filter,Opacity}AnimationsMap?

I ask because that change seems like it would follow naturally if we had element id on each of the nodes, which sounds like this bullet from bug description:

- Store element id on transform and effect tree node.

like, we could just pass the node or the node's element id in directly to the AddToXxxAnimationsMap functions.

I note that http://crrev.com/2758343002 adds Find methods to the trees, but does not add an element id field to Transform/Effect nodes. Is that forthcoming/planned? I may be missing some add'l complexity. Thought useful to generally seek insight.

I'm now reading further in http://crrev.com/2762123004 which looks like it at least partly addresses the above comment, but leaves the Opacity layer id lookup.

Two questions:
- it looks like we haven't yet needed to add element_id to the effect/transform nodes themselves. Do we expect we'll need to do so?
- what is special about opacity animations -- the separate bug/work noted in the change around overlay scrollbars?

To answer the questions:
1. There is no plan to add element_id to effect/transform nodes themselves.
Reasons are: 
- Effect/transform node would have default element id when they are not going to animated, which still takes space on the node themselves.
- Currently we only try to use element id to tick animation on property tree nodes, so the map from element id to property tree nodes would suffice.
- Element id to effect/transform node should be 1:1, and if in rare occasion we would need to find corresponding element id to from property tree node we should be able to do so by traversing the map.


2. About overlay scrollbars:
- They are special because the fading of overlay scrollbars are opacity animation happening outside of the normal animation system. The ScrollbarAnimationController class posts delayed tasks to do this.
- To remove the access from layer id in the opacity code in scrollbars, the overlay scrollbars would need element ids, and those element ids would need to be put into element id to property tree node ids map.
- Side note: scrollbar layers always have an effect node by overriding https://cs.chromium.org/chromium/src/cc/layers/layer.h?type=cs&q=Opacitycananimateonimplthread+package:%5Echromium$&l=128. This behavior probably needs to be duplicated for SPv2.
- Some future work would be making scrollbar opacity animation not a special case any more, but that requires more design and work that probably shouldn't block element id in property trees work.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ea09f3374e3fc4f7669390b1203b1354c3a9cd0e

commit ea09f3374e3fc4f7669390b1203b1354c3a9cd0e
Author: weiliangc <weiliangc@chromium.org>
Date: Wed Mar 29 16:43:24 2017

cc: LayerTreeHostImpl uses element id to tick animations

Remove converting to layer id when animation is ticked with element id
on LayerTreeHostImpl. Also avoids passing in LayerTreeImpl pointer to
property trees.

Opacity animation on scrollbar still uses layer id, this is due to
scrollbars aren't guaranteed with element id. Work is needed in
 crbug.com/702832  to address this.

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

Review-Url: https://codereview.chromium.org/2762123004
Cr-Commit-Position: refs/heads/master@{#460423}

[modify] https://crrev.com/ea09f3374e3fc4f7669390b1203b1354c3a9cd0e/cc/input/scrollbar_animation_controller.cc
[modify] https://crrev.com/ea09f3374e3fc4f7669390b1203b1354c3a9cd0e/cc/input/scrollbar_animation_controller_unittest.cc
[modify] https://crrev.com/ea09f3374e3fc4f7669390b1203b1354c3a9cd0e/cc/layers/layer_impl_unittest.cc
[modify] https://crrev.com/ea09f3374e3fc4f7669390b1203b1354c3a9cd0e/cc/layers/render_surface_unittest.cc
[modify] https://crrev.com/ea09f3374e3fc4f7669390b1203b1354c3a9cd0e/cc/layers/scrollbar_layer_impl_base.cc
[modify] https://crrev.com/ea09f3374e3fc4f7669390b1203b1354c3a9cd0e/cc/layers/scrollbar_layer_impl_base.h
[modify] https://crrev.com/ea09f3374e3fc4f7669390b1203b1354c3a9cd0e/cc/layers/scrollbar_layer_unittest.cc
[modify] https://crrev.com/ea09f3374e3fc4f7669390b1203b1354c3a9cd0e/cc/tiles/tile_manager_unittest.cc
[modify] https://crrev.com/ea09f3374e3fc4f7669390b1203b1354c3a9cd0e/cc/trees/damage_tracker_unittest.cc
[modify] https://crrev.com/ea09f3374e3fc4f7669390b1203b1354c3a9cd0e/cc/trees/layer_tree_host_common_unittest.cc
[modify] https://crrev.com/ea09f3374e3fc4f7669390b1203b1354c3a9cd0e/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/ea09f3374e3fc4f7669390b1203b1354c3a9cd0e/cc/trees/layer_tree_host_impl.h
[modify] https://crrev.com/ea09f3374e3fc4f7669390b1203b1354c3a9cd0e/cc/trees/layer_tree_host_unittest.cc
[modify] https://crrev.com/ea09f3374e3fc4f7669390b1203b1354c3a9cd0e/cc/trees/layer_tree_host_unittest_picture.cc
[modify] https://crrev.com/ea09f3374e3fc4f7669390b1203b1354c3a9cd0e/cc/trees/layer_tree_impl.cc
[modify] https://crrev.com/ea09f3374e3fc4f7669390b1203b1354c3a9cd0e/cc/trees/layer_tree_impl.h
[modify] https://crrev.com/ea09f3374e3fc4f7669390b1203b1354c3a9cd0e/cc/trees/property_tree.cc
[modify] https://crrev.com/ea09f3374e3fc4f7669390b1203b1354c3a9cd0e/cc/trees/property_tree.h
[modify] https://crrev.com/ea09f3374e3fc4f7669390b1203b1354c3a9cd0e/third_party/WebKit/Source/platform/graphics/CompositorMutableState.cpp

Possibly causes this: https://bugs.chromium.org/p/chromium/issues/detail?id=707090
according to bisect?

Yes, 707090 does look likely to be a duplicate of this with another test case which could be helpful for investigation, thanks for filing.

What I meant is that the commit in Comment 6 above, causes  issue 707090 , as per https://bugs.chromium.org/p/chromium/issues/detail?id=707090#c9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9975ad63f85f1fe76100602db98800b9232aa454

commit 9975ad63f85f1fe76100602db98800b9232aa454
Author: weiliangc <weiliangc@chromium.org>
Date: Mon Apr 03 15:24:42 2017

cc: Avoid Crash in Effect Tree Animation by Element ID

Instead of crash, early out of function trying to animate on effect
tree by element id.

The root cause of this crash is we are trying to animate a node that
does not have element id set up in map yet. Before the bug was covered
by operator[] used for access map, and that would ends up being a noop.

R=wkorman
BUG=706766,  707090 ,  702774 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2794673002
Cr-Commit-Position: refs/heads/master@{#461432}

[modify] https://crrev.com/9975ad63f85f1fe76100602db98800b9232aa454/cc/trees/property_tree.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9a3acfa5090cb502ba1af13609446a6b9a052dad

commit 9a3acfa5090cb502ba1af13609446a6b9a052dad
Author: Weiliang Chen <weiliangc@chromium.org>
Date: Mon Apr 03 20:18:28 2017

cc: Avoid Crash in Effect Tree Animation by Element ID

Instead of crash, early out of function trying to animate on effect
tree by element id.

The root cause of this crash is we are trying to animate a node that
does not have element id set up in map yet. Before the bug was covered
by operator[] used for access map, and that would ends up being a noop.

R=wkorman
BUG=706766,  707090 ,  702774 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2794673002
Cr-Commit-Position: refs/heads/master@{#461432}
(cherry picked from commit 9975ad63f85f1fe76100602db98800b9232aa454)

Review-Url: https://codereview.chromium.org/2792143002 .
Cr-Commit-Position: refs/branch-heads/3061@{#3}
Cr-Branched-From: 9e03960f75b3019372157db5c4fe5264dfc36616-refs/heads/master@{#461353}

[modify] https://crrev.com/9a3acfa5090cb502ba1af13609446a6b9a052dad/cc/trees/property_tree.cc

So the reason for crash is AnimationHost and PropertyTreeBuilder disagree on when the animation is complete (no longer needs ticking) and that AnimationHost doesn't think finished animation needs push properties. I have a fix for this which will undo my crash suppression in previous patch.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b1e280d1a61f77855418913417c941c7faff176c

commit b1e280d1a61f77855418913417c941c7faff176c
Author: weiliangc <weiliangc@chromium.org>
Date: Tue Apr 25 16:19:41 2017

cc: Push Animation Finished State and Use Finished State for IsCompleted

This fixes the bug where we try to tick animation on pending tree after
commit using element id, but property trees doesn't think animation is
still running.

There are four parts that are changed by this CL:
1. Property tree check for animation is finished to determine that we
no longer need a node for animation, while animation host only think
animation is completed when animation is deleted. Change animation
host to considered finished animation as completed.

2. When animation is finished, the state is not pushed during commit.
In this case animation host on impl thread would still try to tick this
animation because it does not know animation is finished. Add call to
SetNeedsPushProperties to animation when it is finished.

3. It used to be that when animation no longer affects either pending
and active tree, that animation is removed during activation. After this
CL, animation marked as finished on main thread will get to same stage,
but sending out FINISHED event is still needed. To do this, don't
delete animations during activation, instead mark animations not
affecting trees as finished during UpdateState.

4. For animations removed on main thread, the compositor thread cannot
distinguish it from a FINISHED animation on main thread. When main
thread receives FINISHED event from compositor thread and found that
the animation no longer exists, it is safe to delete the animation on
the compositor thread during commit.

Since this is the correct fix for crashing cause by trying to use
element id to tick animation, the early return to avoid crash is also
removed in this CL.

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

Review-Url: https://codereview.chromium.org/2796013003
Cr-Commit-Position: refs/heads/master@{#467008}

[modify] https://crrev.com/b1e280d1a61f77855418913417c941c7faff176c/cc/animation/animation_player.cc
[modify] https://crrev.com/b1e280d1a61f77855418913417c941c7faff176c/cc/animation/animation_player_unittest.cc
[modify] https://crrev.com/b1e280d1a61f77855418913417c941c7faff176c/cc/animation/element_animations_unittest.cc
[modify] https://crrev.com/b1e280d1a61f77855418913417c941c7faff176c/cc/trees/layer_tree_host_unittest_animation.cc
[modify] https://crrev.com/b1e280d1a61f77855418913417c941c7faff176c/cc/trees/property_tree.cc

Wei are you still working on this, and what remains to be done if so?

Talked with Wei on chat. Will file new if more found to be done.