[webkit-reviews] review requested: [Bug 84454] [chromium] Add support for animation finished events. : [Attachment 138845] Patch

[bugzilla-daemon at webkit.org]

vollick at chromium.org has asked	for review:
Bug 84454: [chromium] Add support for animation finished events.
https://bugs.webkit.org/show_bug.cgi?id=84454

Attachment 138845: Patch
https://bugs.webkit.org/attachment.cgi?id=138845&action=review

------- Additional Comments from vollick at chromium.org
(In reply to comment #4)
> (From update of attachment 138098 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=138098&action=review
>
> Solution seems a bit overboard for the problem as I understand it, and a few
function names aren't clear. Could you provide a bit more background (the
ChangeLogs being empty does not help)?	Gonna leave r? alone for now.
It's true. We used to have animation finished events, but they were removed. It
used to be the case that the animation events carried different information
(the finished events had the final values), so I used virtual type system, but
you're right -- that's way overboard now.

>
> > Source/WebCore/platform/graphics/chromium/LayerChromium.h:250
> > +	 virtual void setAnimationEvent(const CCAnimationEvent&, double
wallClockTime);
>
> what does it mean to "set" an event? there's no m_animationEvent member, so
it's not clear what exactly this is supposed to do
Switched this to two functions, notifyAnimationStarted and
notifyAnimationFinished, as you'd suggested below.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCAnimationEvents.h:38
> > +class CCAnimationEvent {
>
> are we going to start having more animation types than started/finished? if
it's just 2 types then i think having this virtual type system is way too much
overhead - it'd be better to just have different functions for started vs
finished
Agreed. Got rid of the virtual type system, and changed the interface of
LayerChromium to two functions rather than setAnimationEvents(...).
>
> or is the problem that we need to preserve relative order of started vs
finished events in the queue? do these things have such an order?
Nope, the order doesn't matter.
>
> >
Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.h:91
> > +	 void notifyAnimationStarted(const CCAnimationEvent&);
>
> does this only take CCAnimationStartedEvent instances? if so, can you tighten
the type up? if not, can you update the name?
Done.