Fix handling of multiple keyframes at same offset

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

Index: third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp
diff --git a/third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp b/third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp
index bfd271fce467035d3f85a3fe4a16e7c311e6e706..ce41f63100caaa4520853017309ba660ca087f46 100644
--- a/third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp
+++ b/third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp
@@ -209,10 +209,18 @@ void KeyframeEffectModelBase::ensureInterpolationEffectPopulated() const
         for (size_t i = 0; i < keyframes.size() - 1; i++) {
             double applyFrom = i ? keyframes[i]->offset() : (-std::numeric_limits<double>::infinity());
             double applyTo = i == keyframes.size() - 2 ? std::numeric_limits<double>::infinity() : keyframes[i + 1]->offset();
-            if (applyTo == 1)
-                applyTo = std::numeric_limits<double>::infinity();
 
-            m_interpolationEffect.addInterpolationsFromKeyframes(entry.key, *keyframes[i], *keyframes[i + 1], applyFrom, applyTo);
+            if (keyframes[i]->offset() == keyframes[i + 1]->offset()) {
+                // The interpolation we add should use the same keyframe value on both ends
+                if (keyframes[i]->offset() == 0) {
+                    // This keyframe pair is used for iteration progress < 0, at which point we want to use the start value for both
+                    m_interpolationEffect.addInterpolationsFromKeyframes(entry.key, *keyframes[i], *keyframes[i], applyFrom, applyTo);
+                } else {

[alancutter (OOO until 2018), 2016/05/02 07:35:35]

Can this be "else if (offset == 1)"?

[suzyh_UTC10 (ex-contributor), 2016/05/02 08:34:10]

Hmm... I think that at this point, offset can still be something other than 0
and 1. My reading of the spec was that only offset 0 is special, so concluded
that other offsets should behave like 1 in this case.

But now that you mention it, maybe for equal offsets other than 0 and 1 we
should just not add an extra interpolation, because it never applies.

Can you step through an example on paper, working off the spec, and see what you
think the appropriate behaviour is? Then we can compare notes and see where to
go from there.

+                    m_interpolationEffect.addInterpolationsFromKeyframes(entry.key, *keyframes[i + 1], *keyframes[i + 1], applyFrom, applyTo);
+                }
+            } else {
+                m_interpolationEffect.addInterpolationsFromKeyframes(entry.key, *keyframes[i], *keyframes[i + 1], applyFrom, applyTo);
+            }
         }
     }
 
@@ -247,17 +255,15 @@ void KeyframeEffectModelBase::PropertySpecificKeyframeGroup::appendKeyframe(Pass
 
 void KeyframeEffectModelBase::PropertySpecificKeyframeGroup::removeRedundantKeyframes()
 {
-    // As an optimization, removes keyframes in the following categories, as
-    // they will never be used by sample().
-    // - End keyframes with the same offset as their neighbor
-    // - Interior keyframes with the same offset as both their neighbors
+    // As an optimization, removes interior keyframes that have the same offset
+    // as both their neighbours, as they will never be used by sample().
     // Note that synthetic keyframes must be added before this method is
     // called.
     ASSERT(m_keyframes.size() >= 2);
-    for (int i = m_keyframes.size() - 1; i >= 0; --i) {
+    for (int i = m_keyframes.size() - 2; i > 0; --i) {
         double offset = m_keyframes[i]->offset();
-        bool hasSameOffsetAsPreviousNeighbor = !i || m_keyframes[i - 1]->offset() == offset;
-        bool hasSameOffsetAsNextNeighbor = i == static_cast<int>(m_keyframes.size() - 1) || m_keyframes[i + 1]->offset() == offset;
+        bool hasSameOffsetAsPreviousNeighbor = m_keyframes[i - 1]->offset() == offset;
+        bool hasSameOffsetAsNextNeighbor = m_keyframes[i + 1]->offset() == offset;

[alancutter (OOO until 2018), 2016/05/02 07:35:35]

Much nicer!

         if (hasSameOffsetAsPreviousNeighbor && hasSameOffsetAsNextNeighbor)
             m_keyframes.remove(i);
     }