Restart any animations when we allocate a composited layer.

Restart any animations when we allocate a composited layer.

As part of this, fix FrameView to note a compositing update is needed
when it is shown/hidden since it could gain/lose its composited layer
and, if shown/gained, then we want to immediately restart any
animations.

BUG=612436
Committed: https://crrev.com/851bbadbc2348dcf79851c9be1b766e4ce00a2ab
Cr-Commit-Position: refs/heads/master@{#397003}

[wkorman, 2016-05-26 19:21:31]

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

[wkorman, 2016-05-26 19:21:31]

loyso@ seeking your guidance before I dig further -- this change is for
discussion purposes. Test is verbatim from the linked bug, not yet reduced.

Blindly restarting animations when we allocate a composited layer fixes things.
This taken from similar in Element::detach():

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

However, the animation doesn't actually start pushing frames until a mouse event
(press or move) after the checkbox is toggled and the layer is re-shown.

I'm not yet familiar with how the animation internals work. The condition that
animation code may have depended on, and that was changed by
http://crrev.com/1616183002, is that previously composited layers always stuck
around once created, even if their style was set to visibility: hidden. Now,
when hidden, we remove them, and recreate them when needed. I'm assuming
something with this change in logic has affected animator internal
preconditions.

chrishtr@ notes that this situation should be roughly analogous to a test case
where rather than toggling visibility: hidden, we toggle will-change: transform
on the iframe (or something similar), basically some style alteration that
shifts it from composited to non-composited and then back.

His thought was that the change in this patch doesn't seem like it should be
necessary. Rather, the call to DocumentAnimations::updateCompositorAnimations()
here:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

seems like it is intended to take care of these issues, so maybe something is
missing there.

[loyso (OOO), 2016-05-27 05:20:15]

Hi, Walter! Thanks for working on this.

I'm not well familiar with that high-level code where general blink Element and
Document call lower-level Web Animations stuff.
Just a general note from me is that compositor layers are not the only object to
care about. You also should care about CompositorAnimationPlayer and
CompositorAnimationTimeline. At least, attach/detach them properly if you
create/destroy cc::LayerTreeHost (cc::AnimationHost), which is WebLayerTreeView
basically.

However, it looks like that layer/timeline/player are connected properly if
animation stars. The dependency on a mouse event triggering is very very
strange.
Unfortunately, my knowledge is rather limited starting from this point.

Having a much simpler test case which doesn't involve iframes would be helpful.

[loyso (OOO), 2016-05-27 05:30:02]

A remark using some tips from dstockwell@:

ElementAnimations::restartAnimationOnCompositor doesn't restart animations
immediately.
It schedules a pending update instead (Animation::setCompositorPending).
A mouse event triggers that update, probably. 

So you guess that you need to call
DocumentAnimations::updateCompositorAnimations
looks absolutely right.

The updateCompositorAnimations calls document.timeline().scheduleNextService();
which should kick everything off.

[wkorman, 2016-05-28 00:56:56]

Description was changed from

==========
Restart animations when we allocate a composited layer.

BUG=612436
==========

to

==========
Restart any animations when we allocate a composited layer.

As part of this, fix FrameView to note a compositing update is needed
when it is shown/hidden since it could gain/lose its composited layer
and, if shown/gained, then we want to immediately restart any
animations.

BUG=612436
==========

[wkorman, 2016-05-28 00:59:43]

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

[wkorman, 2016-05-28 00:59:44]

Sending for feedback on this small change while test still under work.

We need to both (1) run a compositing update on FrameView in same frame in which
it is shown/hidden, and (2) restart any animations via
Element::elementAnimations()::restartAnimationOnCompositor() as ajuma@ had
suggested might be necessary.

https://codereview.chromium.org/2010963003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/compositing/layer-creation/toggle-animated-iframe-visibility.html
(right):

https://codereview.chromium.org/2010963003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/compositing/layer-creation/toggle-animated-iframe-visibility.html:11:
runAfterLayoutAndPaint(function() {
This test reproduces the issue run locally in content shell and observed
visually, but is not yet useful with run-webkit-tests. I'm still exploring
options. Unfortunately existing animation test infrastructure doesn't look
compatible with an animation performed within an iframe, and workarounds to use
layer tree text directly show some alternate form of flakiness not yet well
understood.

https://codereview.chromium.org/2010963003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2010963003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3887:
setNeedsCompositingUpdate(layoutViewItem(), CompositingUpdateRebuildTree);
As an alternative to adding this here and in line 3906 below, we could override
Widget::setSelfVisible() and then we could do this, and updateScrollableAreaSet,
in there. Should be equivalent. I sort of prefer not overriding things since
perhaps we don't always want to do these two operations, but I can be convinced.
Note we do already override Widget::setParentVisible() above.

https://codereview.chromium.org/2010963003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3906:
setNeedsCompositingUpdate(layoutViewItem(), CompositingUpdateRebuildTree);
This is not needed to fix the bug, but it maintains symmetry with show() and I
believe that without this a composited iframe subsequently hidden won't have its
then-unneeded layer removed until something else causes a lifecycle update
through to compositing clean.

[chrishtr, 2016-05-28 21:18:21]

chrishtr@chromium.org changed reviewers:
+ dstockwell@chromium.org

[chrishtr, 2016-05-28 21:18:22]

https://codereview.chromium.org/2010963003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp
(right):

https://codereview.chromium.org/2010963003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:495:
restartAnimationOnCompositor(*layer->layoutObject());
One thing I'd like to understand: in the scenario of a composited animation
where
in the middle of it, will-change: transform is removed and then added again,
does the animation start running on the compositor again? If so, what makes it
do
so in the code before this CL?

[ajuma, 2016-05-29 00:49:28]

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

[ajuma, 2016-05-29 00:49:29]

https://codereview.chromium.org/2010963003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp
(right):

https://codereview.chromium.org/2010963003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:495:
restartAnimationOnCompositor(*layer->layoutObject());
On 2016/05/28 21:18:22, chrishtr wrote:
> One thing I'd like to understand: in the scenario of a composited animation
> where
> in the middle of it, will-change: transform is removed and then added again,
> does the animation start running on the compositor again? If so, what makes it
> do
> so in the code before this CL?

Having a transform animation is a direct compositing reason, so adding/removing
will-change in the middle has no effect (the composited layer persists for the
duration of the animation). The case this CL is trying to handle is unusual: the
layer has a direct compositing reason (the animation), but we're still choosing
to stop compositing it because it's inside an invisible frame.

[ajuma, 2016-05-29 00:50:24]

Description was changed from

==========
Restart any animations when we allocate a composited layer.

As part of this, fix FrameView to note a compositing update is needed
when it is shown/hidden since it could gain/lose its composited layer
and, if shown/gained, then we want to immediately restart any
animations.

BUG=612436
==========

to

==========
Restart any animations when we allocate a composited layer.

As part of this, fix FrameView to note a compositing update is needed
when it is shown/hidden since it could gain/lose its composited layer
and, if shown/gained, then we want to immediately restart any
animations.

BUG=612436
==========

[ajuma, 2016-05-29 00:50:25]

ajuma@chromium.org changed reviewers:
- ajuma@chromium.org

[wkorman, 2016-05-31 22:47:09]

Filed http://crbug.com/616270 to look into a test separately due to test
flakiness that could take notable time to understand and resolve. Will
investigate further with animation team, but I believe it is worth landing this
change in meantime.

[chrishtr, 2016-05-31 22:53:34]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-05-31 22:53:34]

lgtm

[commit-bot: I haz the power, 2016-05-31 22:54:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2010963003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2010963003/60001

[commit-bot: I haz the power, 2016-06-01 00:57:49]

Description was changed from

==========
Restart any animations when we allocate a composited layer.

As part of this, fix FrameView to note a compositing update is needed
when it is shown/hidden since it could gain/lose its composited layer
and, if shown/gained, then we want to immediately restart any
animations.

BUG=612436
==========

to

==========
Restart any animations when we allocate a composited layer.

As part of this, fix FrameView to note a compositing update is needed
when it is shown/hidden since it could gain/lose its composited layer
and, if shown/gained, then we want to immediately restart any
animations.

BUG=612436
==========

[commit-bot: I haz the power, 2016-06-01 00:57:50]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-01 01:00:08]

Description was changed from

==========
Restart any animations when we allocate a composited layer.

As part of this, fix FrameView to note a compositing update is needed
when it is shown/hidden since it could gain/lose its composited layer
and, if shown/gained, then we want to immediately restart any
animations.

BUG=612436
==========

to

==========
Restart any animations when we allocate a composited layer.

As part of this, fix FrameView to note a compositing update is needed
when it is shown/hidden since it could gain/lose its composited layer
and, if shown/gained, then we want to immediately restart any
animations.

BUG=612436

Committed: https://crrev.com/851bbadbc2348dcf79851c9be1b766e4ce00a2ab
Cr-Commit-Position: refs/heads/master@{#397003}
==========

[commit-bot: I haz the power, 2016-06-01 01:00:10]

Patchset 4 (id:??) landed as
https://crrev.com/851bbadbc2348dcf79851c9be1b766e4ce00a2ab
Cr-Commit-Position: refs/heads/master@{#397003}