Use NaN/Inf to represent unresolved/indefinite SMILTimes

Source/core/svg/animation/SVGSMILElement.cpp

Index: Source/core/svg/animation/SVGSMILElement.cpp
diff --git a/Source/core/svg/animation/SVGSMILElement.cpp b/Source/core/svg/animation/SVGSMILElement.cpp
index aaa39d929f2665b29e14044bda8185c54b06e170..9f7aa230d04542fe3415eb1eca898ed0dadca48e 100644
--- a/Source/core/svg/animation/SVGSMILElement.cpp
+++ b/Source/core/svg/animation/SVGSMILElement.cpp
@@ -491,15 +491,17 @@ void SVGSMILElement::parseBeginOrEnd(const String& parseString, BeginOrEnd begin
     if (beginOrEnd == End)
         m_hasEndEventConditions = false;
     HashSet<double> existing;
-    for (unsigned n = 0; n < timeList.size(); ++n)
-        existing.add(timeList[n].time().value());
+    for (unsigned n = 0; n < timeList.size(); ++n) {
+        if (timeList[n].time().isFinite())

[fs, 2014/08/19 21:49:43]

Could this be isUnresolved instead? I.e. I don't see why having Inf in the
HashSet would be a problem (I can imaging that NaN would be a problem). I also
think that 'indefinite' could appear here while 'unresolved' would be less
interesting if it did so could be dropped without harm.

[reni, 2014/08/21 14:20:06]

Done.

+            existing.add(timeList[n].time().value());
+    }
     Vector<String> splitString;
     parseString.split(';', splitString);
     for (unsigned n = 0; n < splitString.size(); ++n) {
         SMILTime value = parseClockValue(splitString[n]);
         if (value.isUnresolved())
             parseCondition(splitString[n], beginOrEnd);
-        else if (!existing.contains(value.value()))
+        else if (value.isFinite() && !existing.contains(value.value()))

[fs, 2014/08/19 21:49:44]

I think 'indefinite' is [sometimes] valid here, so hopefully doing as prescribed
above works.

[reni, 2014/08/21 14:20:06]

HashSet can really contain Inf value without consequences however looking for an
Inf key in it causes an assertion failure in
HashTableKeyChecker<...>::checkKey(key).

[fs, 2014/08/21 15:30:39]

Ok. I noticed:

    static T emptyValue() { return std::numeric_limits<T>::infinity(); }

(and -Inf is deleted-value)

I guess that means that indefinite/Inf needs to be special cased =/ (short of
using a tweaked HashSet).

[reni, 2014/08/21 20:52:31]

I agree with =/, but done :)

             timeList.append(SMILTimeWithOrigin(value, SMILTimeWithOrigin::ParserOrigin));
     }
     sortTimeList(timeList);
@@ -801,12 +803,14 @@ SMILTime SVGSMILElement::minValue() const
 
 SMILTime SVGSMILElement::simpleDuration() const
 {
-    return std::min(dur(), SMILTime::indefinite());
+    SMILTime duration = dur();
+    return duration.isUnresolved() ? SMILTime::indefinite() : duration;

[fs, 2014/08/19 21:49:44]

I think this could've been left as min(), but maybe better to play it on the
safe side [1] =).

Another possibility would be to implement a min() with (guaranteed)
SMIL-semantics [2].

[1] I.e. hope that the impl. is as prescribed by the spec - !(b < a) ? a : b
[2] I believe the main difference is that MIN(indefinite, unresolved) should
return indefinite. std::min() should do that too with just the plain values FWIW
(but it will return unresolved if the argument order is reversed...)

[reni, 2014/08/21 14:20:06]

