Web Animations: Refactor KeyframeEffectModel to work via an InterpolationEffect.

Web Animations: Refactor KeyframeEffectModel to work via an InterpolationEffect.

This patch modifies KeyframeEffectModel's implementation such that
an InterpolationEffect is generated after Keyframe normalization. The
InterpolationEffect then handles sample generation, and Interpolations are
plumbed down through the style resolution pipeline instead of
CompositableValues.

BUG=346495
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169500

[alancutter (OOO until 2018), 2014-03-12 03:34:06]

It's not entirely clear from the diff whether
AnimationEffect::CompositableValues are dead code or not after this change.
If they are completely unused could they be removed as well? It helps keep
things tidy and helps to understand exactly what is being replaced.

https://codereview.chromium.org/194673002/diff/1/Source/core/animation/Animat...
File Source/core/animation/AnimationEffect.h (right):

https://codereview.chromium.org/194673002/diff/1/Source/core/animation/Animat...
Source/core/animation/AnimationEffect.h:65: typedef
Vector<std::pair<CSSPropertyID, RefPtr<CompositableValue> > >
CompositableValueList;
These two typedefs can be removed.

https://codereview.chromium.org/194673002/diff/1/Source/core/animation/css/CS...
File Source/core/animation/css/CSSAnimations.h (right):

https://codereview.chromium.org/194673002/diff/1/Source/core/animation/css/CS...
Source/core/animation/css/CSSAnimations.h:54: // This includes updates to
animations/transitions as well as the CompositableValueMaps to be applied.
Comment needs updating, CompositableValueMaps are not used anymore.

[shans, 2014-03-14 06:19:58]

Plz can has substantive review nao?

https://codereview.chromium.org/194673002/diff/1/Source/core/animation/Animat...
File Source/core/animation/AnimationEffect.h (right):

https://codereview.chromium.org/194673002/diff/1/Source/core/animation/Animat...
Source/core/animation/AnimationEffect.h:65: typedef
Vector<std::pair<CSSPropertyID, RefPtr<CompositableValue> > >
CompositableValueList;
On 2014/03/12 03:34:07, alancutter wrote:
> These two typedefs can be removed.

Done.

https://codereview.chromium.org/194673002/diff/1/Source/core/animation/css/CS...
File Source/core/animation/css/CSSAnimations.h (right):

https://codereview.chromium.org/194673002/diff/1/Source/core/animation/css/CS...
Source/core/animation/css/CSSAnimations.h:54: // This includes updates to
animations/transitions as well as the CompositableValueMaps to be applied.
On 2014/03/12 03:34:07, alancutter wrote:
> Comment needs updating, CompositableValueMaps are not used anymore.

Done.

[alancutter (OOO until 2018), 2014-03-14 07:26:47]

lgtm!

[dstockwell, 2014-03-17 02:05:57]

lgtm

https://codereview.chromium.org/194673002/diff/40001/Source/core/animation/An...
File Source/core/animation/Animation.h (right):

https://codereview.chromium.org/194673002/diff/40001/Source/core/animation/An...
Source/core/animation/Animation.h:76: ASSERT(m_activeInterpolations);
If it can't be null, return a reference?

https://codereview.chromium.org/194673002/diff/40001/Source/core/animation/Ke...
File Source/core/animation/KeyframeEffectModel.cpp (right):

https://codereview.chromium.org/194673002/diff/40001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.cpp:232: end =
keyframes[i+1]->value();
spaces around +,
and below

[shans, 2014-03-18 20:09:27]

https://codereview.chromium.org/194673002/diff/40001/Source/core/animation/An...
File Source/core/animation/Animation.h (right):

https://codereview.chromium.org/194673002/diff/40001/Source/core/animation/An...
Source/core/animation/Animation.h:76: ASSERT(m_activeInterpolations);
On 2014/03/17 02:05:57, dstockwell wrote:
> If it can't be null, return a reference?

Done.

https://codereview.chromium.org/194673002/diff/40001/Source/core/animation/Ke...
File Source/core/animation/KeyframeEffectModel.cpp (right):

https://codereview.chromium.org/194673002/diff/40001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.cpp:232: end =
keyframes[i+1]->value();
On 2014/03/17 02:05:57, dstockwell wrote:
> spaces around +,
> and below

Done.

[shans, 2014-03-18 20:09:32]

The CQ bit was checked by shans@chromium.org

