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..f7f6dbf810751be06d765c5cef52386f6b632754 100644
--- a/third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp
+++ b/third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp
@@ -209,10 +209,20 @@ 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()) {
+                // There are three cases here:
+                // - offset 0, used for iteration progress (-inf,0), which should use the first offset 0 value
+                // - offset 1, used for iteration progress [1,inf), which should use the second offset 1 value
+                // - 0 < offset < 1, never used in sampling

[alancutter (OOO until 2018), 2016/05/04 01:32:31]

This comment is a touch close to being a straight copy of the code below but I'm
not going to block landing this on it.

[alancutter (OOO until 2018), 2016/05/04 01:35:32]

Possible suggestion:
if (offset == 0) {
    ASSERT(applyFrom == -inf && applyTo == 0);
    addInterpolation...
} else if (offset == 1) {
    ASSERT(applyFrom == 1 && applyTo == inf);
    addInterpolation...
}
// else the interpolation will never be used in sampling.

[suzyh_UTC10 (ex-contributor), 2016/05/04 01:41:00]

Fair. I was trying to explain why the first two cases use the keyframe-pair
values they do, and why the third case was not present, but I can see it may
have ended up a little too close to copying the code. 
Hmm, yeah, I hadn't thought of using ASSERTs to capture this. Good idea.

But now that I look at it, this makes the definitions of applyFrom and applyTo
unnecessarily repetitive. I'll try rewriting the whole loop and see if I can
make it cleaner.

+                if (keyframes[i]->offset() == 0) {
+                    m_interpolationEffect.addInterpolationsFromKeyframes(entry.key, *keyframes[i], *keyframes[i], applyFrom, applyTo);
+                } else if (keyframes[i]->offset() == 1) {
+                    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 +257,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;
         if (hasSameOffsetAsPreviousNeighbor && hasSameOffsetAsNextNeighbor)
             m_keyframes.remove(i);
     }