Fix crash in keyframe-effect/getComputedTiming tests

Fix crash in keyframe-effect/getComputedTiming tests

The keyframe-effect/getComputedTiming-{progress,currentIteration}.html
W3C tests caused an assertion failure in animations with a non-zero
value for iterationStart and a duration value of Infinity. The timing
calculations in the spec that we were following need to be reworked
(see https://github.com/w3c/web-animations/issues/142) but in the
meantime this patch adds a workaround for infinite duration animations
that avoids the assertion (caused by trying to calculate inf/inf).

BUG=600248
Committed: https://crrev.com/ebd2f53756915bbb24eb7230c2fdfa6b8fd0c86a
Cr-Commit-Position: refs/heads/master@{#388386}

[suzyh_UTC10 (ex-contributor), 2016-04-15 00:30:36]

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

[suzyh_UTC10 (ex-contributor), 2016-04-15 00:31:02]

We still fail the test, but now we don't crash X-D

[suzyh_UTC10 (ex-contributor), 2016-04-15 00:31:07]

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

[commit-bot: I haz the power, 2016-04-15 00:31:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1888983003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1888983003/1

[alancutter (OOO until 2018), 2016-04-15 01:01:08]

We should add a basic test that covers this behavioural change given that the
existing tests fail for other reasons.

[alancutter (OOO until 2018), 2016-04-15 01:02:00]

https://codereview.chromium.org/1888983003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/AnimationEffect.cpp (right):

https://codereview.chromium.org/1888983003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/AnimationEffect.cpp:162: if
(!std::isfinite(iterationDuration))
Is there a spec bug we can link to given that this a work around a problem with
the spec?

[suzyh_UTC10 (ex-contributor), 2016-04-15 01:26:09]

On 2016/04/15 at 01:02:00, alancutter wrote:
>
https://codereview.chromium.org/1888983003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/animation/AnimationEffect.cpp (right):
> 
>
https://codereview.chromium.org/1888983003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/animation/AnimationEffect.cpp:162: if
(!std::isfinite(iterationDuration))
> Is there a spec bug we can link to given that this a work around a problem
with the spec?

There's a spec bug linked from the CL description. Do you want it in a comment
in the code as well?

[suzyh_UTC10 (ex-contributor), 2016-04-15 01:42:55]

On 2016/04/15 at 01:01:08, alancutter wrote:
> We should add a basic test that covers this behavioural change given that the
existing tests fail for other reasons.

I'm not sure what you have in mind. If we expose getComputedTiming, so that many
of these tests have a chance to pass, would these getComputedTiming tests be
sufficient?

[commit-bot: I haz the power, 2016-04-15 02:53:45]

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

[commit-bot: I haz the power, 2016-04-15 02:53:46]

Dry run: This issue passed the CQ dry run.

[alancutter (OOO until 2018), 2016-04-15 03:59:23]

On 2016/04/15 at 01:26:09, suzyh wrote:
> On 2016/04/15 at 01:02:00, alancutter wrote:
> >
https://codereview.chromium.org/1888983003/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/animation/AnimationEffect.cpp (right):
> > 
> >
https://codereview.chromium.org/1888983003/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/animation/AnimationEffect.cpp:162: if
(!std::isfinite(iterationDuration))
> > Is there a spec bug we can link to given that this a work around a problem
with the spec?
> 
> There's a spec bug linked from the CL description. Do you want it in a comment
in the code as well?

Any part of the code that deliberately goes against spec should have
justification next to it for future readers of the code, I think adding a
comment is best here.

On 2016/04/15 at 01:42:55, suzyh wrote:
> On 2016/04/15 at 01:01:08, alancutter wrote:
> > We should add a basic test that covers this behavioural change given that
the existing tests fail for other reasons.
> 
> I'm not sure what you have in mind. If we expose getComputedTiming, so that
many of these tests have a chance to pass, would these getComputedTiming tests
be sufficient?

Example test:
target.animate({opacity: [0, 1]}, {duration: Infinity, iterationStart: 4.75});
assert_equals(getComputedStyle(target).opacity, 0.75);

[suzyh_UTC10 (ex-contributor), 2016-04-15 04:43:57]

On 2016/04/15 at 03:59:23, alancutter wrote:
> On 2016/04/15 at 01:26:09, suzyh wrote:
> > On 2016/04/15 at 01:02:00, alancutter wrote:
> > >
https://codereview.chromium.org/1888983003/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/animation/AnimationEffect.cpp (right):
> > > 
> > >
https://codereview.chromium.org/1888983003/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/animation/AnimationEffect.cpp:162: if
(!std::isfinite(iterationDuration))
> > > Is there a spec bug we can link to given that this a work around a problem
with the spec?
> > 
> > There's a spec bug linked from the CL description. Do you want it in a
comment in the code as well?
> 
> Any part of the code that deliberately goes against spec should have
justification next to it for future readers of the code, I think adding a
comment is best here.

Done.


> On 2016/04/15 at 01:42:55, suzyh wrote:
> > On 2016/04/15 at 01:01:08, alancutter wrote:
> > > We should add a basic test that covers this behavioural change given that
the existing tests fail for other reasons.
> > 
> > I'm not sure what you have in mind. If we expose getComputedTiming, so that
many of these tests have a chance to pass, would these getComputedTiming tests
be sufficient?
> 
> Example test:
> target.animate({opacity: [0, 1]}, {duration: Infinity, iterationStart: 4.75});
> assert_equals(getComputedStyle(target).opacity, 0.75);

Done.

[alancutter (OOO until 2018), 2016-04-18 01:42:22]

lgtm

[suzyh_UTC10 (ex-contributor), 2016-04-18 01:47:47]

The CQ bit was checked by suzyh@chromium.org

[commit-bot: I haz the power, 2016-04-18 01:48:04]

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

[commit-bot: I haz the power, 2016-04-18 03:05:47]

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

[commit-bot: I haz the power, 2016-04-18 03:05:48]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[suzyh_UTC10 (ex-contributor), 2016-04-19 03:11:46]

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

[commit-bot: I haz the power, 2016-04-19 03:12:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1888983003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1888983003/40001

[suzyh_UTC10 (ex-contributor), 2016-04-19 03:13:04]

The commit failed due to the virtual/threaded version of the new test; I've
added a related workaround to CompositorAnimations.cpp to handle that case.

PTAL

[alancutter (OOO until 2018), 2016-04-19 04:49:13]

lgtm

We should probably make the new test a ref test instead of using testharness.js
so that composited animations get correctness coverage for this case, not just
assertion coverage.

[commit-bot: I haz the power, 2016-04-19 05:01:52]

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

[commit-bot: I haz the power, 2016-04-19 05:01:53]

Dry run: This issue passed the CQ dry run.

[suzyh_UTC10 (ex-contributor), 2016-04-19 07:40:09]

On 2016/04/19 at 04:49:13, alancutter wrote:
> lgtm
> 
> We should probably make the new test a ref test instead of using
testharness.js so that composited animations get correctness coverage for this
case, not just assertion coverage.

Done.

[suzyh_UTC10 (ex-contributor), 2016-04-19 07:40:15]

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

[commit-bot: I haz the power, 2016-04-19 07:40:29]

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

[commit-bot: I haz the power, 2016-04-19 08:51:36]

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

[commit-bot: I haz the power, 2016-04-19 08:51:37]

Dry run: This issue passed the CQ dry run.

[alancutter (OOO until 2018), 2016-04-20 00:50:09]

lgtm

[suzyh_UTC10 (ex-contributor), 2016-04-20 00:50:45]

The CQ bit was checked by suzyh@chromium.org

[commit-bot: I haz the power, 2016-04-20 00:51:08]

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

[commit-bot: I haz the power, 2016-04-20 00:56:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-22 19:19:16]

Description was changed from

==========
Fix crash in keyframe-effect/getComputedTiming tests

The keyframe-effect/getComputedTiming-{progress,currentIteration}.html
W3C tests caused an assertion failure in animations with a non-zero
value for iterationStart and a duration value of Infinity. The timing
calculations in the spec that we were following need to be reworked
(see https://github.com/w3c/web-animations/issues/142) but in the
meantime this patch adds a workaround for infinite duration animations
that avoids the assertion (caused by trying to calculate inf/inf).

BUG=600248
==========

to

==========
Fix crash in keyframe-effect/getComputedTiming tests

The keyframe-effect/getComputedTiming-{progress,currentIteration}.html
W3C tests caused an assertion failure in animations with a non-zero
value for iterationStart and a duration value of Infinity. The timing
calculations in the spec that we were following need to be reworked
(see https://github.com/w3c/web-animations/issues/142) but in the
meantime this patch adds a workaround for infinite duration animations
that avoids the assertion (caused by trying to calculate inf/inf).

BUG=600248

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

[commit-bot: I haz the power, 2016-04-22 19:19:17]

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