WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
84601
[chromium] Add pause and resume support for accelerated css animations.
https://bugs.webkit.org/show_bug.cgi?id=84601
Summary
[chromium] Add pause and resume support for accelerated css animations.
vollick
Reported
2012-04-23 08:46:06 PDT
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.
Attachments
Patch
(15.06 KB, patch)
2012-04-23 08:56 PDT
,
vollick
no flags
Details
Formatted Diff
Diff
Patch
(15.48 KB, patch)
2012-04-23 10:07 PDT
,
vollick
no flags
Details
Formatted Diff
Diff
Patch
(18.99 KB, patch)
2012-04-25 08:54 PDT
,
vollick
no flags
Details
Formatted Diff
Diff
Patch
(31.91 KB, patch)
2012-04-27 13:08 PDT
,
vollick
no flags
Details
Formatted Diff
Diff
Patch
(31.59 KB, patch)
2012-04-27 13:22 PDT
,
vollick
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
vollick
Comment 1
2012-04-23 08:56:39 PDT
Created
attachment 138357
[details]
Patch
Dana Jansens
Comment 2
2012-04-23 10:01:14 PDT
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
vollick
Comment 3
2012-04-23 10:07:17 PDT
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 4
2012-04-24 23:45:02 PDT
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?
vollick
Comment 5
2012-04-25 08:54:44 PDT
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.
vollick
Comment 6
2012-04-27 13:08:40 PDT
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 7
2012-04-27 13:12:57 PDT
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"
vollick
Comment 8
2012-04-27 13:22:13 PDT
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 9
2012-04-27 17:07:05 PDT
Comment on
attachment 139257
[details]
Patch Clearing flags on attachment: 139257 Committed
r115519
: <
http://trac.webkit.org/changeset/115519
>
WebKit Review Bot
Comment 10
2012-04-27 17:07:13 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug