[Webkit-unassigned] [Bug 19938] Improve AnimationController

[bugzilla-daemon at webkit.org]

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





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

fixed

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

done

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

done

> Ditto. We decided on AnimationStyleChange.

done

> Add a comment to say what a CompositeAnimation is for.

done

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

> spma?

That looks suspiciously like something I would type in order to find later and
remove.

> I'm not sure we can ever get into this state with the transitions we have here.
> Much of this logic could be simplified.

Yes.

> > +    // |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?

We should.


> There's some reference to explicit animations here.

removed

> There's a problem here in some cases: <rdar://problem/6058375>

I haven't fixed this.


> +void CompositeAnimation::setAnimationStartTime(double t)
> +{
> +}

> Remove?

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.

True. removed

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

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.