Bug 94993 – RenderLayerBacking::startAnimation() return value has incorrect semantics

• Component: Layout and Rendering
• Status: NEW

[Shawn Singh]

The return value of RenderLayerBacking::startAnimation(...) returns true if *any* accelerated animation was accepted by GraphicsLayer.  However, outside of this code, the return value seems to be used to indicate if *all* accelerated animations were correctly accepted by GraphicsLayer.

From what I have seen, I think it's appropriate to fix the semantics of RenderLayerBacking return value.  The other option would be to change the animation infrastructure so that we can support a keyframed animation that has some properties updated by non-accelerated code (e.g. a rejected transform animation) and other properties simultaneously updated by accelerated code (e.g. an accepted opacity animation).  That approach seems too heavy-handed and error-prone, maybe not even practical.  Furthermore, this bug occurs only when chromium or safari reject one property of an animation (i.e. an unsupported transform), and that doesn't happen normally.  As I understand, most other "rejections" would occur for all properties of the same keyframed animation, so this issue wouldn't occur.

An example of the break is the attachment available at http://code.google.com/p/chromium/issues/detail?id=143658

This breaks in BOTH chromium and safari -- but to demonstrate the break in safari, I hacked GraphicsLayerCA to reject the transform animation without rejecting the opacity animation.

For chromium specifically, vollick@ is also working on issue 94860, so that almost all transform animations are accepted by chromium, much like Safari is already accepting most transform animations.   We agreed this should probably land before that bug, so that we can more easily verify this patch works as intended.

[Shawn Singh]

Created attachment 160534 [details]
Proposed patch, without layout test and changelogs yet

[Shawn Singh]

Created attachment 160815 [details]
Patch with layout test

Dear Reviewers, the provided layout test is fully correct, however, it does not actually reproduce the original bug until accelerated animations are enabled for chromium port of DumpRenderTree. For Safari, the original bug does not occur until I programmatically forced big rotations to be rejected (by commenting out the PlatformCAAnimation::supportsValueFunction() in GraphicsLayerCA.cpp:1874). For Chrome, the bug reproduces in the new layout test when checking manually, and when accelerated animations for DRT are enabled, this test will be the correct scenario.  I also tested locally on Safari and Chrome layout tests, no obvious regressions, and it passes unit tests and manual testing.  With your approval I would like to go ahead and land this patch, because it will unfortunately be some time more before accelerated animations are enabled for chromium DRT.

[WebKit Review Bot]

Comment on attachment 160815 [details]
Patch with layout test

Attachment 160815 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13638112

New failing tests:
compositing/animation/two-properties-in-keyframed-animation.html

[WebKit Review Bot]

Created attachment 160870 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Shawn Singh]

Created attachment 160914 [details]
same patch, should fix linux

[Simon Fraser (smfr)]

I'd like dino's input here. It's not clear to me that if one simultaneous animation has to be run in software, then they all do.

[Dean Jackson]

I'll review this.

[Shawn Singh]

We left this discussion unresolved several months ago;  I think it's better for me to leave this bug open and let WebKit folks close it if they wish.  That way, if it is a valid issue, it won't get lost.  Thanks!