Construct KeyframeEffectReadOnly objects

Construct KeyframeEffectReadOnly objects

This patch implements the last part of the fix for the bug identified in
https://codereview.chromium.org/2389323003. The content of the
KeyframeEffect::create functions is copied to KeyframeEffectReadOnly::create
functions instead of the latter being a simple wrapper around the former.

While this introduces some code duplication, there seems to be very little in
the way of meaningful refactoring that can be done. The content of the functions
is largely plumbing arguments from one place to another, and the ::create
functions can fail (returning nullptr), so the work cannot be moved into
constructors.

BUG=624639
Committed: https://crrev.com/d7e233e169431f279af4c78940a0d92784e79ddf
Cr-Commit-Position: refs/heads/master@{#431119}

[suzyh_UTC10 (ex-contributor), 2016-10-07 01:27:28]

suzyh@chromium.org changed reviewers:
+ alancutter@chromium.org

[alancutter (OOO until 2018), 2016-10-07 03:30:12]

Do we have tests for animations using KeyframeEffectReadOnly effects?

[suzyh_UTC10 (ex-contributor), 2016-10-09 23:06:53]

On 2016/10/07 at 03:30:12, alancutter wrote:
> Do we have tests for animations using KeyframeEffectReadOnly effects?

The WPT have several tests, but they use the Animation constructor and so don't
run successfully yet. I've added an additional layout test that creates an
animation with the KeyframeEffectReadOnly object via Element.animate instead. Do
you have any suggestions for extra things to add to this test?

[alancutter (OOO until 2018), 2016-10-10 00:19:05]

On 2016/10/09 at 23:06:53, suzyh wrote:
> On 2016/10/07 at 03:30:12, alancutter wrote:
> > Do we have tests for animations using KeyframeEffectReadOnly effects?
> 
> The WPT have several tests, but they use the Animation constructor and so
don't run successfully yet. I've added an additional layout test that creates an
animation with the KeyframeEffectReadOnly object via Element.animate instead. Do
you have any suggestions for extra things to add to this test?

Test that the effect applies to CSS properties via getComputedStyle(). This is
probably not upstream worthy to WPTs but I think it's important that we actually
execute that code path in our tests.

[suzyh_UTC10 (ex-contributor), 2016-10-10 04:19:23]

On 2016/10/10 at 00:19:05, alancutter wrote:
> On 2016/10/09 at 23:06:53, suzyh wrote:
> > On 2016/10/07 at 03:30:12, alancutter wrote:
> > > Do we have tests for animations using KeyframeEffectReadOnly effects?
> > 
> > The WPT have several tests, but they use the Animation constructor and so
don't run successfully yet. I've added an additional layout test that creates an
animation with the KeyframeEffectReadOnly object via Element.animate instead. Do
you have any suggestions for extra things to add to this test?
> 
> Test that the effect applies to CSS properties via getComputedStyle(). This is
probably not upstream worthy to WPTs but I think it's important that we actually
execute that code path in our tests.

Done. I added an extra test to the file I added in the last patch, which caused
it to crash. The fix was to change a toKeyframeEffect conversion in
ElementAnimations.cpp to a toKeyframeEffectReadOnly conversion.

I'll have a look at some of the other isKeyframeEffect/toKeyframeEffect calls to
see if there are any others that look like they should be
KeyframeEffectReadOnlys, and check those are covered by tests too.

[suzyh_UTC10 (ex-contributor), 2016-10-10 04:20:03]

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

[commit-bot: I haz the power, 2016-10-10 04:20:16]

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

[alancutter (OOO until 2018), 2016-10-10 04:21:31]

On 2016/10/10 at 04:19:23, suzyh wrote:
> On 2016/10/10 at 00:19:05, alancutter wrote:
> > On 2016/10/09 at 23:06:53, suzyh wrote:
> > > On 2016/10/07 at 03:30:12, alancutter wrote:
> > > > Do we have tests for animations using KeyframeEffectReadOnly effects?
> > > 
> > > The WPT have several tests, but they use the Animation constructor and so
don't run successfully yet. I've added an additional layout test that creates an
animation with the KeyframeEffectReadOnly object via Element.animate instead. Do
you have any suggestions for extra things to add to this test?
> > 
> > Test that the effect applies to CSS properties via getComputedStyle(). This
is probably not upstream worthy to WPTs but I think it's important that we
actually execute that code path in our tests.
> 
> Done. I added an extra test to the file I added in the last patch, which
caused it to crash. The fix was to change a toKeyframeEffect conversion in
ElementAnimations.cpp to a toKeyframeEffectReadOnly conversion.
> 
> I'll have a look at some of the other isKeyframeEffect/toKeyframeEffect calls
to see if there are any others that look like they should be
KeyframeEffectReadOnlys, and check those are covered by tests too.

Thanks! That's the sort of thing I was worried about. There are probably
instances in compositor animation related code.

[alancutter (OOO until 2018), 2016-10-10 04:22:35]

On 2016/10/10 at 04:21:31, alancutter wrote:
> On 2016/10/10 at 04:19:23, suzyh wrote:
> > On 2016/10/10 at 00:19:05, alancutter wrote:
> > > On 2016/10/09 at 23:06:53, suzyh wrote:
> > > > On 2016/10/07 at 03:30:12, alancutter wrote:
> > > > > Do we have tests for animations using KeyframeEffectReadOnly effects?
> > > > 
> > > > The WPT have several tests, but they use the Animation constructor and
so don't run successfully yet. I've added an additional layout test that creates
an animation with the KeyframeEffectReadOnly object via Element.animate instead.
Do you have any suggestions for extra things to add to this test?
> > > 
> > > Test that the effect applies to CSS properties via getComputedStyle().
This is probably not upstream worthy to WPTs but I think it's important that we
actually execute that code path in our tests.
> > 
> > Done. I added an extra test to the file I added in the last patch, which
caused it to crash. The fix was to change a toKeyframeEffect conversion in
ElementAnimations.cpp to a toKeyframeEffectReadOnly conversion.
> > 
> > I'll have a look at some of the other isKeyframeEffect/toKeyframeEffect
calls to see if there are any others that look like they should be
KeyframeEffectReadOnlys, and check those are covered by tests too.
> 
> Thanks! That's the sort of thing I was worried about. There are probably
instances in compositor animation related code.

Try the test under virtual/threaded, it may already be covering the compositor
binding code.

[commit-bot: I haz the power, 2016-10-10 05:42:40]

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

[commit-bot: I haz the power, 2016-10-10 05:42:41]

Dry run: This issue passed the CQ dry run.

[suzyh_UTC10 (ex-contributor), 2016-10-26 23:31:26]

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

[commit-bot: I haz the power, 2016-10-26 23:32:24]

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, 2016-10-27 00:54:59]

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

