Bug 84454 - [chromium] Add support for animation finished events.
Summary: [chromium] Add support for animation finished events.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: vollick
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-20 08:18 PDT by vollick
Modified: 2012-04-25 14:13 PDT (History)
4 users (show)

See Also:


Attachments
Patch (23.91 KB, patch)
2012-04-20 08:23 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (27.99 KB, patch)
2012-04-20 08:48 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (19.72 KB, patch)
2012-04-25 11:01 PDT, vollick
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description vollick 2012-04-20 08:18:20 PDT
These events are not currently used by anyone, but will be useful for wjmaclean@ shortly.
Comment 1 vollick 2012-04-20 08:23:43 PDT
Created attachment 138097 [details]
Patch
Comment 2 vollick 2012-04-20 08:48:48 PDT
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.
Comment 3 W. James MacLean 2012-04-20 08:51:20 PDT
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!
Comment 4 James Robinson 2012-04-24 23:32:26 PDT
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?
Comment 5 vollick 2012-04-25 11:01:42 PDT
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.
Comment 6 James Robinson 2012-04-25 13:31:39 PDT
Comment on attachment 138845 [details]
Patch

Cool, much better. Thanks!
Comment 7 WebKit Review Bot 2012-04-25 14:12:54 PDT
Comment on attachment 138845 [details]
Patch

Clearing flags on attachment: 138845

Committed r115243: <http://trac.webkit.org/changeset/115243>
Comment 8 WebKit Review Bot 2012-04-25 14:13:09 PDT
All reviewed patches have been landed.  Closing bug.