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..3c9c790a20aef469b54e205db864e6b52fc1f77f 100644
--- a/third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp
+++ b/third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp
@@ -207,12 +207,37 @@ void KeyframeEffectModelBase::ensureInterpolationEffectPopulated() const
     for (const auto& entry : *m_keyframeGroups) {
         const PropertySpecificKeyframeVector& keyframes = entry.value->keyframes();
         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)
+            size_t startKeyframe = i;

[alancutter (OOO until 2018), 2016/05/04 07:00:03]

I'd call these startIndex and endIndex to avoid confusing them with Keyframes.

[suzyh_UTC10 (ex-contributor), 2016/05/04 07:15:32]

Done.

+            size_t endKeyframe = i+1;
+            double startOffset = keyframes[startKeyframe]->offset();
+            double endOffset = keyframes[endKeyframe]->offset();
+            double applyFrom = startOffset;
+            double applyTo = endOffset;
+
+            if (i == 0) {
+                applyFrom = -std::numeric_limits<double>::infinity();
+                ASSERT(startOffset == 0.0);
+                if (endOffset == 0.0) {
+                    ASSERT(endKeyframe + 1 < keyframes.size()

[alancutter (OOO until 2018), 2016/05/04 07:00:03]

No need to check this, WTF::Vector indexing comes with bounds checking.

[suzyh_UTC10 (ex-contributor), 2016/05/04 07:15:32]

Done.

+                        && keyframes[endKeyframe + 1]->offset() != 0.0);
+                    endKeyframe = startKeyframe;
+                }
+            }
+            if (i == keyframes.size() - 2) {
                 applyTo = std::numeric_limits<double>::infinity();
-
-            m_interpolationEffect.addInterpolationsFromKeyframes(entry.key, *keyframes[i], *keyframes[i + 1], applyFrom, applyTo);
+                ASSERT(endOffset == 1.0);
+                if (startOffset == 1.0) {
+                    ASSERT(startKeyframe > 1

[alancutter (OOO until 2018), 2016/05/04 07:00:03]

Won't this fail with [{}, {offset: 1}, {}]?
I don't think this check is necessary with bounds checking.

[suzyh_UTC10 (ex-contributor), 2016/05/04 07:15:32]

Ah, typo, that should have been startKeyframe >= 1 (or > 0) but at any rate it
is now obsolete.

+                        && keyframes[startKeyframe - 1]->offset() != 1.0);
+                    startKeyframe = endKeyframe;
+                }
+            }
+
+            if (applyFrom != applyTo) {
+                m_interpolationEffect.addInterpolationsFromKeyframes(
+                    entry.key, *keyframes[startKeyframe], *keyframes[endKeyframe], applyFrom, applyTo);
+            }
+            // else the interpolation will never be used in sampling
         }
     }
 
@@ -247,17 +272,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);
     }