[commit-bot: I haz the power, 2016-10-27 00:55:00]

Dry run: This issue passed the CQ dry run.

[suzyh_UTC10 (ex-contributor), 2016-10-27 00:58:56]

I've now added a couple more tests:
- animations: Tests that using a KERO for the animation works, gives the right
types in the JS objects, and doesn't cause a crash when applying an effect.
- virtual/threaded/animations: Prior to changing some KE to KERO in the code,
the checks about whether something could be sent to the compositor thread did
not work if the effect was created as a KERO.
- inspector/animation: Loading a test with the KERO-based animation, while the
DevTools animation inspector was open, caused a crash.


I don't think that all code changes I've made are directly exercised by these
tests:

One of the changes I made was allowing the Animation.m_content object to be a
KeyframeEffectReadOnly instead of a KeyframeEffect; I changed many
isKeyframeEffect/toKeyframeEffect tests/conversions to KERO as a result of this
without strictly requiring that each change was being executed by a test.

The inspector/InspectorTraceEvents.cpp change is not gated by a test; if I
understand the code correctly, without this change some piece of information
would just not be available in a trace in the case that an animation was a KERO
instead of a KE.


KEs that have not been changed to KEROs:
- ElementAnimation.h animateInternal creates a KE. By my understanding, this
means that any call to Element.animate will cause us to use a KE object
internally.
- CustomCompositorAnimations.cpp creates and uses KEs. This seems to be
self-contained. In our discussions offline I believe you suggested I leave this
as-is; correct me if I have mis-remembered.
- Unit tests (CompositorAnimationsTest.cpp, DocumentTimelineTest.cpp,
AnimationTimelineTest.cpp, AnimationStackTest.cpp, KeyframeEffectTest.cpp,
AnimationTest.cpp) I have left untouched - these still create KEs.
- css/CSSAnimations.cpp creates KEs from InertEffects and plays them in
maybeApplyPendingUpdate. I expect this is a similar usage to that in
ElementAnimation.h.
- ../inspector/InspectorAnimationAgent.cpp creates a KE when calling
animationClone. I've left one other toKeyframeEffect conversion as-is, which
acts on the output of animationClone, and requires the effect timing to be
mutable.


I'm not confident that this is sufficiently tested, or that I have done the
right combinations of changing-KE-to-KERO and not-changing-KE-to-KERO. What do
you think?

[alancutter (OOO until 2018), 2016-10-28 03:38:32]

lgtm

https://codereview.chromium.org/2398373002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/animation/animation-KeyframeEffectReadOnly.html
(right):

https://codereview.chromium.org/2398373002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/animation/animation-KeyframeEffectReadOnly.html:50:
KeyframeEffectReadOnly.
This description should be more clear that we're only testing for a lack of
crashing here.

https://codereview.chromium.org/2398373002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/threaded/animations/KeyframeEffectReadOnly-composited-animation.html
(right):

