Web Animations: Throw TypeErrors for invalid timing parameters

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

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/animation/TimingInput.h"
 
 #include "bindings/core/v8/ExceptionState.h"
 #include "core/animation/AnimationInputHelpers.h"
 #include "core/animation/KeyframeEffectOptions.h"
 
@@ skipping 23 matching lines @@
         timing.fillMode = Timing::FillModeBackwards;
     } else if (fillMode == "both") {
         timing.fillMode = Timing::FillModeBoth;
     } else if (fillMode == "forwards") {
         timing.fillMode = Timing::FillModeForwards;
     } else {
         timing.fillMode = Timing::defaults().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)
 {
     if (std::isfinite(playbackRate))
         timing.playbackRate = playbackRate;
     else
         timing.playbackRate = Timing::defaults().playbackRate;
 }
 
@@ skipping 17 matching lines @@
         return true;
     }
     return false;
 }
 
 bool TimingInput::convert(const KeyframeEffectOptions& timingInput, Timing& timingOutput, Document* document, ExceptionState& exceptionState)
 {
     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;
 
     timingOutput.assertValid();
 
     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