Use NaN/Inf to represent unresolved/indefinite SMILTimes

Source/core/svg/animation/SVGSMILElement.cpp

 /*
  * Copyright (C) 2008 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 473 matching lines @@
 
     return true;
 }
 
 void SVGSMILElement::parseBeginOrEnd(const String& parseString, BeginOrEnd beginOrEnd)
 {
     Vector<SMILTimeWithOrigin>& timeList = beginOrEnd == Begin ? m_beginTimes : m_endTimes;
     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);
 }
 
 bool SVGSMILElement::isSupportedAttribute(const QualifiedName& attrName)
 {
     DEFINE_STATIC_LOCAL(HashSet<QualifiedName>, supportedAttributes, ());
     if (supportedAttributes.isEmpty()) {
         SVGTests::addSupportedAttributes(supportedAttributes);
@@ skipping 281 matching lines @@
 {
     if (m_cachedMin != invalidCachedTime)
         return m_cachedMin;
     const AtomicString& value = fastGetAttribute(SVGNames::minAttr);
     SMILTime result = parseClockValue(value);
     return m_cachedMin = (result.isUnresolved() || result < 0) ? 0 : result;
 }
 
 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);
 }
 
 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);
 }
 
 inline bool compareTimes(const SMILTimeWithOrigin& left, const SMILTimeWithOrigin& right)
 {
     return left.time() < right.time();
 }
 
@@ skipping 38 matching lines @@
 SMILTime SVGSMILElement::repeatingDuration() const
 {
     // Computing the active duration
     // http://www.w3.org/TR/SMIL2/smil-timing.html#Timing-ComputingActiveDur
     SMILTime repeatCount = this->repeatCount();
     SMILTime repeatDur = this->repeatDur();
     SMILTime simpleDuration = this->simpleDuration();
     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()));
 }
 
 SMILTime SVGSMILElement::resolveActiveEnd(SMILTime resolvedBegin, SMILTime resolvedEnd) const
 {
     // Computing the active duration
     // http://www.w3.org/TR/SMIL2/smil-timing.html#Timing-ComputingActiveDur
     SMILTime preliminaryActiveDuration;
     if (!resolvedEnd.isUnresolved() && dur().isUnresolved() && repeatDur().isUnresolved() && repeatCount().isUnresolved())
         preliminaryActiveDuration = resolvedEnd - resolvedBegin;
     else if (!resolvedEnd.isFinite())
         preliminaryActiveDuration = repeatingDuration();
     else
         preliminaryActiveDuration = std::min(repeatingDuration(), resolvedEnd - resolvedBegin);
 
     SMILTime minValue = this->minValue();
     SMILTime maxValue = this->maxValue();
     if (minValue > maxValue) {
         // Ignore both.
         // http://www.w3.org/TR/2001/REC-smil-animation-20010904/#MinMax
         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
 {
     bool first = resolveIntervalType == FirstInterval;
     // See the pseudocode in http://www.w3.org/TR/SMIL3/smil-timing.html#q90.
     SMILTime beginAfter = first ? -std::numeric_limits<double>::infinity() : m_interval.end;
     SMILTime lastIntervalTempEnd = std::numeric_limits<double>::infinity();
     while (true) {
         bool equalsMinimumOK = !first || m_interval.end > m_interval.begin;
@@ skipping 23 matching lines @@
 }
 
 void SVGSMILElement::resolveFirstInterval()
 {
     SMILInterval firstInterval = resolveInterval(FirstInterval);
     ASSERT(!firstInterval.begin.isIndefinite());
 
     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();
     }
 }
 
 bool SVGSMILElement::resolveNextInterval()
 {
     SMILInterval nextInterval = resolveInterval(NextInterval);
     ASSERT(!nextInterval.begin.isIndefinite());
 
     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;
     }
 
     return false;
 }
 
 SMILTime SVGSMILElement::nextProgressTime() const
 {
     return m_nextProgressTime;
 }
@@ skipping 383 matching lines @@
 #if ENABLE(OILPAN)
     visitor->trace(m_targetElement);
     visitor->trace(m_timeContainer);
     visitor->trace(m_conditions);
     visitor->trace(m_syncBaseDependents);
 #endif
     SVGElement::trace(visitor);
 }
 
 }