https://codereview.chromium.org/2398373002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/threaded/animations/KeyframeEffectReadOnly-composited-animation.html:6:
<script src="../../../imported/wpt/web-animations/testcommon.js"></script>
Not sure we need to depend on this library for a single function especially in a
test that will never be upstreamed.
I suggest just using
<div id="target"></div>
<div id="targetRO"></div>
as your targets.

https://codereview.chromium.org/2398373002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/threaded/animations/KeyframeEffectReadOnly-composited-animation.html:31:
t.done();
Nit: If you use promise_test() you can avoid the t handle.

[suzyh_UTC10 (ex-contributor), 2016-10-30 22:56:32]

suzyh@chromium.org changed reviewers:
+ pfeldman@chromium.org

[suzyh_UTC10 (ex-contributor), 2016-10-30 22:57:20]

+pfeldman for inspector


On 2016/10/28 at 03:38:32, alancutter wrote:
> lgtm
> 
>
https://codereview.chromium.org/2398373002/diff/100001/third_party/WebKit/Lay...
> File
third_party/WebKit/LayoutTests/inspector/animation/animation-KeyframeEffectReadOnly.html
(right):
> 
>
https://codereview.chromium.org/2398373002/diff/100001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/inspector/animation/animation-KeyframeEffectReadOnly.html:50:
KeyframeEffectReadOnly.
> This description should be more clear that we're only testing for a lack of
crashing here.

Done.


https://codereview.chromium.org/2398373002/diff/100001/third_party/WebKit/Lay...
> File
third_party/WebKit/LayoutTests/virtual/threaded/animations/KeyframeEffectReadOnly-composited-animation.html
(right):
> 
>
https://codereview.chromium.org/2398373002/diff/100001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/virtual/threaded/animations/KeyframeEffectReadOnly-composited-animation.html:6:
<script src="../../../imported/wpt/web-animations/testcommon.js"></script>
> Not sure we need to depend on this library for a single function especially in
a test that will never be upstreamed.
> I suggest just using
> <div id="target"></div>
> <div id="targetRO"></div>
> as your targets.

Done.


https://codereview.chromium.org/2398373002/diff/100001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/virtual/threaded/animations/KeyframeEffectReadOnly-composited-animation.html:31:
t.done();
> Nit: If you use promise_test() you can avoid the t handle.

Done.

[suzyh_UTC10 (ex-contributor), 2016-10-30 22:57:28]

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

[commit-bot: I haz the power, 2016-10-30 22:57:33]

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, 2016-10-31 00:08:39]

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

[commit-bot: I haz the power, 2016-10-31 00:08:40]

Dry run: This issue passed the CQ dry run.

[suzyh_UTC10 (ex-contributor), 2016-11-02 01:35:20]

pfeldman: Ping? Feel free to redirect to another reviewer if appropriate.
Thanks!

[suzyh_UTC10 (ex-contributor), 2016-11-09 04:55:58]

suzyh@chromium.org changed reviewers:
+ dgozman@chromium.org
- pfeldman@chromium.org

[suzyh_UTC10 (ex-contributor), 2016-11-09 04:56:55]

-pfeldman, +dgozman

dgozman: Do you have time to review this for core/inspector and the inspector
layout tests? If not, can you recommend an alternative reviewer? Thanks!

[dgozman, 2016-11-09 18:14:17]

inspector lgtm

[suzyh_UTC10 (ex-contributor), 2016-11-09 23:02:47]

The CQ bit was checked by suzyh@chromium.org

[suzyh_UTC10 (ex-contributor), 2016-11-09 23:02:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/2398373002/#ps120001
(title: "Test tweaks in response to review")

[commit-bot: I haz the power, 2016-11-09 23:03:28]

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-11-10 01:00:49]

Committed patchset #7 (id:120001)

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

Description was changed from

==========
Construct KeyframeEffectReadOnly objects

This patch implements the last part of the fix for the bug identified in
https://codereview.chromium.org/2389323003. The content of the
KeyframeEffect::create functions is copied to KeyframeEffectReadOnly::create
functions instead of the latter being a simple wrapper around the former.

While this introduces some code duplication, there seems to be very little in
the way of meaningful refactoring that can be done. The content of the functions
is largely plumbing arguments from one place to another, and the ::create
functions can fail (returning nullptr), so the work cannot be moved into
constructors.

BUG=624639
==========

to

==========
Construct KeyframeEffectReadOnly objects

This patch implements the last part of the fix for the bug identified in
https://codereview.chromium.org/2389323003. The content of the
KeyframeEffect::create functions is copied to KeyframeEffectReadOnly::create
functions instead of the latter being a simple wrapper around the former.

While this introduces some code duplication, there seems to be very little in
the way of meaningful refactoring that can be done. The content of the functions
is largely plumbing arguments from one place to another, and the ::create
functions can fail (returning nullptr), so the work cannot be moved into
constructors.

BUG=624639

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

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

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