[Webkit-unassigned] [Bug 19938] Improve AnimationController

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jul 7 21:07:23 PDT 2008


https://bugs.webkit.org/show_bug.cgi?id=19938





------- Comment #5 from simon.fraser at apple.com  2008-07-07 21:07 PDT -------
+++ 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?


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.



More information about the webkit-unassigned mailing list