[commit-bot: I haz the power, 2014-03-18 20:09:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shans@chromium.org/194673002/60001

[commit-bot: I haz the power, 2014-03-18 20:10:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-18 20:10:29]

Failed to apply patch for Source/core/animation/AnimationStack.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/animation/AnimationStack.cpp
  Hunk #3 FAILED at 67.
  Hunk #4 succeeded at 79 with fuzz 2.
  1 out of 4 hunks FAILED -- saving rejects to file
Source/core/animation/AnimationStack.cpp.rej

Patch:       Source/core/animation/AnimationStack.cpp
Index: Source/core/animation/AnimationStack.cpp
diff --git a/Source/core/animation/AnimationStack.cpp
b/Source/core/animation/AnimationStack.cpp
index
601e7e598e4d4ccb17ba4d8e76841adbd84d1cce..b47d52ded6dc3107240b075b492d38a8502c0c1f
100644
--- a/Source/core/animation/AnimationStack.cpp
+++ b/Source/core/animation/AnimationStack.cpp
@@ -30,6 +30,7 @@
 
 #include "config.h"
 #include "core/animation/AnimationStack.h"
+#include "core/animation/Interpolation.h"
 
 #include "core/animation/css/CSSAnimations.h"
 
@@ -37,12 +38,13 @@ namespace WebCore {
 
 namespace {
 
-void copyToCompositableValueMap(const AnimationEffect::CompositableValueList*
source, AnimationEffect::CompositableValueMap& target)
+void copyToActiveInterpolationMap(const Vector<RefPtr<WebCore::Interpolation>
>& source, HashMap<CSSPropertyID, RefPtr<WebCore::Interpolation> >& target)
 {
-    if (!source)
-        return;
-    for (AnimationEffect::CompositableValueList::const_iterator iter =
source->begin(); iter != source->end(); ++iter)
-        target.set(iter->first, iter->second);
+    for (Vector<RefPtr<WebCore::Interpolation> >::const_iterator iter =
source.begin(); iter != source.end(); ++iter) {
+        RefPtr<WebCore::Interpolation> interpolation = *iter;
+        WebCore::StyleInterpolation *styleInterpolation =
toStyleInterpolation(interpolation.get());
+        target.set(styleInterpolation->id(), styleInterpolation);
+    }
 }
 
 } // namespace
@@ -65,9 +67,9 @@ bool
AnimationStack::hasActiveAnimationsOnCompositor(CSSPropertyID property) con
     return false;
 }
 
-AnimationEffect::CompositableValueMap AnimationStack::compositableValues(const
AnimationStack* animationStack, const Vector<InertAnimation*>* newAnimations,
const HashSet<const Player*>* cancelledPlayers, Animation::Priority priority)
+HashMap<CSSPropertyID, RefPtr<Interpolation> >
AnimationStack::activeInterpolations(const AnimationStack* animationStack, const
Vector<InertAnimation*>* newAnimations, const HashSet<const Player*>*
cancelledPlayers, Animation::Priority priority)
 {
-    AnimationEffect::CompositableValueMap result;
+    HashMap<CSSPropertyID, RefPtr<Interpolation> > result;
 
     if (animationStack) {
         const Vector<Animation*>& animations =
animationStack->m_activeAnimations;
@@ -77,13 +79,17 @@ AnimationEffect::CompositableValueMap
AnimationStack::compositableValues(const A
                 continue;
             if (cancelledPlayers &&
cancelledPlayers->contains(animation->player()))
                 continue;
-            copyToCompositableValueMap(animation->compositableValues(),
result);
+            copyToActiveInterpolationMap(animation->activeInterpolations(),
result);
         }
     }
 
     if (newAnimations) {
-        for (size_t i = 0; i < newAnimations->size(); ++i)
-            copyToCompositableValueMap(newAnimations->at(i)->sample().get(),
result);
+        for (size_t i = 0; i < newAnimations->size(); ++i) {
+            OwnPtr<Vector<RefPtr<Interpolation> > > sample =
newAnimations->at(i)->sample();
+            if (sample) {
+                copyToActiveInterpolationMap(*sample, result);
+            }
+        }
     }
 
     return result;

[shans, 2014-03-18 20:29:15]

The CQ bit was checked by shans@chromium.org

[commit-bot: I haz the power, 2014-03-18 20:29:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shans@chromium.org/194673002/80001

[commit-bot: I haz the power, 2014-03-18 21:21:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-18 21:22:03]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[shans, 2014-03-18 22:48:04]

The CQ bit was checked by shans@chromium.org

[commit-bot: I haz the power, 2014-03-18 22:48:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shans@chromium.org/194673002/80001

[commit-bot: I haz the power, 2014-03-18 23:20:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-18 23:21:00]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[shans, 2014-03-18 23:35:23]

The CQ bit was checked by shans@chromium.org

[commit-bot: I haz the power, 2014-03-18 23:35:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shans@chromium.org/194673002/80001

[commit-bot: I haz the power, 2014-03-18 23:37:45]

Change committed as 169500