[Webkit-unassigned] [Bug 19938] Improve AnimationController
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jul 8 18:26:15 PDT 2008
------- Comment #7 from dino at apple.com 2008-07-08 18:26 PDT -------
> Isn't this t->property()? If not, why not?
This was to avoid what hyatt complains about below - that our names are too
generic. However I changed it to property(). We should clarify the naming.
> Shouldn't duration have the FInteger|FNonNeg flags?
Changed to FTime|FNonNeg.
> I don't think the Transtion -> LayerType changes are necessary. We'll only
> ever make Transition objects.
True, until the following animation patch comes. Hyatt comments on this too.
> WebKit style is no parens around single line block.
> styleIsSetup() is a crappy method name. We should think of something better.
hyatt comments on this too
> These changes are different from those we reviewed internally. They need to be
> Do we want animation events in this patch?
No, I didn't want them. I could probably remove it, but there is some code in
AnimationController which uses these to clean up transitions (even if the
events are not fired). I didn't want to butcher AnimationController too much
since I want to finish the patch that adds the events back in.
> Also needs syncing with internally reviewed changes.
> Ditto. We decided on AnimationStyleChange.
> Add a comment to say what a CompositeAnimation is for.
> Should we use CSSPropertyID rather than ints these days?
This is a good suggestion, which I tried only to discover that we use some
special cases, such as cAnimateAll. These are not properties and so we can't
restrict our property type without bigger changes.
> I don't think we need this complexity of states for the existing transitions.
True, but again I didn't want to create a special version of
AnimationController just for this patch. In fact, I already do create a special
one, because i remove all the keyframe stuff, but i didn't want to play with
the state machine logic.
That looks suspiciously like something I would type in order to find later and
> I'm not sure we can ever get into this state with the transitions we have here.
> Much of this logic could be simplified.
> > + // |this| may be deleted here if we came out of STATE_ENDING when we've
> > been called from timerFired()
> Ick. Maybe we should RefCount<> these guys?
> There's some reference to explicit animations here.
> There's a problem here in some cases: <rdar://problem/6058375>
I haven't fixed this.
> +void CompositeAnimation::setAnimationStartTime(double t)
done (it was also being called from somewhere else, that itself wasn't being
called - so everything was removed)
> I don't think this will ever get called.
> Hmm, does playState do anything for transitions?
It could but doesn't (because there is no such property). It's a generic
feature of the animation engine though, and the same rules will apply to
animation (which will have a prop)
> Would be nice to have some comments explaing what this "overriding" is all
This is another feature from animations that isn't needed here. I'll see if I
can remove it.
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