Done.

 }
 
 void SVGSMILElement::addBeginTime(SMILTime eventTime, SMILTime beginTime, SMILTimeWithOrigin::Origin origin)
 {
-    ASSERT(!std::isnan(beginTime.value()));
+    if (!beginTime.isFinite())

[fs, 2014/08/19 21:49:43]

Could we drop this (and the below) back to asserts? It generally seems better
for the caller to filter out values.

[reni, 2014/08/21 14:20:06]

I set them back to asserts. However, are we sure that these asserts are needed
at all if the caller filtered out the unresolved values already?

[fs, 2014/08/21 15:30:39]

If nothing else, they serve to communicate the preconditions of these methods.
I'm not quite sure what the original assert was for, because there would've been
asserts in the constructor first. If you want to drop the asserts, feel free.

[reni, 2014/08/21 20:52:31]

They are dropped.

+        return;
     m_beginTimes.append(SMILTimeWithOrigin(beginTime, origin));
     sortTimeList(m_beginTimes);
     beginListChanged(eventTime);
@@ -814,7 +818,8 @@ void SVGSMILElement::addBeginTime(SMILTime eventTime, SMILTime beginTime, SMILTi
 
 void SVGSMILElement::addEndTime(SMILTime eventTime, SMILTime endTime, SMILTimeWithOrigin::Origin origin)
 {
-    ASSERT(!std::isnan(endTime.value()));
+    if (!endTime.isFinite())
+        return;
     m_endTimes.append(SMILTimeWithOrigin(endTime, origin));
     sortTimeList(m_endTimes);
     endListChanged(eventTime);
@@ -873,6 +878,8 @@ SMILTime SVGSMILElement::repeatingDuration() const
     if (!simpleDuration || (repeatDur.isUnresolved() && repeatCount.isUnresolved()))
         return simpleDuration;
     SMILTime repeatCountDuration = simpleDuration * repeatCount;
+    if (repeatCountDuration.isUnresolved())
+        return std::min(repeatDur, SMILTime::indefinite());

[fs, 2014/08/19 21:49:44]

This would probably look more obvious if the min(repeatDur, indefinite) was
computed first?

SMILTime a = min(repeatDur, ...);
SMILTime repeatCountDuration = ...;
if (!...)
  a = min(a, repeatCountDuration);
return a;

?

[reni, 2014/08/21 14:20:07]

Done.

     return std::min(repeatCountDuration, std::min(repeatDur, SMILTime::indefinite()));
 }
 
@@ -896,7 +903,7 @@ SMILTime SVGSMILElement::resolveActiveEnd(SMILTime resolvedBegin, SMILTime resol
         minValue = 0;
         maxValue = SMILTime::indefinite();
     }
-    return resolvedBegin + std::min(maxValue, std::max(minValue, preliminaryActiveDuration));
+    return resolvedBegin + (preliminaryActiveDuration.isUnresolved() ? preliminaryActiveDuration : std::max(minValue, preliminaryActiveDuration));

[fs, 2014/08/19 21:49:44]

Looks like maxValue disappeared? Also, if |preliminaryActiveDuration| is
unresolved then the result is undefined too, so that could be filtered earlier
(after it's set)?

[reni, 2014/08/21 14:20:07]

Done.

 }
 
 SMILInterval SVGSMILElement::resolveInterval(ResolveInterval resolveIntervalType) const
@@ -940,7 +947,7 @@ void SVGSMILElement::resolveFirstInterval()
     if (!firstInterval.begin.isUnresolved() && firstInterval != m_interval) {
         m_interval = firstInterval;
         notifyDependentsIntervalChanged();
-        m_nextProgressTime = std::min(m_nextProgressTime, m_interval.begin);
+        m_nextProgressTime = m_nextProgressTime.isUnresolved() ? m_interval.begin : std::min(m_nextProgressTime, m_interval.begin);
 
         if (m_timeContainer)
             m_timeContainer->notifyIntervalsChanged();
@@ -955,7 +962,7 @@ bool SVGSMILElement::resolveNextInterval()
     if (!nextInterval.begin.isUnresolved() && nextInterval.begin != m_interval.begin) {
         m_interval = nextInterval;
         notifyDependentsIntervalChanged();
-        m_nextProgressTime = std::min(m_nextProgressTime, m_interval.begin);
+        m_nextProgressTime = m_nextProgressTime.isUnresolved() ? m_interval.begin : std::min(m_nextProgressTime, m_interval.begin);
         return true;
     }