Bug 77662 - [chromium] Add impl-thread support for fill-mode and direction css animation properties
Summary: [chromium] Add impl-thread support for fill-mode and direction css animation ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: vollick
URL:
Keywords:
Depends on: 77646 79819
Blocks: 85813
  Show dependency treegraph
 
Reported: 2012-02-02 11:38 PST by vollick
Modified: 2012-05-09 13:26 PDT (History)
4 users (show)

See Also:


Attachments
Patch (10.45 KB, patch)
2012-04-03 10:23 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (17.78 KB, patch)
2012-05-09 08:25 PDT, vollick
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description vollick 2012-02-02 11:38:08 PST
There are css animation properties that aren’t yet supported by the cc-thread animation code. If we detect an unsupported feature, it is possible to return false in GraphicsLayer::addAnimation to revert to the usual, main-thread animation code, but we should support most/all css animation properties. Specifically: animation-direction, animation-delay, animation-timing-function, and animation-fill-mode.
Comment 1 vollick 2012-03-15 19:49:33 PDT
This should not block turning the feature on by default, since we gracefully degrade.
Comment 2 vollick 2012-04-03 10:23:15 PDT
Created attachment 135354 [details]
Patch
Comment 3 James Robinson 2012-04-03 11:15:01 PDT
Comment on attachment 135354 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135354&action=review

> Source/WebCore/ChangeLog:3
> +        [chromium] Add impl-thread support for all css animation properties.

I think "all" is a bit ambitious. Can you say specifically what this patch is adding support for?

> Source/WebKit/chromium/src/TextFieldDecoratorImpl.cpp:36
> +#include "Image.h"

this looks unrelated

> Source/WebKit/chromium/tests/CCActiveAnimationTest.cpp:72
> +TEST(CCActiveAnimationTest, TrimTimeAlternating)

is this sufficient test coverage?  should we have some tests for reverse as well?
Comment 4 vollick 2012-05-09 08:25:10 PDT
Created attachment 140950 [details]
Patch

(In reply to comment #3)
> (From update of attachment 135354 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135354&action=review
>
> > Source/WebCore/ChangeLog:3
> > +        [chromium] Add impl-thread support for all css animation properties.
>
> I think "all" is a bit ambitious. Can you say specifically what this patch is adding support for?
Done.
>
> > Source/WebKit/chromium/src/TextFieldDecoratorImpl.cpp:36
> > +#include "Image.h"
>
> this looks unrelated
Removed.
>
> > Source/WebKit/chromium/tests/CCActiveAnimationTest.cpp:72
> > +TEST(CCActiveAnimationTest, TrimTimeAlternating)
>
> is this sufficient test coverage?  should we have some tests for reverse as well?
It isn't. Added tests to cover reverse.
Comment 5 James Robinson 2012-05-09 11:47:35 PDT
Comment on attachment 140950 [details]
Patch

Cool, R=me. Thanks for the extra tests
Comment 6 WebKit Review Bot 2012-05-09 13:26:18 PDT
Comment on attachment 140950 [details]
Patch

Clearing flags on attachment: 140950

Committed r116554: <http://trac.webkit.org/changeset/116554>
Comment 7 WebKit Review Bot 2012-05-09 13:26:22 PDT
All reviewed patches have been landed.  Closing bug.