Web Animations: Throw TypeErrors for invalid timing parameters

third_party/WebKit/Source/core/animation/TimingInput.cpp

Index: third_party/WebKit/Source/core/animation/TimingInput.cpp
diff --git a/third_party/WebKit/Source/core/animation/TimingInput.cpp b/third_party/WebKit/Source/core/animation/TimingInput.cpp
index 84f837f22892bd8531a5356988543b8128e8c4a6..9408b55765be729831815cc7686f81594674e40b 100644
--- a/third_party/WebKit/Source/core/animation/TimingInput.cpp
+++ b/third_party/WebKit/Source/core/animation/TimingInput.cpp
@@ -41,28 +41,48 @@ void TimingInput::setFillMode(Timing& timing, const String& fillMode)
     }
 }
 
-void TimingInput::setIterationStart(Timing& timing, double iterationStart)
+bool TimingInput::setIterationStart(Timing& timing, double iterationStart, ExceptionState& exceptionState)
 {
-    if (std::isfinite(iterationStart))
-        timing.iterationStart = std::max<double>(iterationStart, 0);
-    else
-        timing.iterationStart = Timing::defaults().iterationStart;
+    ASSERT(std::isfinite(iterationStart));
+    if (std::isnan(iterationStart) || iterationStart < 0) {
+        exceptionState.throwTypeError("iterationStart must be non-negative.");
+        return false;
+    }
+    timing.iterationStart = iterationStart;
+    return true;
 }
 
-void TimingInput::setIterationCount(Timing& timing, double iterationCount)
+bool TimingInput::setIterationCount(Timing& timing, double iterationCount, ExceptionState& exceptionState)
 {
-    if (!std::isnan(iterationCount))
-        timing.iterationCount = std::max<double>(iterationCount, 0);
-    else
-        timing.iterationCount = Timing::defaults().iterationCount;
+    if (std::isnan(iterationCount) || iterationCount < 0) {
+        exceptionState.throwTypeError("iterationCount must be non-negative.");
+        return false;
+    }
+    timing.iterationCount = iterationCount;
+    return true;
 }
 
-void TimingInput::setIterationDuration(Timing& timing, double iterationDuration)
+bool TimingInput::setIterationDuration(Timing& timing, const UnrestrictedDoubleOrString& iterationDuration, ExceptionState& exceptionState)
 {
-    if (!std::isnan(iterationDuration) && iterationDuration >= 0)
-        timing.iterationDuration = iterationDuration / 1000;
-    else
-        timing.iterationDuration = Timing::defaults().iterationDuration;
+    static const char* errorMessage = "duration must be non-negative or auto.";
+
+    if (iterationDuration.isUnrestrictedDouble()) {
+        double durationNumber = iterationDuration.getAsUnrestrictedDouble();
+        if (std::isnan(durationNumber) || durationNumber < 0) {
+            exceptionState.throwTypeError(errorMessage);
+            return false;
+        }
+        timing.iterationDuration = durationNumber / 1000;
+        return true;
+    }
+
+    if (iterationDuration.getAsString() != "auto") {
+        exceptionState.throwTypeError(errorMessage);
+        return false;
+    }
+
+    timing.iterationDuration = -1;

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

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?

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

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.

+    return true;
 }
 
 void TimingInput::setPlaybackRate(Timing& timing, double playbackRate)
@@ -100,14 +120,19 @@ bool TimingInput::convert(const KeyframeEffectOptions& timingInput, Timing& timi
     setStartDelay(timingOutput, timingInput.delay());
     setEndDelay(timingOutput, timingInput.endDelay());
     setFillMode(timingOutput, timingInput.fill());
-    setIterationStart(timingOutput, timingInput.iterationStart());
-    setIterationCount(timingOutput, timingInput.iterations());
-    if (timingInput.duration().isUnrestrictedDouble())
-        setIterationDuration(timingOutput, timingInput.duration().getAsUnrestrictedDouble());
-    else
-        setIterationDuration(timingOutput, -1);
+
+    if (!setIterationStart(timingOutput, timingInput.iterationStart(), exceptionState))
+        return false;
+
+    if (!setIterationCount(timingOutput, timingInput.iterations(), exceptionState))
+        return false;
+
+    if (!setIterationDuration(timingOutput, timingInput.duration(), exceptionState))
+        return false;
+
     setPlaybackRate(timingOutput, timingInput.playbackRate());
     setPlaybackDirection(timingOutput, timingInput.direction());
+
     if (!setTimingFunction(timingOutput, timingInput.easing(), document, exceptionState))
         return false;
 
@@ -116,11 +141,9 @@ bool TimingInput::convert(const KeyframeEffectOptions& timingInput, Timing& timi
     return true;
 }
 
-Timing TimingInput::convert(double duration)
+bool TimingInput::convert(double duration, Timing& timingOutput, ExceptionState& exceptionState)
 {
-    Timing result;
-    setIterationDuration(result, duration);
-    return result;
+    return setIterationDuration(timingOutput, UnrestrictedDoubleOrString::fromUnrestrictedDouble(duration), exceptionState);

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

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.

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

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

 }
 
 } // namespace blink