Bug 84454 – [chromium] Add support for animation finished events.

• Component: WebKit Misc.
• Status: RESOLVED FIXED

These events are not currently used by anyone, but will be useful for wjmaclean@ shortly.

Created attachment 138097 [details]
Patch

Created attachment 138098 [details]
Patch

Plumbs the animation finished event out through the layer animation delegate.
Adds a unit test to ensure that the notification is actually sent.

[W. James MacLean]

Comment on attachment 138098 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138098&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationDelegate.h:34
> +    virtual void notifyAnimationFinished(double time) = 0;

LGTM. Thanks for adding this to the patch!

[James Robinson]

Comment on attachment 138098 [details]
Patch

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.

> 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

> 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

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?

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

Created attachment 138845 [details]
Patch

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

[James Robinson]

Comment on attachment 138845 [details]
Patch

Cool, much better. Thanks!

[WebKit Review Bot]

Comment on attachment 138845 [details]
Patch

Clearing flags on attachment: 138845

Committed r115243: <http://trac.webkit.org/changeset/115243>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.