[webkit-reviews] review denied: [Bug 20119] Execute CSS Animations : [Attachment 22437] Updated patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Aug 4 12:02:04 PDT 2008

Eric Seidel <eric at webkit.org> has denied 's request for review:
Bug 20119: Execute CSS Animations

Attachment 22437: Updated patch

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.

Style violation:
+    if (m_frame) {
+    }

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

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; }

+    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 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()

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

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

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
+	 if ((anim->duration() || anim->delay()) && anim->iterationCount() &&
+	      anim->keyframeList().get() && !anim->keyframeList()->isEmpty())

bool willAnimate = (anim->duration() || anim->delay()) &&
bool hasKeyframes = anim->keyframeList() && !anim->keyframeList()->isEmpty();
if (willAnimate && hasKeyframes)
KeyframeAnimation(const_cast<Transition*>(anim), renderer, index, this));

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.

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 =
+    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);

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;

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.

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

This is probably just a call to:

+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. :)

Are there test cases for these changes?

More information about the webkit-reviews mailing list