Bug 83283 – [chromium] Ensure that animations continue to run when transform-style is changed

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

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

Created attachment 135833 [details]
Patch

Ensures that we do push running animation to the impl thread when necessary.

[Nat Duca]

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.

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.

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 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 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?

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.

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]

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 on attachment 139660 [details]
Patch

Patch itself, LGTM.

Created attachment 141199 [details]
Patch

Added layout test.

Created attachment 141210 [details]
Patch

Updating layout test so that there is no visible text.

[WebKit Review Bot]

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.

(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 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

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 on attachment 141331 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 141331 [details]
Patch

Clearing flags on attachment: 141331

Committed r116786: <http://trac.webkit.org/changeset/116786>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.