Bug 84601 – [chromium] Add pause and resume support for accelerated css animations.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

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

[Dana Jansens]

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.

[James Robinson]

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.

[James Robinson]

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.

[WebKit Review Bot]

Comment on attachment 139257 [details]
Patch

Clearing flags on attachment: 139257

Committed r115519: <http://trac.webkit.org/changeset/115519>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.