[Webkit-unassigned] [Bug 19938] Improve AnimationController

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


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


hyatt at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #22151|review?(hyatt at apple.com)    |review-
               Flag|                            |




------- Comment #6 from hyatt at apple.com  2008-07-08 14:36 PDT -------
(From update of attachment 22151)
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. :)


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