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.
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 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.
Comment on attachment 138845 [details] Patch Cool, much better. Thanks!
Comment on attachment 138845 [details] Patch Clearing flags on attachment: 138845 Committed r115243: <http://trac.webkit.org/changeset/115243>
All reviewed patches have been landed. Closing bug.