Summary: | [chromium] Animation events should only be used for synchronizing animation start times | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | vollick | ||||||||
Component: | WebKit Misc. | Assignee: | vollick | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cc-bugs, enne, jamesr, nduca, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 81106 | ||||||||||
Bug Blocks: | 79536 | ||||||||||
Attachments: |
|
Description
vollick
2012-03-18 20:25:39 PDT
Created attachment 132534 [details]
Patch
Will post another CL with the ChangeLog changes when 81106 lands.
Created attachment 132827 [details]
Patch
Eliminated animation finished events. Animations will finish naturally on the
main thread and the final values will be set.
Animation started events are now also used to synchronize the main thread
animations' start times with their impl thread equivalents, preventing skew.
Until main thread animations receive their synchronized start times, they will
apply their initial values (i.e., they will be paused at time zero). This
guarantees that we will not jump to the animation's final value on the main
thread while we wait for the synchronized start time.
Comment on attachment 132827 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132827&action=review A thing of beauty. @Enne. > Source/WebCore/platform/graphics/chromium/cc/CCAnimationEvents.cpp:-33 > -CCAnimationEvent::CCAnimationEvent(int layerId, CCActiveAnimation::TargetProperty targetProperty) No need for the CPP anymore, methinks? Just put the constructor in the .h, c.f. CCLayerTreeHost.h's CCSettings Comment on attachment 132827 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132827&action=review Drive by language question: is monotonic time a synonym for wall clock time here? > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:391 > + if (m_activeAnimations[i]->runState() == CCActiveAnimation::Running && !m_activeAnimations[i]->needsSynchronizedStartTime()) { > double trimmed = m_activeAnimations[i]->trimTimeToCurrentIteration(monotonicTime); > > + // Animation assumes its initial value until it gets the synchronized start time > + // from the impl thread and can start ticking. > + if (m_activeAnimations[i]->needsSynchronizedStartTime()) > + trimmed = 0; I don't understand this second conditional here. It looks like it will never be true, based on the first conditional. Created attachment 132950 [details] Patch (In reply to comment #3) CCAnimationEvents.cpp is toast. (In reply to comment #4) > (From update of attachment 132827 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=132827&action=review > > Drive by language question: is monotonic time a synonym for wall clock time here? > I've been using monotonicTime to refer to times obtained via monotonicallyIncreasingTime() and wallClockTime for times obtained using currentTime() > > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:391 > > + if (m_activeAnimations[i]->runState() == CCActiveAnimation::Running && !m_activeAnimations[i]->needsSynchronizedStartTime()) { > > double trimmed = m_activeAnimations[i]->trimTimeToCurrentIteration(monotonicTime); > > > > + // Animation assumes its initial value until it gets the synchronized start time > > + // from the impl thread and can start ticking. > > + if (m_activeAnimations[i]->needsSynchronizedStartTime()) > > + trimmed = 0; > > I don't understand this second conditional here. It looks like it will never be true, based on the first conditional. Good catch. The first conditional was meant to be deleted. Fixed. Comment on attachment 132950 [details]
Patch
Thanks for the language clarification. I must have misread some code to think that monotonic and wall clock were being interchangeably used.
R=me, mostly due to Nat's review.
Comment on attachment 132950 [details] Patch Clearing flags on attachment: 132950 Committed r111525: <http://trac.webkit.org/changeset/111525> All reviewed patches have been landed. Closing bug. |