Web Animations CSS: Snapshot missing start/end Keyframes and remove duplicates

Source/core/css/resolver/StyleResolver.cpp

Index: Source/core/css/resolver/StyleResolver.cpp
diff --git a/Source/core/css/resolver/StyleResolver.cpp b/Source/core/css/resolver/StyleResolver.cpp
index e1e8378cfd96c4f0aa8c644b2a7c210a9d958b60..ba9a457e050045810357906d2a3da43af0e778c5 100644
--- a/Source/core/css/resolver/StyleResolver.cpp
+++ b/Source/core/css/resolver/StyleResolver.cpp
@@ -763,6 +763,11 @@ void StyleResolver::keyframeStylesForAnimation(Element* e, const RenderStyle* el
     }
 }
 
+bool compareOffsets(const RefPtr<Keyframe>& a, const RefPtr<Keyframe>& b)
+{
+    return a->offset() < b->offset();
+}

[Steve Block, 2013/08/06 06:30:20]

We already have this helper in KeyframeAnimationEffect. Maybe we should make it
a static member of Keyframe so it can be shared?

[dstockwell, 2013/08/06 07:14:28]

Done.

+
 void StyleResolver::resolveKeyframes(Element* element, const RenderStyle* style, const StringImpl* name, KeyframeAnimationEffect::KeyframeVector& keyframes)
 {
     const StyleRuleKeyframes* keyframesRule = matchScopedKeyframesRule(element, name);
@@ -774,7 +779,6 @@ void StyleResolver::resolveKeyframes(Element* element, const RenderStyle* style,
     for (unsigned i = 0; i < styleKeyframes.size(); ++i) {
         const StyleKeyframe* styleKeyframe = styleKeyframes[i].get();
         RefPtr<RenderStyle> keyframeStyle = styleForKeyframe(element, style, styleKeyframe);
-
         Vector<float> offsets;
         styleKeyframe->getKeys(offsets);
         for (size_t j = 0; j < offsets.size(); ++j) {
@@ -784,15 +788,59 @@ void StyleResolver::resolveKeyframes(Element* element, const RenderStyle* style,
             // FIXME: AnimatableValues should be shared between the keyframes at different offsets.
             for (unsigned k = 0; k < properties->propertyCount(); k++) {
                 CSSPropertyID property = properties->propertyAt(k).id();
-                // FIXME: CSSValue needs to be resolved.
                 keyframe->setPropertyValue(property, CSSAnimatableValueFactory::create(property, keyframeStyle.get()).get());
             }
             keyframes.append(keyframe);
         }
     }
 
-    // FIXME: If the 0% keyframe is missing, create it (but only if there is at least one other keyframe)
-    // FIXME: If the 100% keyframe is missing, create it (but only if there is at least one other keyframe)
+    if (keyframes.isEmpty())
+        return;
+
+    // Remove duplicate keyframes. In CSS the last keyframe at a given offset takes priority.
+    std::stable_sort(keyframes.begin(), keyframes.end(), compareOffsets);
+    unsigned duplicates = 0;

[Timothy Loh, 2013/08/05 03:44:59]

I think this should work:
    keyframes.shrink(std::unique(keyframes.begin(), keyframes.end()) -
keyframes.begin());

[dstockwell, 2013/08/05 05:52:18]

Tim and I discussed off thread, and while something like this should work (keep
the last as opposed to the first duplicate):

keyframes.remove(0, keyframes.rend() - std::unique(keyframes.rbegin(),
keyframes.rend(), equalOffsets))

It's not clear if it's a win, and still appears to be the first use of
std::unique in blink??

[Steve Block, 2013/08/06 06:30:20]

Does unique() work with reverse iterators?

+    for (size_t i = 1; i < keyframes.size(); i++) {
+        if (keyframes[i]->offset() == keyframes[i - 1 - duplicates]->offset()) {
+            keyframes[i - 1 - duplicates] = keyframes[i];
+            ++duplicates;
+        } else if (duplicates) {
+            keyframes[i - duplicates] = keyframes[i];

[Steve Block, 2013/08/06 06:30:20]

I think this would be easier to follow if you used a local 'size_t targetIndex'
or 'iterator targetSlot' to represent the slot in the array that the current
keyframe will be moved to.

[dstockwell, 2013/08/06 07:14:28]

Done.

+        }
+    }
+    keyframes.shrink(keyframes.size() - duplicates);
+
+    bool missingStartKeyframe = !keyframes[0]->offset;

[Steve Block, 2013/08/06 06:30:20]

I think you have this inverted.

Also, this local isn't a keyframe, so should probably be isStartKeyframeMissing

[dstockwell, 2013/08/06 07:14:28]

Fixed. Be nice if we could write != 0 :)
Done.

+    bool missingEndKeyframe = keyframes[keyframes.size() - 1]->offset != 1;
+    if (!missingStartKeyframe && !missingEndKeyframe)
+        return;
+
+    HashSet<CSSPropertyID> allProperties;
+    for (unsigned i = 0; i < styleKeyframes.size(); ++i) {

[Steve Block, 2013/08/06 06:30:20]

I think you should use size_t in loops like this, as that what Vector::size()
and Vector::operator[] use.

[dstockwell, 2013/08/06 07:14:28]

Done.

+        const StyleKeyframe* styleKeyframe = styleKeyframes[i].get();
+        Vector<float> offsets;
+        styleKeyframe->getKeys(offsets);
+        const StylePropertySet* properties = styleKeyframe->properties();
+        for (unsigned j = 0; j < properties->propertyCount(); ++j) {
+            allProperties.add(properties->propertyAt(j).id());
+        }
+    }
+
+    if (missingStartKeyframe) {
+        RefPtr<Keyframe> keyframe = Keyframe::create();
+        keyframe->setOffset(0);
+        for (HashSet<CSSPropertyID>::const_iterator iter = allProperties.begin(); iter != allProperties.end(); ++iter)
+            keyframe->setPropertyValue(*iter, CSSAnimatableValueFactory::create(*iter, style).get());
+        keyframes.prepend(keyframe);
+    }
+
+    if (missingEndKeyframe) {
+        RefPtr<Keyframe> keyframe = Keyframe::create();
+        keyframe->setOffset(1);
+        for (HashSet<CSSPropertyID>::const_iterator iter = allProperties.begin(); iter != allProperties.end(); ++iter)
+            keyframe->setPropertyValue(*iter, CSSAnimatableValueFactory::create(*iter, style).get());
+        keyframes.append(keyframe);
+    }
 }
 
 const StylePropertySet* StyleResolver::firstKeyframeStyles(const Element* element, const StringImpl* animationName)