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

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Dean Jackson <dino at apple.com>'s request
for review:
Bug 19938: Improve AnimationController
https://bugs.webkit.org/show_bug.cgi?id=19938

Attachment 22151: Improvement of AnimationController
https://bugs.webkit.org/attachment.cgi?id=22151&action=edit

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

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