WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
83283
[chromium] Ensure that animations continue to run when transform-style is changed
https://bugs.webkit.org/show_bug.cgi?id=83283
Summary
[chromium] Ensure that animations continue to run when transform-style is cha...
vollick
Reported
2012-04-05 08:32:09 PDT
The current sync logic avoids pushing running animations to the impl thread because they should already be there. However, if the impl thread layer tree has been recreated (due to being backgrounded/unbackgrounded), then we do need to push the running animations across to the impl thread.h
Attachments
Patch
(9.45 KB, patch)
2012-04-05 08:34 PDT
,
vollick
no flags
Details
Formatted Diff
Diff
Patch
(10.72 KB, patch)
2012-04-05 21:11 PDT
,
vollick
no flags
Details
Formatted Diff
Diff
Patch
(16.76 KB, patch)
2012-04-11 16:08 PDT
,
vollick
no flags
Details
Formatted Diff
Diff
Patch
(16.09 KB, patch)
2012-04-30 07:25 PDT
,
vollick
no flags
Details
Formatted Diff
Diff
Patch
(20.97 KB, patch)
2012-05-01 12:53 PDT
,
vollick
no flags
Details
Formatted Diff
Diff
Patch
(30.71 KB, patch)
2012-05-10 10:59 PDT
,
vollick
no flags
Details
Formatted Diff
Diff
Patch
(27.41 KB, patch)
2012-05-10 11:42 PDT
,
vollick
no flags
Details
Formatted Diff
Diff
Patch
(21.11 KB, patch)
2012-05-10 22:56 PDT
,
vollick
no flags
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
vollick
Comment 1
2012-04-05 08:34:58 PDT
Created
attachment 135833
[details]
Patch Ensures that we do push running animation to the impl thread when necessary.
Nat Duca
Comment 2
2012-04-05 09:57:51 PDT
Comment on
attachment 135833
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=135833&action=review
> Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:-135 > -void CCActiveAnimation::synchronizeProperties(CCActiveAnimation* other)
not used any longer?
> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:-250 > - if (!m_activeAnimations[i]->needsSynchronizedStartTime())
Hoist m_activeAnimations[i] to a temp var. Possibly, move the body of this loop to CCActiveAnimation where pushProps used to was? Not sure... but this all feels like animation "push" logic.
> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:252 > + if (controllerImpl->m_initialized && !m_activeAnimations[i]->needsSynchronizedStartTime())
Can you come up with a better name. ActivatedOnImplThread? Having a hard time understanding why this is conditional on initialize. Document why.
> Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:258 > + // If controllerImpl is uninitialized and the animation does not need a synchronized start
Again, unitialized isn't conveying purpose here. This whole comment block really breaks my head. Your comment text says if (!uninitialized) but then your branch is actually the reverse, which makes it hard to associate the explanation with the code.
vollick
Comment 3
2012-04-05 21:11:09 PDT
Created
attachment 135977
[details]
Patch Ensures that we do push running animation to the impl thread when necessary.(In reply to
comment #2
)
> (From update of
attachment 135833
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=135833&action=review
>
> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:-135 > > -void CCActiveAnimation::synchronizeProperties(CCActiveAnimation* other)
>
> not used any longer?
Nope, it's dead code. >
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:-250 > > - if (!m_activeAnimations[i]->needsSynchronizedStartTime())
>
> Hoist m_activeAnimations[i] to a temp var.
Done. >
> Possibly, move the body of this loop to CCActiveAnimation where pushProps used to was? Not sure... but this all feels like animation "push" logic.
Hmm. I can't see a nice way of doing that. The function would conditionally create the new animation and return it and we would add it to the impl thread controller in this loop? >
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:252 > > + if (controllerImpl->m_initialized && !m_activeAnimations[i]->needsSynchronizedStartTime())
>
> Can you come up with a better name. ActivatedOnImplThread?
I renamed it m_hasSynchronizedWithMainThread since it's meant to indicate that we've done at least one sync. >
> Having a hard time understanding why this is conditional on initialize. Document why.
Updated the comment. >
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:258 > > + // If controllerImpl is uninitialized and the animation does not need a synchronized start
>
> Again, unitialized isn't conveying purpose here. This whole comment block really breaks my head. Your comment text says if (!uninitialized) but then your branch is actually the reverse, which makes it hard to associate the explanation with the code.
Renamed the variable and updated the comment.
vollick
Comment 4
2012-04-11 16:08:57 PDT
Created
attachment 136777
[details]
Patch Added code to ensure that when we switch the primary layer in GraphicsLayerChromium, we also transfer the layer animation controller and all its animations (and force a transfer of all animations to the cc thread).
James Robinson
Comment 5
2012-04-11 16:22:44 PDT
Comment on
attachment 136777
[details]
Patch This is a case where I think having layout test coverage (in addition to the unit test) makes sense since this layer transferring behavior is pretty particular to how the GraphicsLayer interface works. Do we run layout tests with this animation path on currently? What do we need to do to get there?
Nat Duca
Comment 6
2012-04-11 18:06:39 PDT
Comment on
attachment 136777
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=136777&action=review
+1 to layout test to test this case. Do we need something else that tests the transfer code?
> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:589 > + m_transformLayer->layerAnimationController()->setForceSync();
is this forceSync required? Does it make sense for setting an animation controller to internally force sync?
vollick
Comment 7
2012-04-30 07:25:25 PDT
Created
attachment 139450
[details]
Patch (In reply to
comment #6
)
> (From update of
attachment 136777
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=136777&action=review
>
> +1 to layout test to test this case. Do we need something else that tests the transfer code?
Would it be acceptable to submit the layout test in a separate CL? Layout tests with threaded animation have been delayed. >
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:589 > > + m_transformLayer->layerAnimationController()->setForceSync();
>
> is this forceSync required? Does it make sense for setting an animation controller to internally force sync?
Done.
vollick
Comment 8
2012-05-01 12:53:51 PDT
Created
attachment 139660
[details]
Patch I've added another unit test to cover the GraphicsLayerChromium portion of the CL. Would this be acceptable in place of a layout test?
Nat Duca
Comment 9
2012-05-02 00:09:22 PDT
Sorry, I agree with james here. I think you need to do a real layout test... Look at the animation layout tests for examples... I know its different feeling than layout testing, but all you're trying to do is come up with a reduced html version of the thing that failed on us originally, and then check it in as a layout test.
Nat Duca
Comment 10
2012-05-02 00:10:47 PDT
Comment on
attachment 139660
[details]
Patch Patch itself, LGTM.
vollick
Comment 11
2012-05-10 10:59:31 PDT
Created
attachment 141199
[details]
Patch Added layout test.
vollick
Comment 12
2012-05-10 11:42:06 PDT
Created
attachment 141210
[details]
Patch Updating layout test so that there is no visible text.
WebKit Review Bot
Comment 13
2012-05-10 11:46:44 PDT
Attachment 141210
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/anim..." exit_code: 1 LayoutTests/platform/chromium/test_expectations.txt:3891: More specific entry on line 2976 overrides line 3891 fast/images/color-jpeg-with-color-profile.html [test/expectations] [5] LayoutTests/platform/chromium/test_expectations.txt:3927: Duplicate or ambiguous expectation. tables/mozilla/bugs/bug1188.html [test/expectations] [5] Total errors found: 2 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
vollick
Comment 14
2012-05-10 11:58:40 PDT
(In reply to
comment #13
)
>
Attachment 141210
[details]
did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/anim..." exit_code: 1 > LayoutTests/platform/chromium/test_expectations.txt:3891: More specific entry on line 2976 overrides line 3891 fast/images/color-jpeg-with-color-profile.html [test/expectations] [5] > LayoutTests/platform/chromium/test_expectations.txt:3927: Duplicate or ambiguous expectation. tables/mozilla/bugs/bug1188.html [test/expectations] [5] > Total errors found: 2 in 15 files > > > If any of these errors are false positives, please file a bug against check-webkit-style.
These aren't my changes (double checked the diff). Not sure what's making the style bot unhappy here.
James Robinson
Comment 15
2012-05-10 17:47:12 PDT
Comment on
attachment 141210
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=141210&action=review
> LayoutTests/animations/change-transform-style-during-animation-expected.txt:1 > +layer at (0,0) size 800x600
we shouldn't have a render tree dump on this - use layoutTestController.dumpAsText(true) to suppress this output and have the output just be the textContent of the document (i.e. the string "PASS - ..."
> LayoutTests/platform/chromium/test_expectations.txt:3886 > +// Will need to rebaseline once results are generated for these platforms
if you fix the test to output only text+pixels instead of a render tree you won't need this
vollick
Comment 16
2012-05-10 22:56:26 PDT
Created
attachment 141331
[details]
Patch There was some logic added to CCLayerAnimationController to handle the case where we've dropped the impl tree. Since this is no longer done, I've removed this code. (In reply to
comment #15
)
> (From update of
attachment 141210
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=141210&action=review
>
> > LayoutTests/animations/change-transform-style-during-animation-expected.txt:1 > > +layer at (0,0) size 800x600
>
> we shouldn't have a render tree dump on this - use layoutTestController.dumpAsText(true) to suppress this output and have the output just be the textContent of the document (i.e. the string "PASS - ..."
Done. >
> > LayoutTests/platform/chromium/test_expectations.txt:3886 > > +// Will need to rebaseline once results are generated for these platforms
>
> if you fix the test to output only text+pixels instead of a render tree you won't need this
Removed.
James Robinson
Comment 17
2012-05-11 10:34:07 PDT
Comment on
attachment 141331
[details]
Patch R=me
WebKit Review Bot
Comment 18
2012-05-11 11:03:31 PDT
Comment on
attachment 141331
[details]
Patch Clearing flags on attachment: 141331 Committed
r116786
: <
http://trac.webkit.org/changeset/116786
>
WebKit Review Bot
Comment 19
2012-05-11 11:03:37 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