Bug 19938 – Improve AnimationController

• Component: CSS
• Status: RESOLVED FIXED

[Dean Jackson]

AnimationController should be updated so adding CSS Animations is easier. The attached patch does the following:

- adds a new AnimationController
- supports -webkit-transition-delay (since the new controller does it)
- removes -webkit-transition-repeat-count (since it never existed)
- updates the -webkit-transition shorthand to reflect removing repeat count
- provides computed style for transition properties
- updates the Transition class so that properties can be shared with animations
- adds a "now" keyword for -webkit-transition-delay
- adds a new change type for style (changed by animation)
- adds new event names (although they are not dispatched yet)
- makes text stroke and text fill colors returned by RenderStyle const

Sorry this patch is so big. The problem is that AnimationController is quite complex and drags in a lot of other stuff. I was hoping to get something fairly minimal that just did Transitions (ie. not much different from the current TOT).

[Dean Jackson]

Created attachment 22151 [details]
Improvement of AnimationController

huge patch that adds a state-based AnimationController

[Dean Jackson]

Created attachment 22152 [details]
Test case for simple transitions

[Dean Jackson]

Created attachment 22153 [details]
test case for different timing functions

[Dean Jackson]

Created attachment 22154 [details]
Test case for transition delay

[Simon Fraser (smfr)]

+++ b/WebCore/css/CSSComputedStyleDeclaration.cpp

