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()) {
+                // 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);
+            }
         }
     }
 
     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;

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

Much nicer!

         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