[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