Implement the CSS Advanced Visual Effects specifications from http://www.webkit.org/specs/CSSVisualEffects The current TOT webkit implements CSS Transitions. The goal is to bring the transition implementation up to date with a new Animation Controller, add support for CSS Animations with keyframes (and therefore the @-webkit-keyframes rule), add events for animation start/end/iteration, interfaces for accessing the keyframes and enhancing the CSS Transforms to provide a basis for 3d.
Created attachment 21107 [details] in progress patch - not ready for review This patch is included here to get initial feedback. Some parts are stubbed out and there are a couple of known bugs, but it gives an idea of the general approach. A patch ready for review will be attached within a day.
Attached a patch that does most things but: - does not implement events yet - does not do computed style for animations - does not do the JS interfaces for keyframes or matrices - has a known bug where style is applied (possibly) too often in animation loops - has unknown behaviour if 3d transforms are specified - does not parse perspective (not sure if it should, since it is ignored anyway) A complete patch should appear within 24 hours.
Created attachment 21108 [details] Simple keyframe animation with infinite repeat and easing.
Don't ever do if (layer()) That makes a virtual function call. We have if (hasLayer()) that checks a bit just to avoid that. I'm a bit confused as to why a bunch of animation functions got added to RenderObject. That doesn't seem appropriate, especially given that they're just calling through to a layer...
Comment on attachment 21107 [details] in progress patch - not ready for review This is just comments from a cursory reading, not a real review yet: I see that the RenderLayer functions are just stubs. I'd just remove them for now then. The class declaration of KeyframeList is indented using very odd style. Please pull the inline function definitions back next to the function names (instead of doing odd spacing to try to line up the braces). + // make sure there are no duplicate properties. This is an O(n^2) algorithm + // but the lists tend to be very short, so it is probably ok Why are we removing duplicate properties here? I don't like O(n^2) algorithms even when the lists are short. :) + // tell the animation controller that the style is all setup and it can start animations "setup" should be two words. We usually capitalize the first words of comment sentences, e.g., "// Tell", not "// tell". I'm still not convinced keyframes are implemented in a way that really works.
> Why are we removing duplicate properties here? It's possible to end up with two "cAnimateAll" transtisions, e.g. for properties like: -webkit-transition-property: "color", "left"; Note the quotes, which result in parsing failure. It's hard to throw those away at parsing time, because the comma-separated values have to be kept in sync with transition-duration etc.
<rdar://problem/5704655>
Created attachment 21146 [details] Completed patch (to be split) This is pretty much everything. Adds Animation Events, IDL interfaces, computed style. Addresses the comments by Hyatt above.
Created attachment 21179 [details] Update support for CSS Transforms This is an extraction of the transforms code from the complete patch.
Created attachment 21180 [details] Patch for using updated animation controller and keyframe parsing Extracted patch for software animations and keyframe parsing.
Created attachment 21181 [details] Patch for animation/transition events and other dom interfaces This is the extraction of the IDL/Events from the complete patch.
Comment on attachment 21179 [details] Update support for CSS Transforms In Transform3D.cpp: +Transform3D &Transform3D::function() {} Move the & to be with the return value, so: Transform3D& Transform3D::function() {} In CodeGeneratorObjC.pm: I don't understand this: - return "CSSValue" if $parent eq "SVGColor"; + return "CSSValue" if $parent eq "SVGColor" or $parent eq "CSSValueList"; In DerivedSources.make: VoidCallback \ + WebKitCSSTransformValue \ WheelEvent \ Double-check if tabs are actually used in that file. I see "KeyframeList" in this patch. I think that was unintentional. Can you remove it? This is very close to being able to land. I think I'll be able to r+ it with one more revision.
Should we maybe split this up into separate bugs? The attachment list is going to get long and confusing as we iterate over each patch. :)
Also, make sure you are adding files to all projects (Qt, wx, Windows, OS X, GTK).
Transforms moved to http://bugs.webkit.org/show_bug.cgi?id=19091
How do I add files to all projects?
CSS Animations now in http://bugs.webkit.org/show_bug.cgi?id=19092
DOM stuff now in http://bugs.webkit.org/show_bug.cgi?id=19091
Since this bug has been broken up, should the patches on this bug be unflagged and obsoleted?
I'll be deprecating this bug soon. See http://bugs.webkit.org/show_bug.cgi?id=19938
Comment on attachment 21181 [details] Patch for animation/transition events and other dom interfaces In general this patch looks fine. Tabs vs. spaces here: 39 VoidCallback \ 340 WebKitAnimationEvent \ 341 WebKitCSSKeyframeRule \ 342 WebKitCSSKeyframesRule \ 343 WebKitCSSTransformValue \ 340344 WheelEvent \ Single line ifs don't get { } (yes, I know the rest of that function has them wrong already: + } else if (attr->name() == onwebkittransitionendAttr) { + setHTMLEventListener(webkitTransitionEndEvent, attr); } Can't we use CamelCase for these? +onwebkitanimationend +onwebkitanimationiteration +onwebkitanimationstart +onwebkittransitionend Seems it would make them more readable. Yes, i know the other event attributes are all lower-case. I'm not sure they should be though. I guess the case matters for XHTML though, so I guess they probably have to be all lower case... This seems wrong: + if (eventType == "WebKitAnimationEvent") + return new WebKitAnimationEvent; + if (eventType == "WebKitTransitionEvent") + return new WebKitTransitionEvent; I guess Events haven't been converted to use the new Foo::create() syntax yet? They still start with a RefCount of 0 instead of 1? These could probably go in the header just fine: +bool Event::isWebKitTransitionEvent() const +{ + return false; +} Again, no create() syntax? + RefPtr<WebKitAnimationEvent> animEvent = new WebKitAnimationEvent(eventType, animationName, elapsedTime); I would think we would want to use the newer PassRefPtr<WebKitAnimationEvent> create(); syntax for all new classes. If the parent class hasn't yet been converted to start with a RefCount of 1, this one could easily ASSERT that new WebKitAnimationEvent() comes back with a RefCount of O (to make things crash if someone changes the base class and forgets to change this one) and then still return it from create(). When someone fixes Event to start with a ref count of 1, then they'll have to change the "new" in your create() method to be adoptRef(new Foo) instead. I'm surprised this ASSERT isn't in dispatchEvent!? + ASSERT(!eventDispatchForbidden()); Why should each caller need to ASSERT that? This does not need to call the constructor for the String, but it *does* need to init the double to some sane value: + WebKitAnimationEvent::WebKitAnimationEvent() + : m_animationName() + { And again: + WebKitTransitionEvent::WebKitTransitionEvent() + : m_propertyName() + { Seems these two classes are just copy/paste of one another... maybe they should just be the same class? This is not needed in the idl files: + // Introduced in DOM Level ?: Otherwise this looks fine. Maybe you still want Hyatt to look at it for content. r- though for the various small nits above.
Oops. Thanks for the comments Eric, but this bug is stale. I'll copy them into the relevant new bugs.
Comment on attachment 21180 [details] Patch for using updated animation controller and keyframe parsing stale patch.