+        case CSSPropertyWebkitTransitionProperty: {
+            RefPtr<CSSValueList> list = CSSValueList::createCommaSeparated();
+            const Transition* t = style->transitions();
+            if (t) {
+                for ( ; t; t = t->next()) {
+                    int prop = t->transitionProperty();

Isn't this t->property()? If not, why not?

diff --git a/WebCore/css/CSSParser.cpp b/WebCore/css/CSSParser.cpp

-PassRefPtr<CSSValue> CSSParser::parseTransitionRepeatCount()
+PassRefPtr<CSSValue> CSSParser::parseDuration()
 {
     CSSParserValue* value = m_valueList->current();
-    if (value->id == CSSValueInfinite)
-        return CSSPrimitiveValue::createIdentifier(value->id);
-    if (validUnit(value, FInteger|FNonNeg, m_strict))
+    if (validUnit(value, FTime, m_strict))
         return CSSPrimitiveValue::create(value->fValue, (CSSPrimitiveValue::UnitTypes)value->unit);
     return 0;
 }

Shouldn't duration have the FInteger|FNonNeg flags?

diff --git a/WebCore/css/CSSStyleSelector.cpp b/WebCore/css/CSSStyleSelector.cpp
index 36c652d..3b9a456 100644
--- a/WebCore/css/CSSStyleSelector.cpp
+++ b/WebCore/css/CSSStyleSelector.cpp
@@ -182,15 +182,15 @@ HANDLE_FILL_LAYER_INHERIT_AND_INITIAL(mask, Mask, prop, Prop)
 #define HANDLE_MASK_VALUE(prop, Prop, value) \
 HANDLE_FILL_LAYER_VALUE(mask, Mask, prop, Prop, value)
 
-#define HANDLE_TRANSITION_INHERIT_AND_INITIAL(prop, Prop) \
+#define HANDLE_MULTILAYER_INHERIT_AND_INITIAL(layerType, LayerType, prop, Prop) \
 if (isInherit) { \
-    Transition* currChild = m_style->accessTransitions(); \
-    Transition* prevChild = 0; \
-    const Transition* currParent = m_parentStyle->transitions(); \
+    LayerType* currChild = m_style->access##LayerType##s(); \
+    LayerType* prevChild = 0; \

I don't think the Transtion -> LayerType changes are necessary. We'll only
ever make Transition objects.

diff --git a/WebCore/dom/Document.cpp b/WebCore/dom/Document.cpp

 void Document::updateRendering()
 {
-    if (hasChangedChild())
+    if (hasChangedChild()) {
         recalcStyle(NoChange);
+    }

WebKit style is no parens around single line block.

+    // tell the animation controller that the style is all setup and it can start animations
+    if (m_frame)
+        m_frame->animation()->styleIsSetup();    
 }

styleIsSetup() is a crappy method name. We should think of something better.

diff --git a/WebCore/dom/Element.cpp b/WebCore/dom/Element.cpp
index c83a7d2..81bb90d 100644
--- a/WebCore/dom/Element.cpp
+++ b/WebCore/dom/Element.cpp
@@ -751,6 +751,10 @@ void Element::recalcStyle(StyleChange change)
     if (hasParentStyle && (change >= Inherit || changed())) {
         RenderStyle *newStyle = document()->styleSelector()->styleForElement(this);
         StyleChange ch = diff(currentStyle, newStyle);
+
+        if (styleChangeType() == AnimatableStyleChange)
+            ch = Force;
+
         if (ch == Detach || !currentStyle) {
             if (attached())
                 detach();
@@ -784,9 +788,11 @@ void Element::recalcStyle(StyleChange change)
                 newStyle->setChildrenAffectedByDirectAdjacentRules();
         }
 
-        if (ch != NoChange)
-            setRenderStyle(newStyle);
-        else if (changed() && (document()->usesSiblingRules() || document()->usesDescendantRules())) {
+        if (ch != NoChange) {
+            if (newStyle) {
+                setRenderStyle(newStyle);
+            }
+        } else if (changed() && (document()->usesSiblingRules() || document()->usesDescendantRules())) {
             // Although no change occurred, we use the new style so that the cousin style sharing code won't get
             // fooled into believing this style is the same.  This is only necessary if the document actually uses
             // sibling/descendant rules, since otherwise it isn't possible for ancestor styles to affect sharing of
@@ -800,7 +806,7 @@ void Element::recalcStyle(StyleChange change)
         newStyle->deref(document()->renderArena());
 
         if (change != Force) {
-            if ((document()->usesDescendantRules() || hasPositionalRules) && styleChangeType() == FullStyleChange)
+            if ((document()->usesDescendantRules() || hasPositionalRules) && styleChangeType() >= FullStyleChange)
                 change = Force;
             else
                 change = ch;

These changes are different from those we reviewed internally. They need to be updated.

diff --git a/WebCore/dom/EventNames.h b/WebCore/dom/EventNames.h
index 872c6cf..d289829 100644
--- a/WebCore/dom/EventNames.h
+++ b/WebCore/dom/EventNames.h
@@ -119,6 +119,12 @@ namespace WebCore { namespace EventNames {
     macro(progress) \
     macro(stalled) \
     \
+    macro(webkitAnimationEnd) \
+    macro(webkitAnimationStart) \
+    macro(webkitAnimationIteration) \

Do we want animation events in this patch?

diff --git a/WebCore/dom/Node.cpp b/WebCore/dom/Node.cpp
index c291b60..435d029 100644
--- a/WebCore/dom/Node.cpp
+++ b/WebCore/dom/Node.cpp
@@ -380,7 +380,7 @@ void Node::setChanged(StyleChangeType changeType)
     if ((changeType != NoStyleChange) && !attached()) // changed compared to what?
         return;
 
-    if (!(changeType == InlineStyleChange && m_styleChange == FullStyleChange))
+    if (changeType > (StyleChangeType)m_styleChange)

Also needs syncing with internally reviewed changes.

diff --git a/WebCore/dom/Node.h b/WebCore/dom/Node.h
index 60f7a9c..20370d0 100644
--- a/WebCore/dom/Node.h
+++ b/WebCore/dom/Node.h
@@ -60,7 +60,7 @@ struct NodeListsNodeData;
 
 typedef int ExceptionCode;
 
-enum StyleChangeType { NoStyleChange, InlineStyleChange, FullStyleChange };
+enum StyleChangeType { NoStyleChange, InlineStyleChange, FullStyleChange, AnimatableStyleChange };

Ditto. We decided on AnimationStyleChange.

diff --git a/WebCore/page/AnimationController.cpp b/WebCore/page/AnimationController.cpp
index 444e6b4..97d92da 100644
--- a/WebCore/page/AnimationController.cpp
+++ b/WebCore/page/AnimationController.cpp

+class CompositeAnimation : public Noncopyable {

Add a comment to say what a CompositeAnimation is for.

+public:
+    CompositeAnimation(bool suspended, AnimationControllerPrivate* animationController)
+    : m_suspended(suspended)
+    , m_animationController(animationController)
+    , m_numStyleIsSetupWaiters(0)
+    { }
+    
+    ~CompositeAnimation()
+    {
+        deleteAllValues(m_transitions);
+    }
+    
+    RenderStyle* animate(RenderObject*, RenderStyle* currentStyle, RenderStyle* targetStyle);
+    
+    void setAnimating(bool inAnimating);
+    bool animating();
+    
+    bool hasAnimationForProperty(int prop) const { return m_transitions.contains(prop); }
+    
+    void resetTransitions(RenderObject*);
+    void resetAnimations(RenderObject*);
+    
+    void cleanupFinishedAnimations(RenderObject*);
+    
+    void setAnimationStartTime(double t);
+    void setTransitionStartTime(int property, double t);

Should we use CSSPropertyID rather than ints these days?

+    // Animations and Transitions go through the states below. When entering the STARTED state
+    // the animation is started. This may or may not require deferred response from the animator.
+    // If so, we stay in this state until that response is received (and it returns the start time).
+    // Otherwise, we use the current time as the start time and go immediately to the LOOPING or
+    // ENDING state.
+    enum AnimState { 
+        STATE_NEW,                      // animation just created, animation not running yet
+        STATE_START_WAIT_TIMER,         // start timer running, waiting for fire
+        STATE_START_WAIT_STYLE_SETUP,   // waiting for style setup so we can start animations
+        STATE_START_WAIT_RESPONSE,      // animation started, waiting for response
+        STATE_LOOPING,                  // response received, animation running, loop timer running, waiting for fire
+        STATE_ENDING,                   // response received, animation running, end timer running, waiting for fire
+        STATE_PAUSED_WAIT_TIMER,        // animation in pause mode when animation started
+        STATE_PAUSED_WAIT_RESPONSE,     // animation paused when in STARTING state
+        STATE_PAUSED_RUN,               // animation paused when in LOOPING or ENDING state
+        STATE_DONE                      // end timer fired, animation finished and removed
+    };
+    
+    enum AnimStateInput {
+        STATE_INPUT_MAKE_NEW,           // reset back to new from any state
+        STATE_INPUT_START_ANIMATION,    // animation requests a start
+        STATE_INPUT_RESTART_ANIMATION,  // force a restart from any state
+        STATE_INPUT_START_TIMER_FIRED,  // start timer fired
+        STATE_INPUT_STYLE_SETUP,        // style is setup, ready to start animating
+        STATE_INPUT_START_TIME_SET,     // m_startTime was set
+        STATE_INPUT_LOOP_TIMER_FIRED,   // loop timer fired
+        STATE_INPUT_END_TIMER_FIRED,    // end timer fired
+        STATE_INPUT_PAUSE_OVERRIDE,     // pause an animation due to override
+        STATE_INPUT_RESUME_OVERRIDE,    // resume an overridden animation
+        STATE_INPUT_PLAY_STATE_RUNNING, // play state paused -> running
+        STATE_INPUT_PLAY_STATE_PAUSED,  // play state running -> paused
+        STATE_INPUT_END_ANIMATION       // force an end from any state
+    };

I don't think we need this complexity of states for the existing transitions.

+class ImplicitAnimation : public AnimationBase {
+public:
+    ImplicitAnimation(const Transition* transition, RenderObject* renderer, CompositeAnimation* compAnim)
+    : AnimationBase(transition, renderer, compAnim)
+    , m_property(transition->transitionProperty())
+    , m_overridden(false)
+    , m_fromStyle(0)
+    , m_toStyle(0)
+    {
+    }
+    
+    virtual ~ImplicitAnimation()
+    {
+        ASSERT(!m_fromStyle && !m_toStyle);
+        
+        // If we were waiting for an end event, we need to finish the animation to make sure no old
+        // animations stick around in the lower levels
+        if (waitingForEndEvent() && m_object)
+            ASSERT(0);
+        /* spma
+            m_object->transitionFinished(m_property);
+         */

spma?

+        case STATE_START_WAIT_STYLE_SETUP:
+            ASSERT(input == STATE_INPUT_STYLE_SETUP || input == STATE_INPUT_PLAY_STATE_PAUSED);
+            
+            m_compAnim->setWaitingForStyleIsSetup(false);
+            
+            if (input == STATE_INPUT_STYLE_SETUP) {
+                DSM_PRINT("STATE_START_WAIT_STYLE_SETUP", "STATE_INPUT_STYLE_SETUP", "STATE_START_WAIT_RESPONSE");
+                // Start timer has fired, tell the animation to start and wait for it to respond with start time
+                m_animState = STATE_START_WAIT_RESPONSE;
+                
+                overrideAnimations();
+                
+                // Send start event, if needed
+                onAnimationStart(0.0f); // the elapsedTime is always 0 here
+                
+                // Start the animation
+                if (overridden() || !startAnimation(0)) {
+#ifdef DEBUG_STATE_MACHINE
+                    fprintf(stderr, "***         startAnimation returned false, setting start time immediately\n");
+#endif
+                    // We're not going to get a startTime callback, so fire the start time here
+                    m_animState = STATE_START_WAIT_RESPONSE;
+                    updateStateMachine(STATE_INPUT_START_TIME_SET, currentTime());
+                }
+                else
+                    m_waitedForResponse = true;

I'm not sure we can ever get into this state with the transitions we have here. Much of this logic could be simplified.

+    // |this| may be deleted here if we came out of STATE_ENDING when we've been called from timerFired()

Ick. Maybe we should RefCount<> these guys?


+void AnimationBase::animationEventDispatcherFired(HTMLElement* element, const AtomicString& name, int property, 
+                                                  bool reset, const AtomicString& eventType, double elapsedTime)
 {
-    double elapsedTime = currentTime() - m_startTime;
+    m_waitingForEndEvent = false;
     
-    if (m_finished || !m_duration || elapsedTime >= m_duration)
-        return 1.0;
+    // Keep an atomic string on the stack to keep it alive until we exit this method
+    // (since dispatching the event may cause |this| to be deleted, therefore removing
+    // the last ref to the atomic string).
+    AtomicString animName(name);
+    AtomicString animEventType(eventType);
+    // Make sure the element sticks around too
+    RefPtr<HTMLElement> elementRetainer(element);
+    
+    ASSERT(!element || (element->document() && !element->document()->inPageCache()));
+    if (!element)
+        return;
+    
+    // We didn't make the endAnimation call if we dispatched an event. This is so
+    // we can call the animation event handler then remove the animation. 
+    // We can't call endAnimation() here because it is specialized depending on whether
+    // we are doing Transitions or Animations. So we do the equivalent to it here.
+    if (animEventType == EventNames::webkitAnimationEndEvent) {
+        if (element->renderer()) {
+            // restore the original (unanimated) style
+            setChanged(element->renderer()->element());
+        }
+    }
+}

There's some reference to explicit animations here.

 static inline TransformOperations blendFunc(const TransformOperations& from, const TransformOperations& to, double progress)
@@ -237,7 +925,8 @@ static inline TransformOperations blendFunc(const TransformOperations& from, con
         TransformOperation* fromOp = i < fromSize ? from[i].get() : 0;
         TransformOperation* toOp = i < toSize ? to[i].get() : 0;
         TransformOperation* blendedOp = toOp ? toOp->blend(fromOp, progress) : fromOp->blend(0, progress, true);
-        result.append(blendedOp);
+        if (blendedOp)
+            result.append(blendedOp);
     }

There's a problem here in some cases: <rdar://problem/6058375>

+// "animating" means that something is running that requires the timer to keep firing
+// (e.g. a software animation)

e.g. transition

+void CompositeAnimation::setAnimationStartTime(double t)
+{
+}

Remove?

+void CompositeAnimation::setTransitionStartTime(int property, double t)
+{
+    // Set start time for given property transition
+    CSSPropertyTransitionsMap::const_iterator end = m_transitions.end();
+    for (CSSPropertyTransitionsMap::const_iterator it = m_transitions.begin(); it != end; ++it) {
+        ImplicitAnimation* anim = it->second;
+        if (anim && anim->waitingForStartTime() && 
+            (anim->transitionProperty() == property || anim->transitionProperty() == cAnimateAll))
+            anim->updateStateMachine(AnimationBase::STATE_INPUT_START_TIME_SET, t);
+    }
+}

I don't think this will ever get called.

+void CompositeAnimation::suspendAnimations()
+{
+    if (m_suspended)
+        return;
+    
+    m_suspended = true;
+
+    CSSPropertyTransitionsMap::const_iterator end = m_transitions.end();
+    for (CSSPropertyTransitionsMap::const_iterator it = m_transitions.begin(); it != end; ++it) {
+        ImplicitAnimation* anim = it->second;
+        if (anim && anim->hasStyle()) {
+            anim->updatePlayState(false);
+        }
+    }
+}

Hmm, does playState do anything for transitions?

+void CompositeAnimation::overrideImplicitAnimations(int property)
+{
+    CSSPropertyTransitionsMap::const_iterator end = m_transitions.end();
+    for (CSSPropertyTransitionsMap::const_iterator it = m_transitions.begin(); it != end; ++it) {
+        ImplicitAnimation* anim = it->second;
+        if (anim && (anim->transitionProperty() == property || anim->transitionProperty() == cAnimateAll))
+            anim->setOverridden(true);
+    }
+}
+
+void CompositeAnimation::resumeOverriddenImplicitAnimations(int property)
+{
+    CSSPropertyTransitionsMap::const_iterator end = m_transitions.end();
+    for (CSSPropertyTransitionsMap::const_iterator it = m_transitions.begin(); it != end; ++it) {
+        ImplicitAnimation* anim = it->second;
+        if (anim && (anim->transitionProperty() == property || anim->transitionProperty() == cAnimateAll))
+            anim->setOverridden(false);
+    }
+}

Would be nice to have some comments explaing what this "overriding" is all about.

diff --git a/WebCore/rendering/style/RenderStyle.cpp b/WebCore/rendering/style/RenderStyle.cpp
index 0213a29..402c5a2 100644
--- a/WebCore/rendering/style/RenderStyle.cpp
+++ b/WebCore/rendering/style/RenderStyle.cpp
@@ -41,6 +41,12 @@ static bool imagesEquivalent(StyleImage* image1, StyleImage* image2)
     return true;
 }
 
+// compare floating points, taking nan into account
+static inline bool floatingPointValuesEqual(float a, float b)
+{
+    return (isnan(a) && isnan(b)) || (a == b);
+}

Don't need this.

@@ -1869,32 +1851,58 @@ const Vector<StyleDashboardRegion>& RenderStyle::noneDashboardRegions()
 void RenderStyle::adjustTransitions()
 {
     if (transitions()) {
-        if (transitions()->isEmpty()) {
+        if (transitions()->isEmptyOrZeroDuration()) {
             clearTransitions();
             return;
         }
 
         Transition* next;
         for (Transition* p = accessTransitions(); p; p = next) {
-            next = p->m_next;
+            next = p->next();
             if (next && next->isEmpty()) {
-                delete next;
-                p->m_next = 0;
+                p->setNext(0); // removes the ref, so next gets deleted
+                next = 0;
                 break;
             }
         }
-    
+
+        if (!transitions())
+            return;
+
         // Repeat patterns into layers that don't have some properties set.
         accessTransitions()->fillUnsetProperties();
+        
+        // make sure there are no duplicate properties. This is an O(n^2) algorithm
+        // but the lists tend to be very short, so it is probably ok

Not when it could be used as a DOS attack. Would be nice to find something more
efficient.

diff --git a/WebCore/rendering/style/RenderStyle.h b/WebCore/rendering/style/RenderStyle.h
index ebab8b6..2d21186 100644
--- a/WebCore/rendering/style/RenderStyle.h
+++ b/WebCore/rendering/style/RenderStyle.h
@@ -1257,58 +1257,68 @@ private:

-    int m_duration;
-    int m_repeatCount;
+private:
+    double m_duration;
     TimingFunction m_timingFunction;
+    float m_delay;

Stupid question: why is duration a double, and delay a float?

[Dave Hyatt]

Comment on attachment 22151 [details]
Improvement of AnimationController

Just an initial comment... the bigger you make a patch, the more likely there will be something in it that holds it all up.  :)  Keep breaking stuff up if you can, but for now I'll comment on the whole shebang.

I would not change the name of this macro...

HANDLE_MULTILAYER_INHERIT_AND_INITIAL

The use of the term "layer" is something we should avoid with the transition/animation stuff, since that's more of a background term (and background no longer uses this macro).

Just keep using the term TRANSITION and pass in an "animationType" instead.

I'm a little concerned about generic names in the parser like "parseDuration"... but I can't think of anything better.

+    // tell the animation controller that the style is all setup and it can start animations

Set up is two words not one.  Fix the comment.  I also hate the method name "styleIsSetup".  How about "styleAvailable."

I don't understand the loadComplete stuff in Document.  Are animations really delayed all the way until the onload fires?  That does not seem like what we want at all.... some slow-loading ad in an iframe holds up all animations?

+        if (ch != NoChange) {
+            if (newStyle) {
+                setRenderStyle(newStyle);
+            }

I don't understand this null check.  I didn't think newStyle could ever be 0.

+
+        if (styleChangeType() == AnimatableStyleChange)
+            ch = Force;
+

Why do you need a new style change type?

-    if (!(changeType == InlineStyleChange && m_styleChange == FullStyleChange))
+    if (changeType > (StyleChangeType)m_styleChange)

Is the above really correct?  Does not look logically the same to me.

"setWaitingForStyleIsSetup"  Ludicrous name.  setWaitingForStyle would be sufficient.

Please verify that the transitions tests in WebCore/manual-tests still work correctly.  It looks to me like your merge has broken this and regressed how transitions work when they change.


In general I really dislike #ifdef DEBUGGING code that does printfs.  We ended up removing a lot of the KHTML code that did that, since it just clutters the code up and makes it harder to read.  Write a test harness that can use run-webkit-tests in some way if need be, but ditch all the printf debugging. :)

[Dean Jackson]

> Isn't this t->property()? If not, why not?

This was to avoid what hyatt complains about below - that our names are too generic. However I changed it to property(). We should clarify the naming.

> Shouldn't duration have the FInteger|FNonNeg flags?

Changed to FTime|FNonNeg.

> I don't think the Transtion -> LayerType changes are necessary. We'll only
> ever make Transition objects.

True, until the following animation patch comes. Hyatt comments on this too.

> WebKit style is no parens around single line block.

fixed

> styleIsSetup() is a crappy method name. We should think of something better.

hyatt comments on this too


> These changes are different from those we reviewed internally. They need to be
> updated.

done

> Do we want animation events in this patch?

No, I didn't want them. I could probably remove it, but there is some code in AnimationController which uses these to clean up transitions (even if the events are not fired). I didn't want to butcher AnimationController too much since I want to finish the patch that adds the events back in.

> Also needs syncing with internally reviewed changes.

done

> Ditto. We decided on AnimationStyleChange.

done

> Add a comment to say what a CompositeAnimation is for.

done

> Should we use CSSPropertyID rather than ints these days?

This is a good suggestion, which I tried only to discover that we use some special cases, such as cAnimateAll. These are not properties and so we can't restrict our property type without bigger changes.


> I don't think we need this complexity of states for the existing transitions.

True, but again I didn't want to create a special version of AnimationController just for this patch. In fact, I already do create a special one, because i remove all the keyframe stuff, but i didn't want to play with the state machine logic.

> spma?

That looks suspiciously like something I would type in order to find later and remove.

> I'm not sure we can ever get into this state with the transitions we have here.
> Much of this logic could be simplified.

Yes.

> > +    // |this| may be deleted here if we came out of STATE_ENDING when we've
> > been called from timerFired()
>
> Ick. Maybe we should RefCount<> these guys?

We should.


> There's some reference to explicit animations here.

removed

> There's a problem here in some cases: <rdar://problem/6058375>

I haven't fixed this.


> +void CompositeAnimation::setAnimationStartTime(double t)
> +{
> +}

> Remove?

done (it was also being called from somewhere else, that itself wasn't being called - so everything was removed)

> I don't think this will ever get called.

True. removed

> Hmm, does playState do anything for transitions?

It could but doesn't (because there is no such property). It's a generic feature of the animation engine though, and the same rules will apply to animation (which will have a prop)

> Would be nice to have some comments explaing what this "overriding" is all
> about.

This is another feature from animations that isn't needed here. I'll see if I can remove it.

[Dean Jackson]

Created attachment 22315 [details]
updated animation controller patch

This patch is updated after comments by smfr and hyatt. It has a regression, in that running transitions will be reset if style changes. A fix for that is in the works, but I'd like to progress the review anyway.

[Dean Jackson]

Created attachment 22442 [details]
Patch update 2

This patch adds the code to not restart running transitions unless the to value changes.

[Dave Hyatt]

Comment on attachment 22442 [details]
Patch update 2

CSSComputedStyleDeclaration.cpp should not be in this patch.

Please get rid of the loadComplete stuff.  Animations need to just start immediately when the now keyword is used.  It's completely wrong to wait until the entire document has loaded (in an incremental world you just can't hold off starting animations because some ad in an iframe never completes).  The animation feature will just be completely bizarre and unpredictable in the real world if a user is able to interact with the page for many seconds before animations are allowed to run.

Is "#pragma mark -" really necessary?  We don't tend to put those in WebCore code.  Really that just signals that the file needs to be split up, which we could do post-landing.

This seems wrong:

+    HTMLElement* elementForEventDispatch()
+    {
+        if (m_object->node() && m_object->node()->isHTMLElement())
+            return static_cast<HTMLElement*>(m_object->node());
+        return 0;
+    }

Why would I need an HTMLElement to do event dispatch?  Animation events should be fired on all types of elements...

I notice that HTMLElements are used in several places.  This seems wrong and this code needs to be changed to use Element instead.

If you really want Transition to be refcounted, then I would suggest moving to a model where you have Vector<RefPtr<Transition> >* m_transitions in the RenderStyle.  If someone outside of RenderStyle is holding on to a ref to a transition, then I might also suggest having the RenderStyle transition "list node" object hold the refcounted "transition" object as a member in each node of the list.

The mixture though just makes everything look really strange, since RenderStyle doesn't really pay attention to the refcounting (it clones m_next chains in the assignment operator and copy constructor).

I also think Transition should be renamed to something more generic that is suitable for both Animations and Transitions (but am unsure of what that name should be).

[Dave Hyatt]

g_propertyWrappers should be gPropertyWrappers.

[Dean Jackson]

Created attachment 22478 [details]
Patch update 3

Updated patch after hyatt's review. Still waiting on the RefPtr fix for Animation lists in RenderStyle.
That update will come soon (fingers crossed)

[Dean Jackson]

Created attachment 22571 [details]
Patch Update 4

This patch includes all the review fixes (Animations now stored in Vector). It also fixes a running transition bug, and moves Animations to the static create() syntax. I think it's ready to land now.

[Dave Hyatt]

Comment on attachment 22571 [details]
Patch Update 4

r=me

[Dean Jackson]

Committed r35545
	A	WebCore/manual-tests/transition-left.html
	A	WebCore/manual-tests/transition-timing-functions.html
	A	WebCore/manual-tests/transition-delay.html
	M	WebCore/rendering/RenderObject.cpp
	M	WebCore/rendering/RenderWidget.cpp
	M	WebCore/rendering/style/RenderStyle.cpp
	M	WebCore/rendering/style/RenderStyle.h
	M	WebCore/dom/EventNames.h
	M	WebCore/dom/Document.cpp
	M	WebCore/dom/Document.h
	M	WebCore/dom/Node.cpp
	M	WebCore/dom/Element.cpp
	M	WebCore/dom/Node.h
	M	WebCore/ChangeLog
	M	WebCore/css/CSSValueKeywords.in
	M	WebCore/css/CSSStyleSelector.cpp
	M	WebCore/css/CSSStyleSelector.h
	M	WebCore/css/CSSParser.cpp
	M	WebCore/css/CSSValue.h
	M	WebCore/css/CSSPropertyNames.in
	M	WebCore/css/CSSComputedStyleDeclaration.cpp
	M	WebCore/css/CSSParser.h
	M	WebCore/css/CSSTimingFunctionValue.h
	M	WebCore/history/CachedPage.cpp
	M	WebCore/page/AnimationController.h
	M	WebCore/page/AnimationController.cpp
	M	WebCore/page/Frame.cpp
	M	LayoutTests/ChangeLog
	M	LayoutTests/fast/css/computed-style-without-renderer-expected.txt
	M	LayoutTests/fast/css/computed-style-expected.txt
	M	LayoutTests/svg/css/getComputedStyle-basic-expected.txt