[webkit-reviews] review denied: [Bug 19938] Improve AnimationController : [Attachment 22151] Improvement of AnimationController

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jul 8 14:36:10 PDT 2008

Dave Hyatt <hyatt at apple.com> has denied Dean Jackson <dino at apple.com>'s request
for review:
Bug 19938: Improve AnimationController

Attachment 22151: Improvement of AnimationController

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
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...


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 ==
+    if (changeType > (StyleChangeType)m_styleChange)

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

"setWaitingForStyleIsSetup"  Ludicrous name.  setWaitingForStyle would be

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

More information about the webkit-reviews mailing list