Web Animations: Throw TypeErrors for invalid timing parameters

Web Animations: Throw TypeErrors for invalid timing parameters

This change throws TypeErrors when invalid parameters are given
for the following AnimationEffectTiming parameters:
 - iterationStart
 - iterations
 - duration

See spec for valid inputs:
https://w3c.github.io/web-animations/#animationeffecttiming

BUG=605450
Committed: https://crrev.com/f760e7025c61bd64c3a6bdcf22b1af294ebcad90
Cr-Commit-Position: refs/heads/master@{#390040}

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

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

[suzyh_UTC10 (ex-contributor), 2016-04-20 21:54:12]

A couple of initial comments.

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

https://codereview.chromium.org/1906463002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/TimingInput.cpp:84:
timing.iterationDuration = -1;
The existing TimingInput::setIterationDuration suggests that the value here
should be Timing::defaults().iterationDuration, which is quiet_NaN, rather than
-1. This is also the value up in the existing
AnimationEffectTiming::setDuration. The existing TimingInput::convert below,
though, uses -1.
http://w3c.github.io/web-animations/#dom-animationeffecttimingreadonly-duration
says "auto" should translate to 0.

Can you clarify what is supposed to happen here, and add a test if there isn't
already one in imported/web-platform-tests that covers this?

https://codereview.chromium.org/1906463002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/TimingInput.cpp:146: return
setIterationDuration(timingOutput,
UnrestrictedDoubleOrString::fromUnrestrictedDouble(duration), exceptionState);
What should the behaviour be if the caller passes in a timingOutput that has
values filled in? (The callers don't currently do this, but it could happen in
future.) Should this function clear all other fields of timingOutput? Should it
have an ASSERT that those fields are not set? A comment on the function
declaration might also be appropriate.

https://codereview.chromium.org/1906463002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp
(right):

https://codereview.chromium.org/1906463002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp:336:
NonThrowableExceptionState exceptionState;
I don't know the implications of this, so best to run it by samli if you haven't
already.

[alancutter (OOO until 2018), 2016-04-21 08:32:36]

Description was changed from

==========
timingInputTypeErrors WIP

BUG=
==========

to

==========
Web Animations: Throw TypeErrors for invalid timing parameters

This change throws TypeErrors when invalid parameters are given
for the following AnimationEffectTiming parameters:
 - iterationStart
 - iterations
 - duration

See spec for valid inputs:
https://w3c.github.io/web-animations/#animationeffecttiming

BUG=605450
==========

[alancutter (OOO until 2018), 2016-04-21 08:32:36]

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

[alancutter (OOO until 2018), 2016-04-21 08:33:07]

Updated tests.

+timloh for minor changes to core/style.

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

https://codereview.chromium.org/1906463002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/TimingInput.cpp:84:
timing.iterationDuration = -1;
On 2016/04/20 at 21:54:12, suzyh wrote:
> The existing TimingInput::setIterationDuration suggests that the value here
should be Timing::defaults().iterationDuration, which is quiet_NaN, rather than
-1. This is also the value up in the existing
AnimationEffectTiming::setDuration. The existing TimingInput::convert below,
though, uses -1.
http://w3c.github.io/web-animations/#dom-animationeffecttimingreadonly-duration
says "auto" should translate to 0.
> 
> Can you clarify what is supposed to happen here, and add a test if there isn't
already one in imported/web-platform-tests that covers this?

auto represents the intrinsic duration which for Web Animations level 1 is
always 0s. For groups the intrinsic duration depends on the timing parameters of
its children.

I failed to read the original code correctly here. It was deliberately using -1
as an invalid value to set NaN. Changed patch to match original logic without
using -1.
My original code fails the timingOutput.assertValid() check below.

https://codereview.chromium.org/1906463002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/TimingInput.cpp:146: return
setIterationDuration(timingOutput,
UnrestrictedDoubleOrString::fromUnrestrictedDouble(duration), exceptionState);
On 2016/04/20 at 21:54:11, suzyh wrote:
> What should the behaviour be if the caller passes in a timingOutput that has
values filled in? (The callers don't currently do this, but it could happen in
future.) Should this function clear all other fields of timingOutput? Should it
have an ASSERT that those fields are not set? A comment on the function
declaration might also be appropriate.

Good consideration, if only one could conveniently return multiple values in
this language.
Added assert.

[alancutter (OOO until 2018), 2016-04-21 08:33:11]

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

[commit-bot: I haz the power, 2016-04-21 08:33:30]

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

[commit-bot: I haz the power, 2016-04-21 09:32:35]

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

[commit-bot: I haz the power, 2016-04-21 09:32:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_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-22 00:32:37]

lgtm

[Timothy Loh, 2016-04-22 00:36:56]

lgtm for core/(not animation)

[alancutter (OOO until 2018), 2016-04-26 02:08:13]

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

[alancutter (OOO until 2018), 2016-04-26 02:08:13]

+haraken for additional change to core/html/HTMLMarqueeElement.js to make
fast/inline/inline-marquee-crash.html pass.

Updated imported web-animations tests with passing expectations.

[alancutter (OOO until 2018), 2016-04-26 02:08:35]

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

[commit-bot: I haz the power, 2016-04-26 02:08:57]

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

[haraken, 2016-04-26 03:24:22]

HTMLMarqueeElement.js LGTM

[commit-bot: I haz the power, 2016-04-26 03:30:43]

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

[commit-bot: I haz the power, 2016-04-26 03:30:44]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[alancutter (OOO until 2018), 2016-04-26 07:15:38]

Updated unit tests, suzyh PTAL.

[suzyh_UTC10 (ex-contributor), 2016-04-27 00:08:54]

lgtm

yay for passing more web-platform-tests!

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

The CQ bit was checked by alancutter@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1906463002/#ps60001
(title: "Fix unit tests")

[commit-bot: I haz the power, 2016-04-27 01:09:04]

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

[commit-bot: I haz the power, 2016-04-27 01:14:58]

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

[commit-bot: I haz the power, 2016-04-27 01:14:59]

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

[alancutter (OOO until 2018), 2016-04-27 10:01:17]

The CQ bit was checked by alancutter@chromium.org

[commit-bot: I haz the power, 2016-04-27 10:01:40]

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

[commit-bot: I haz the power, 2016-04-27 10:25:23]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-27 10:27:13]

Description was changed from

==========
Web Animations: Throw TypeErrors for invalid timing parameters

This change throws TypeErrors when invalid parameters are given
for the following AnimationEffectTiming parameters:
 - iterationStart
 - iterations
 - duration

See spec for valid inputs:
https://w3c.github.io/web-animations/#animationeffecttiming

BUG=605450
==========

to

==========
Web Animations: Throw TypeErrors for invalid timing parameters

This change throws TypeErrors when invalid parameters are given
for the following AnimationEffectTiming parameters:
 - iterationStart
 - iterations
 - duration

See spec for valid inputs:
https://w3c.github.io/web-animations/#animationeffecttiming

BUG=605450

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

[commit-bot: I haz the power, 2016-04-27 10:27:14]

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