Fix handling of multiple keyframes at same offset

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

 /*
  * Copyright (C) 2013 Google 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:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 191 matching lines @@
 void KeyframeEffectModelBase::ensureInterpolationEffectPopulated() const
 {
     if (m_interpolationEffect.isPopulated())
         return;
 
     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)
-                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);
+            }
         }
     }
 
     m_interpolationEffect.setPopulated();
 }
 
 bool KeyframeEffectModelBase::isReplaceOnly()
 {
     ensureKeyframeGroups();
     for (const auto& entry : *m_keyframeGroups) {
@@ skipping 14 matching lines @@
 }
 
 void KeyframeEffectModelBase::PropertySpecificKeyframeGroup::appendKeyframe(PassRefPtr<Keyframe::PropertySpecificKeyframe> keyframe)
 {
     ASSERT(m_keyframes.isEmpty() || m_keyframes.last()->offset() <= keyframe->offset());
     m_keyframes.append(keyframe);
 }
 
 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);
     }
     ASSERT(m_keyframes.size() >= 2);
 }
 
 bool KeyframeEffectModelBase::PropertySpecificKeyframeGroup::addSyntheticKeyframeIfRequired(PassRefPtr<TimingFunction> zeroOffsetEasing)
 {
     ASSERT(!m_keyframes.isEmpty());
 
     bool addedSyntheticKeyframe = false;
 
     if (m_keyframes.first()->offset() != 0.0) {
         m_keyframes.insert(0, m_keyframes.first()->neutralKeyframe(0, zeroOffsetEasing));
         addedSyntheticKeyframe = true;
     }
     if (m_keyframes.last()->offset() != 1.0) {
         appendKeyframe(m_keyframes.last()->neutralKeyframe(1, nullptr));
         addedSyntheticKeyframe = true;
     }
 
     return addedSyntheticKeyframe;
 }
 
 } // namespace blink