[Webkit-unassigned] [Bug 20119] Execute CSS Animations

[bugzilla-daemon at webkit.org]

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





------- Comment #12 from dino at apple.com  2008-08-07 15:44 PDT -------
> Bleh.  I really hate the use of long macros.  I'd much rather see that code
> re-written to use static inlines with function pointers. You can use macros to
> call the static inlines if you would prefer to not type the 8 permutation of
> set/get/clear Prop or whatever you'd need.

Yeah, we hate the macros too. We'll open a bug to remove them (and the macros
for
background layers, which is the code we copied)

> 
> Style violation:
> +    if (m_frame) {
>          m_frame->animation()->styleAvailable();
> +    }

Fixed

> I would have to look at the code... but "f" seems like a bad choice in variable
> name:
> +    if (f)
> +        f->animation()->resumeAnimations(this);

This is pretty common throughout (ie. not just this function and file). In this
case
it is Frame.

> > Personally I prefer using "name" in cases like this... but that said, it really
> doens't matter either way:
> +    void setName(const String& s) { m_name = s; }

I'm not sure what you mean. s/setName/name/ ?

> +
> +    if (m_animation->direction() && (i & 1))
> +        t = 1 - t;
> "t" is not a very clear variable name for "number of iterations"
> 
> tt could have a better name too:
> +    double tt = solveCubicBezierFunction(m_animation->timingFunction().x1(),

I've renamed these to something better.

> I don't think you need the .get()s in these calls:
> +        RefPtr<TransformOperation> fromOp = i < fromSize ? from[i].get() : 0;
> 
> RefPtr will corectly construct from another RefPtr just fine.  Hum... of course
> the ternary operator might get made at you for having different types on each
> side of the : so you may need to leave it as .get()

I got rid of it in the lines above, but left it in this one since the compiler
complained.

> Style:
> +        if (!anim->isValidAnimation()) {
> +            animsChanged = true;
> +        } else {
> +    if ((m_keyframeAnimations.isEmpty() && !anim) ||
> +        (currentStyle && currentStyle->animations() &&
> *(currentStyle->animations()) == *anim)) {
> +        return;
> +    }

Fixed

> 
> I would have just written out "keyframeAnim" here:
> +            KeyframeAnimation* kfAnim = (numAnims <
> m_keyframeAnimations.size()) ? m_keyframeAnimations[numAnims] : 0;

Fixed

> 
> If I was writting this out, I might have tried to make it self-documenting:
> 
> +        // Don't bother adding the animation if it has no keyframes or won't
> animate
> +        if ((anim->duration() || anim->delay()) && anim->iterationCount() &&
> +             anim->keyframeList().get() && !anim->keyframeList()->isEmpty())
> 
> bool willAnimate = (anim->duration() || anim->delay()) &&
> anim->iterationCount();
> bool hasKeyframes = anim->keyframeList() && !anim->keyframeList()->isEmpty();
> if (willAnimate && hasKeyframes)
>   m_keyframeAnimations.append(new
> KeyframeAnimation(const_cast<Transition*>(anim), renderer, index, this));

I haven't fixed this one yet, but will.

> Why are you calling anim->keyframeList().get() here?  If that's returning a
> PassRefPtr, that's a horribly inefficient way to get a boolean answer. :) 
> (Since only functions which allocate return PassRefPtrs).  Generally we don't
> return RefPtr's from any function (since they cause ref-churn by copying and
> destroying the temporary.

Again, haven't fixed yet but will.

> 
> Also, RefPtr knows how to transparently convert to a bool, so even if that
> function were returning a RefPtr, you wouldn't need to call .get() to convert
> it to a raw pointer to convert it to a bool.
> 
> 
> There is a vector function "deleteAllValues" which will do this for you:
> +    Vector<KeyframeAnimation*>::const_iterator kfend =
> m_keyframeAnimations.end();
> +    for (Vector<KeyframeAnimation*>::const_iterator it =
> m_keyframeAnimations.begin(); it != kfend; ++it) {
> +        KeyframeAnimation* anim = *it;
> +        delete anim;
> +    }
> 
> I take it this vector m_keyframeAnimations can have null values?  You check *it
> against null all over.  Yet I don't see (or must have just missed) where these
> values are ever set to null.
> 
> I don't believe it's webkit style to indicate that variables are in/out by
> prepending "io":
> +        if (!ioAnimatedStyle)
> +            ioAnimatedStyle = const_cast<RenderStyle*>(targetStyle);

Changed

> Bad variable names:
> +    double t = m_animation->duration() ? (elapsedTime /
> m_animation->duration()) : 1;
> +    int i = (int) t;
> +    t -= i;
> +    if (m_animation->direction() && (i & 1))
> +        t = 1 - t;
> 

Fixed.

> I assume the compiler is smart enough to convert the 1 to a double instead of
> converting the division result to an int:
> +    double t = m_animation->duration() ? (elapsedTime /
> m_animation->duration()) : 1;
> 
> 
> In general I think we try and make double constants doubles (using 1.0 and 0.0,
> etc.) and floats floats, and ints ints.

Fixed (although there may be other places I haven't checked yet)

> If KeyframeAnimation is refcounted, it should be using the new create() syntax.
> instead of direct calls to "new".

Will fix this.

>  This is probably just a call to:
> dispatchGenericEvent:
> 
> +bool KeyframeAnimation::sendAnimationEvent(const AtomicString& inEventType,
> double inElapsedTime)
> +{
> +    // FIXME: Event dispatch goes here
> +    return false; // didn't dispatch an event
> 
> However I can understand if you're trying to limit the scope of your patch. :)

In this case it was an intentional limitation. A subsequent bug will add the
code.

Thanks Eric!!


-- 
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.