Need to implement suspendAnimations and resumeAnimations and make sure that the run state changes make it to the impl thread. This will be important for testing.
Created attachment 138357 [details] Patch
Comment on attachment 138357 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138357&action=review > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:626 > void LayerChromium::suspendAnimations(double time) Can you name the 'time' variable more descriptively, or otherwise make it more clear what the variable's base is? Or maybe the conversion below is more obvious to others
Created attachment 138371 [details] Patch (In reply to comment #2) > (From update of attachment 138357 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=138357&action=review > > > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:626 > > void LayerChromium::suspendAnimations(double time) > > Can you name the 'time' variable more descriptively, or otherwise make it more clear what the variable's base is? Or maybe the conversion below is more obvious to others Good point! I've renamed the parameters.
Comment on attachment 138371 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138371&action=review > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:626 > +void LayerChromium::suspendAnimations(double wallClockTime) why does this take a wall clock time? who's passing this in to us? if this is API we control, it should be using monotonicallyIncreasingTime. if this is being fed in by WebCore, i'd prefer to handle the time rebasing before it gets in to LayerChromium so we don't have to muck with it there > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:137 > + // Currently, we only push changes due to pausing and resuming animations on the main thread. could you explain why it's OK to delete the code that used to live here?
Created attachment 138820 [details] Patch (In reply to comment #4) > (From update of attachment 138371 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=138371&action=review > > > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:626 > > +void LayerChromium::suspendAnimations(double wallClockTime) > > why does this take a wall clock time? who's passing this in to us? > > if this is API we control, it should be using monotonicallyIncreasingTime. if this is being fed in by WebCore, i'd prefer to handle the time rebasing before it gets in to LayerChromium so we don't have to muck with it there > The time comes from WebCore. I've moved the rebasing out to GraphicsLayerChromium. > > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:137 > > + // Currently, we only push changes due to pausing and resuming animations on the main thread. > > could you explain why it's OK to delete the code that used to live here? The old code is never called. I've included its removal in several cl's but none of them have landed yet.
Created attachment 139253 [details] Patch Incorporates fixes for bugs I've found getting threaded animation in DRT. Noteworthy changes - CCActiveAnimations need to keep track of their time offset. This is used when resuming animations. - Suspend and Resume must cause the animation to pause at the given time and prevent any run state changes.
Comment on attachment 139253 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139253&action=review Great, R=me. Thanks for moving the time rebase - I'd like to keep as much of the compositor interface monotonic time as possible and keep WebCore-specific fiddly bits in GLC > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:48 > +#include <wtf/CurrentTime.h> > + can kill this now > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:145 > + // there is absolutely no need for clients of this controller to kmonotonicTime typo - looks like a bad search->replace of "now"->"monotonicTime"
Created attachment 139257 [details] Patch (In reply to comment #7) > (From update of attachment 139253 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=139253&action=review > > Great, R=me. Thanks for moving the time rebase - I'd like to keep as much of the compositor interface monotonic time as possible and keep WebCore-specific fiddly bits in GLC > > > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:48 > > +#include <wtf/CurrentTime.h> > > + > > can kill this now Done. > > > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.h:145 > > + // there is absolutely no need for clients of this controller to kmonotonicTime > > typo - looks like a bad search->replace of "now"->"monotonicTime" D'oh. Fixed.
Comment on attachment 139257 [details] Patch Clearing flags on attachment: 139257 Committed r115519: <http://trac.webkit.org/changeset/115519>
All reviewed patches have been landed. Closing bug.