Bug 20068 - adding animation property support to RenderStyle
Summary: adding animation property support to RenderStyle
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 19938
Blocks: 19028
  Show dependency treegraph
 
Reported: 2008-07-16 17:52 PDT by Dean Jackson
Modified: 2008-08-05 12:09 PDT (History)
1 user (show)

See Also:


Attachments
adds animation properties into RenderStyle and Transition (25.89 KB, patch)
2008-07-16 17:53 PDT, Dean Jackson
hyatt: review+
Details | Formatted Diff | Diff
updated patch (38.16 KB, patch)
2008-07-25 18:08 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Second update to patch (28.63 KB, patch)
2008-07-31 10:45 PDT, Dean Jackson
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2008-07-16 17:52:02 PDT
RenderStyle and its Transition class needs to be extended to support the animation properties:
- direction
- iterationCount
- playState
- name

This patch adds these properties, but doesn't actually set them (ie. does not implement the CSS parsing part).
Comment 1 Dean Jackson 2008-07-16 17:53:15 PDT
Created attachment 22327 [details]
adds animation properties into RenderStyle and Transition
Comment 2 Dave Hyatt 2008-07-23 13:53:11 PDT
Comment on attachment 22327 [details]
adds animation properties into RenderStyle and Transition

-    setStyle(style);
+    if (style)
+        setStyle(style);

setAnimatableStyle should never be called with a null style.   This smacks of working around some other problem.  If you have a test case where a null style was set, then we should be debugging how that happened and stop it rather than throwing in an unnecessary null check.

I think "Transition" could ultimately be split up into multiple classes (it seems gross to have all the animation and transition properties together in a single class that is still called "Transition").  I think this could be done later though.

Keyframes are all wrong, but I have promised to look the other way for this initial landing.

EAnimPlayState is only 2 bits so could really be part of the "isSet" bitfield properties if you moved it to after the keyframe list and made it into:

unsigned m_playState : 2;

(unsigned for Windows compiler's benefit)

That would save a bit of memory.

r=me without making these changes, but I'd like to see the null check removed.
Comment 3 Dave Hyatt 2008-07-23 13:58:51 PDT
Comment on attachment 22327 [details]
adds animation properties into RenderStyle and Transition

Oh I also forgot that KeyframeList needs to be updated to the new refcounting model (with a create function).  That's probably enough to tip this over into needing a new patch.
Comment 4 Simon Fraser (smfr) 2008-07-23 14:02:08 PDT
Filed bug 20148 on fixing keyframes to not live in RenderStyle.
Comment 5 Dave Hyatt 2008-07-23 14:11:41 PDT
Comment on attachment 22327 [details]
adds animation properties into RenderStyle and Transition

Ok, r=me assuming cleanup is done.
Comment 6 Dean Jackson 2008-07-25 18:08:59 PDT
Created attachment 22489 [details]
updated patch

Updated patch with review fixes. It won't be committed until a blocking patch lands.
Comment 7 Dean Jackson 2008-07-31 10:45:32 PDT
Created attachment 22579 [details]
Second update to patch

Second update to patch. Hyatt already gave a review+. This is waiting on the blocking bug before it can be landed.
Comment 8 Dean Jackson 2008-08-05 12:09:11 PDT
Committed r35568
	M	WebCore/rendering/style/RenderStyle.cpp
	M	WebCore/rendering/style/RenderStyle.h
	M	WebCore/ChangeLog
	M	WebCore/css/CSSStyleSelector.cpp
	M	WebCore/css/CSSComputedStyleDeclaration.cpp
	M	WebCore/page/AnimationController.cpp