Paint some content for animated elements in composited animation tests.

Paint some content for animated elements in composited animation tests.

In SPv2 we won't create a layer for an element that paints no content.
For composited animation focused layout tests we explicitly want to
make sure the element under test is composited. Thus we must paint
some kind of content (what it is doesn't actually matter).

Here we add an 'x' as dummy text content to be painted by any element that is
intended to be animated. We could alternatively set a background color and
ensure the element has some non-empty size, but that involves more styling which
feels more intrusive.

BUG=692310
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2702483002
Cr-Commit-Position: refs/heads/master@{#453303}
Committed: https://chromium.googlesource.com/chromium/src/+/6d087c27d71247d697ae2afee12b5a7532551e68

[wkorman, 2017-02-16 00:22:21]

Description was changed from

==========
Paint some content for animated elements in composited animation tests.

In SPv2 we won't create a layer for an element that paints no content.
For composited animation focused layout tests we explicitly want to
make sure the element under test is composited. Thus we must paint
some kind of content (what it is doesn't actually matter).

Here we add an 'x' as dummy text content to be painted by any element that is
intended to be animated. We could alternatively set a background color and
ensure the element has some non-empty size, but that involves more styling which
feels more intrusive.

BUG=692310
==========

to

==========
Paint some content for animated elements in composited animation tests.

In SPv2 we won't create a layer for an element that paints no content.
For composited animation focused layout tests we explicitly want to
make sure the element under test is composited. Thus we must paint
some kind of content (what it is doesn't actually matter).

Here we add an 'x' as dummy text content to be painted by any element that is
intended to be animated. We could alternatively set a background color and
ensure the element has some non-empty size, but that involves more styling which
feels more intrusive.

BUG=692310
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[wkorman, 2017-02-16 00:22:43]

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

[commit-bot: I haz the power, 2017-02-16 00:23:24]

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

[wkorman, 2017-02-16 00:24:00]

wkorman@chromium.org changed reviewers:
+ alancutter@chromium.org, pdr@chromium.org, suzyh@chromium.org

[wkorman, 2017-02-16 00:24:01]

First pass attempt. This passes with SPv2 enabled on my local workstation. It
looks like we don't have pixel tests turned on for these, so this seems a
reasonably unobtrusive SPv2-compatible update for these tests.

Open to thoughts per the separate email discussion.

[alancutter (OOO until 2018), 2017-02-16 01:12:30]

https://codereview.chromium.org/2702483002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/animations/sample-on-last-keyframe.html
(right):

https://codereview.chromium.org/2702483002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/animations/sample-on-last-keyframe.html:37: <div
id="test">x</div>
This test doesn't depend on the animation being composited as it uses
getComputedStyle(). It should be left as is.

https://codereview.chromium.org/2702483002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/animations/zoom-responsive-transform-animation.html
(right):

https://codereview.chromium.org/2702483002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/animations/zoom-responsive-transform-animation.html:55:
return footer.animate({opacity: ['1', '1']}, 1).ready;
Does the footer not need content as well? We use composited animations on it to
sync with the compositor before proceeding in this test.

[commit-bot: I haz the power, 2017-02-16 01:14:34]

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

[commit-bot: I haz the power, 2017-02-16 01:14:35]

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

[wkorman, 2017-02-17 21:42:31]

https://codereview.chromium.org/2702483002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/animations/sample-on-last-keyframe.html
(right):

https://codereview.chromium.org/2702483002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/animations/sample-on-last-keyframe.html:37: <div
id="test">x</div>
On 2017/02/16 at 01:12:30, alancutter wrote:
> This test doesn't depend on the animation being composited as it uses
getComputedStyle(). It should be left as is.

This test hangs when run in SPv2 via:

1. comment out the entry for the test in
third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2)
2. % ./third_party/WebKit/Tools/Scripts/run-webkit-tests
--additional-driver-flag=--enable-slimming-paint-v2
virtual/threaded/animations/sample-on-last-keyframe.html

AFAIK this is due to the animation being considered compositable as it is a
straightforward opacity animation from 0 to 1, but per the linked bug, without
an 'x' or some painted content SPv2 doesn't create a layer for the animation,
and so the animation is never ticked/started.

Through debug logging I find that animation-test-helpers.js is adding an event
listener on webkitAnimationStart which never triggers. This can be explained by
the above -- the animation metadata comes into existence but it is never started
thus the event never fires.

Adding some painted content fixes for same reason in rest of change. Thoughts?

https://codereview.chromium.org/2702483002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/animations/zoom-responsive-transform-animation.html
(right):

https://codereview.chromium.org/2702483002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/animations/zoom-responsive-transform-animation.html:55:
return footer.animate({opacity: ['1', '1']}, 1).ready;
On 2017/02/16 01:12:30, alancutter wrote:
> Does the footer not need content as well? We use composited animations on it
to
> sync with the compositor before proceeding in this test.

Ah, yes, but, weird. I think there's something else going on with this test. It
passes when run via content shell but still times out as a layout test, even
with content in footer, and target.

I'll going to remove it from this change and investigate separately.

[alancutter (OOO until 2018), 2017-02-20 00:18:30]

https://codereview.chromium.org/2702483002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/animations/sample-on-last-keyframe.html
(right):

https://codereview.chromium.org/2702483002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/animations/sample-on-last-keyframe.html:37: <div
id="test">x</div>
On 2017/02/17 at 21:42:31, wkorman wrote:
> On 2017/02/16 at 01:12:30, alancutter wrote:
> > This test doesn't depend on the animation being composited as it uses
getComputedStyle(). It should be left as is.
> 
> This test hangs when run in SPv2 via:
> 
> 1. comment out the entry for the test in
third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2)
> 2. % ./third_party/WebKit/Tools/Scripts/run-webkit-tests
--additional-driver-flag=--enable-slimming-paint-v2
virtual/threaded/animations/sample-on-last-keyframe.html
> 
> AFAIK this is due to the animation being considered compositable as it is a
straightforward opacity animation from 0 to 1, but per the linked bug, without
an 'x' or some painted content SPv2 doesn't create a layer for the animation,
and so the animation is never ticked/started.
> 
> Through debug logging I find that animation-test-helpers.js is adding an event
listener on webkitAnimationStart which never triggers. This can be explained by
the above -- the animation metadata comes into existence but it is never started
thus the event never fires.
> 
> Adding some painted content fixes for same reason in rest of change. Thoughts?

I consider this test failing as an indication that bug 692310 is active. I think
we should set failure expectations rather than add workarounds to avoid the bug.

The other test changes have been things that depend on implementation specific
details and need updating to match the future animation compositing behaviour.

https://codereview.chromium.org/2702483002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/animations/zoom-responsive-transform-animation.html
(right):

https://codereview.chromium.org/2702483002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/animations/zoom-responsive-transform-animation.html:55:
return footer.animate({opacity: ['1', '1']}, 1).ready;
On 2017/02/17 at 21:42:31, wkorman wrote:
> On 2017/02/16 01:12:30, alancutter wrote:
> > Does the footer not need content as well? We use composited animations on it
to
> > sync with the compositor before proceeding in this test.
> 
> Ah, yes, but, weird. I think there's something else going on with this test.
It passes when run via content shell but still times out as a layout test, even
with content in footer, and target.
> 
> I'll going to remove it from this change and investigate separately.

I suspect the test is just flaky on the side of passing rather than failing. It
will probably fail more often on debug Windows builds. We should make the footer
a composited animation to avoid the flakiness.

[wkorman, 2017-02-23 21:36:59]

https://codereview.chromium.org/2702483002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/animations/sample-on-last-keyframe.html
(right):

https://codereview.chromium.org/2702483002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/animations/sample-on-last-keyframe.html:37: <div
id="test">x</div>
On 2017/02/20 00:18:30, alancutter wrote:
> On 2017/02/17 at 21:42:31, wkorman wrote:
> > On 2017/02/16 at 01:12:30, alancutter wrote:
> > > This test doesn't depend on the animation being composited as it uses
> getComputedStyle(). It should be left as is.
> > 
> > This test hangs when run in SPv2 via:
> > 
> > 1. comment out the entry for the test in
> third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2)
> > 2. % ./third_party/WebKit/Tools/Scripts/run-webkit-tests
> --additional-driver-flag=--enable-slimming-paint-v2
> virtual/threaded/animations/sample-on-last-keyframe.html
> > 
> > AFAIK this is due to the animation being considered compositable as it is a
> straightforward opacity animation from 0 to 1, but per the linked bug, without
> an 'x' or some painted content SPv2 doesn't create a layer for the animation,
> and so the animation is never ticked/started.
> > 
> > Through debug logging I find that animation-test-helpers.js is adding an
event
> listener on webkitAnimationStart which never triggers. This can be explained
by
> the above -- the animation metadata comes into existence but it is never
started
> thus the event never fires.
> > 
> > Adding some painted content fixes for same reason in rest of change.
Thoughts?
> 
> I consider this test failing as an indication that bug 692310 is active. I
think
> we should set failure expectations rather than add workarounds to avoid the
bug.
> 
> The other test changes have been things that depend on implementation specific
> details and need updating to match the future animation compositing behaviour.

I see what you're saying, but let me make sure I understand what you're
thinking. So once we fix 692310, for SPv2, this test as currently written will
cease to consider this animation a composited animation when run as
virtual/threaded/animations/sample-on-last-keyframe.html, because it paints no
content and so has no layer.

But isn't the intent of having this test included under the virtual/threaded
test suite to ensure that this case (on last keyframe, the opacity is what we
expect, within some tolerance) runs correctly as both a composited and
non-composited animation?

And so once we have "fixed" 692310, unless we modify this test to paint some
content, it will essentially be running as a non-composited animation even when
run under virtual/threaded. So it doesn't seem to me like it would then be
testing what it's aiming to test. But I am quite likely missing something.

[alancutter (OOO until 2018), 2017-02-24 00:12:42]

On 2017/02/23 at 21:36:59, wkorman wrote:
> But isn't the intent of having this test included under the virtual/threaded
test suite to ensure that this case (on last keyframe, the opacity is what we
expect, within some tolerance) runs correctly as both a composited and
non-composited animation?

It gets run under virtual/threaded just because it's in the animations
directory, it uses getComputedStyle() under the hood which uses the main thread
code path for timing/interpolation calculations. This does not test what the
compositor outputs, the only way we can do that currently is to use ref tests
and compare the final pixel output. Despite this it still caught a bug in
compositor animations. (:

[wkorman, 2017-02-24 18:32:31]

On 2017/02/24 00:12:42, alancutter wrote:
> On 2017/02/23 at 21:36:59, wkorman wrote:
> > But isn't the intent of having this test included under the virtual/threaded
> test suite to ensure that this case (on last keyframe, the opacity is what we
> expect, within some tolerance) runs correctly as both a composited and
> non-composited animation?
> 
> It gets run under virtual/threaded just because it's in the animations
> directory, it uses getComputedStyle() under the hood which uses the main
thread
> code path for timing/interpolation calculations. This does not test what the
> compositor outputs, the only way we can do that currently is to use ref tests
> and compare the final pixel output. Despite this it still caught a bug in
> compositor animations. (:

OK, I get it now. Un-did revisions to this test and put it back as a timeout
failure tagged with bug 692310.

PTAL I think this change is now suitable for landing. I am still working
on the other changes we've discussed.

[alancutter (OOO until 2018), 2017-02-27 03:11:29]

lgtm, thanks!

[wkorman, 2017-02-27 18:35:12]

The CQ bit was checked by wkorman@chromium.org

[commit-bot: I haz the power, 2017-02-27 18:36:08]

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-02-27 19:48:58]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488220512434330,
"parent_rev": "329d64d814fc7af1b3e3673acb461599f514c5cf", "commit_rev":
"6d087c27d71247d697ae2afee12b5a7532551e68"}

[commit-bot: I haz the power, 2017-02-27 19:50:21]

Description was changed from

==========
Paint some content for animated elements in composited animation tests.

In SPv2 we won't create a layer for an element that paints no content.
For composited animation focused layout tests we explicitly want to
make sure the element under test is composited. Thus we must paint
some kind of content (what it is doesn't actually matter).

Here we add an 'x' as dummy text content to be painted by any element that is
intended to be animated. We could alternatively set a background color and
ensure the element has some non-empty size, but that involves more styling which
feels more intrusive.

BUG=692310
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Paint some content for animated elements in composited animation tests.

In SPv2 we won't create a layer for an element that paints no content.
For composited animation focused layout tests we explicitly want to
make sure the element under test is composited. Thus we must paint
some kind of content (what it is doesn't actually matter).

Here we add an 'x' as dummy text content to be painted by any element that is
intended to be animated. We could alternatively set a background color and
ensure the element has some non-empty size, but that involves more styling which
feels more intrusive.

BUG=692310
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2702483002
Cr-Commit-Position: refs/heads/master@{#453303}
Committed:
https://chromium.googlesource.com/chromium/src/+/6d087c27d71247d697ae2afee12b...
==========

[commit-bot: I haz the power, 2017-02-27 19:50:22]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/6d087c27d71247d697ae2afee12b...