Web Animations CSS: Support Animation of StyleImage and LengthBox

Web Animations CSS: Support Animation of StyleImage and LengthBox

This patch adds two new types for animation:
* AnimatableImage implements interpolation of images as defined by web-animations: http://dev.w3.org/fxtf/web-animations/#the--image--type
* AnimatableLengthBox implements interpolation for the LengthBox platform type. This matches the existing implementation with the exception that it allows interpolation between different length types.

Adds support and tests for interpolation of clip. Behavior changes slightly under
Web Animations as we step at 50% when smooth interpolation is not defined (eg. between auto
and 10px). The existing implementation steps at 0% (http://crbug.com/269451). Note that
neither the existing implementation or Web Animations yet support animating a clip when it is 
not set by some other style (http://crbug.com/279684).

A whitelist has been added to allow filtering of properties which do not currently support animation.

BUG=258896, 257591
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156846

[alancutter (OOO until 2018), 2013-08-27 02:37:45]

https://codereview.chromium.org/22799020/diff/19001/Source/core/animation/Ani...
File Source/core/animation/AnimatableLengthBox.h (right):

https://codereview.chromium.org/22799020/diff/19001/Source/core/animation/Ani...
Source/core/animation/AnimatableLengthBox.h:67: RefPtr<AnimatableValue>
m_bottom;
We may want to abstract this as AnimatableQuad in the future since it's storing
AnimatableValues. For now as its the only use case it's fine.

https://codereview.chromium.org/22799020/diff/19001/Source/core/animation/Ani...
File Source/core/animation/AnimatableValue.h (right):

https://codereview.chromium.org/22799020/diff/19001/Source/core/animation/Ani...
Source/core/animation/AnimatableValue.h:51: static PassRefPtr<AnimatableValue>
defaultAddWith(const AnimatableValue* left, const AnimatableValue* right) {
return takeConstRef(right); }
Is there a use for exposing these methods publicly?

https://codereview.chromium.org/22799020/diff/19001/Source/core/css/resolver/...
File Source/core/css/resolver/AnimatedStyleBuilder.cpp (right):

https://codereview.chromium.org/22799020/diff/19001/Source/core/css/resolver/...
Source/core/css/resolver/AnimatedStyleBuilder.cpp:68:
animatableValueToLength(animatableLengthBox->bottom(), state));
This will fail when any of these values are 'auto' though this is a fault of
animatableValueToLength() rather than animatableValueToLengthBox().

[Timothy Loh, 2013-08-27 04:26:06]

https://codereview.chromium.org/22799020/diff/19001/Source/core/animation/Ani...
File Source/core/animation/AnimatableImage.h (right):

https://codereview.chromium.org/22799020/diff/19001/Source/core/animation/Ani...
Source/core/animation/AnimatableImage.h:36: #include
"core/rendering/style/StyleGeneratedImage.h"
I think you only use this in the .cpp

https://codereview.chromium.org/22799020/diff/19001/Source/core/animation/Ani...
Source/core/animation/AnimatableImage.h:49: StyleImage* toStyleImage() const {
return m_image.get(); }
Maybe const StyleImage*?

https://codereview.chromium.org/22799020/diff/19001/Source/core/animation/Ani...
Source/core/animation/AnimatableImage.h:63: RefPtr<StyleImage> m_image;
Perhaps const RefPtr if possible.

https://codereview.chromium.org/22799020/diff/19001/Source/core/animation/Ani...
File Source/core/animation/AnimatableLengthBox.cpp (right):

https://codereview.chromium.org/22799020/diff/19001/Source/core/animation/Ani...
Source/core/animation/AnimatableLengthBox.cpp:36: PassRefPtr<AnimatableValue>
AnimatableLengthBox::interpolateTo(const AnimatableValue* other, double
fraction) const
tiny nit: I think we were using value as the name for this in the other
AnimatableValues?

https://codereview.chromium.org/22799020/diff/19001/Source/core/animation/Ani...
Source/core/animation/AnimatableLengthBox.cpp:38: const AnimatableLengthBox*
otherAnimatableLengthBox = toAnimatableLengthBox(other);
tiny nit continued: And this would be named lengthBox

https://codereview.chromium.org/22799020/diff/19001/Source/core/animation/Ani...
File Source/core/animation/AnimatableLengthBox.h (right):

https://codereview.chromium.org/22799020/diff/19001/Source/core/animation/Ani...
Source/core/animation/AnimatableLengthBox.h:67: RefPtr<AnimatableValue>
m_bottom;
On 2013/08/27 02:37:45, alancutter wrote:
> We may want to abstract this as AnimatableQuad in the future since it's
storing
> AnimatableValues. For now as its the only use case it's fine.

I don't think we want this as an AnimatableQuad, as per previous discussions.
We'll want the type distinction between different 4-tuples of AnimatableValues
(I don't think we'll have others anyway?)

https://codereview.chromium.org/22799020/diff/19001/Source/core/css/resolver/...
File Source/core/css/resolver/AnimatedStyleBuilder.cpp (right):

https://codereview.chromium.org/22799020/diff/19001/Source/core/css/resolver/...
Source/core/css/resolver/AnimatedStyleBuilder.cpp:68:
animatableValueToLength(animatableLengthBox->bottom(), state));
I think the logic for creating a LengthBox should be on the AnimatableLengthBox
itself (similar to toLength on AnimatableNumber), although this might be a bit
awkward given auto and whatnot?

[dstockwell, 2013-08-27 05:02:01]

https://codereview.chromium.org/22799020/diff/19001/Source/core/animation/Ani...
File Source/core/animation/AnimatableImage.h (right):

https://codereview.chromium.org/22799020/diff/19001/Source/core/animation/Ani...
Source/core/animation/AnimatableImage.h:36: #include
"core/rendering/style/StyleGeneratedImage.h"
On 2013/08/27 04:26:06, Timothy Loh wrote:
> I think you only use this in the .cpp

Done.

https://codereview.chromium.org/22799020/diff/19001/Source/core/animation/Ani...
Source/core/animation/AnimatableImage.h:49: StyleImage* toStyleImage() const {
return m_image.get(); }
On 2013/08/27 04:26:06, Timothy Loh wrote:
> Maybe const StyleImage*?

no viable conversion from 'const WebCore::StyleImage *' to
'PassRefPtr<WebCore::StyleImage>

https://codereview.chromium.org/22799020/diff/19001/Source/core/animation/Ani...
Source/core/animation/AnimatableImage.h:63: RefPtr<StyleImage> m_image;
On 2013/08/27 04:26:06, Timothy Loh wrote:
> Perhaps const RefPtr if possible.

Done.

https://codereview.chromium.org/22799020/diff/19001/Source/core/animation/Ani...
File Source/core/animation/AnimatableLengthBox.cpp (right):

https://codereview.chromium.org/22799020/diff/19001/Source/core/animation/Ani...
Source/core/animation/AnimatableLengthBox.cpp:36: PassRefPtr<AnimatableValue>
AnimatableLengthBox::interpolateTo(const AnimatableValue* other, double
fraction) const
On 2013/08/27 04:26:06, Timothy Loh wrote:
> tiny nit: I think we were using value as the name for this in the other
> AnimatableValues?

Done.

https://codereview.chromium.org/22799020/diff/19001/Source/core/animation/Ani...
Source/core/animation/AnimatableLengthBox.cpp:38: const AnimatableLengthBox*
otherAnimatableLengthBox = toAnimatableLengthBox(other);
On 2013/08/27 04:26:06, Timothy Loh wrote:
> tiny nit continued: And this would be named lengthBox

Done.

https://codereview.chromium.org/22799020/diff/19001/Source/core/animation/Ani...
File Source/core/animation/AnimatableLengthBox.h (right):

https://codereview.chromium.org/22799020/diff/19001/Source/core/animation/Ani...
Source/core/animation/AnimatableLengthBox.h:67: RefPtr<AnimatableValue>
m_bottom;
On 2013/08/27 02:37:45, alancutter wrote:
> We may want to abstract this as AnimatableQuad in the future since it's
storing
> AnimatableValues. For now as its the only use case it's fine.

Or Tuple, or List? Wait... :)

https://codereview.chromium.org/22799020/diff/19001/Source/core/animation/Ani...
File Source/core/animation/AnimatableValue.h (right):

https://codereview.chromium.org/22799020/diff/19001/Source/core/animation/Ani...
Source/core/animation/AnimatableValue.h:51: static PassRefPtr<AnimatableValue>
defaultAddWith(const AnimatableValue* left, const AnimatableValue* right) {
return takeConstRef(right); }
On 2013/08/27 02:37:45, alancutter wrote:
> Is there a use for exposing these methods publicly?

This was for AnimatableList, I'll revert it.

https://codereview.chromium.org/22799020/diff/19001/Source/core/css/resolver/...
File Source/core/css/resolver/AnimatedStyleBuilder.cpp (right):

https://codereview.chromium.org/22799020/diff/19001/Source/core/css/resolver/...
Source/core/css/resolver/AnimatedStyleBuilder.cpp:68:
animatableValueToLength(animatableLengthBox->bottom(), state));
On 2013/08/27 04:26:06, Timothy Loh wrote:
> I think the logic for creating a LengthBox should be on the
AnimatableLengthBox
> itself (similar to toLength on AnimatableNumber), although this might be a bit
> awkward given auto and whatnot?

I've updated animatableValueToLength for now.

[Timothy Loh, 2013-08-27 05:48:17]

https://codereview.chromium.org/22799020/diff/47001/LayoutTests/animations/in...
File LayoutTests/animations/interpolation/clip-interpolation-expected.txt
(right):

https://codereview.chromium.org/22799020/diff/47001/LayoutTests/animations/in...
LayoutTests/animations/interpolation/clip-interpolation-expected.txt:6: clip
from [rect(auto, auto, auto, 10px)] to [rect(20px, 50px, 50px, auto)] was
[rect(20px 50px 50px 0px)] at 0.4
Why is this right? Shouldn't we have rect(auto, auto, auto, 10px) here?
(Actually it looks like we don't handle getComputedStyle for auto components
correctly, so rect(0px, 0px, 0px, 10px)). Also we might want a test where there
are both unmatched (auto/px) pairs and matched (px/px) pairs.

https://codereview.chromium.org/22799020/diff/47001/Source/core/css/resolver/...
File Source/core/css/resolver/AnimatedStyleBuilder.cpp (right):

https://codereview.chromium.org/22799020/diff/47001/Source/core/css/resolver/...
Source/core/css/resolver/AnimatedStyleBuilder.cpp:58: if
(cssPrimitiveValue->isLength())
Looks wrong -- isLength returns false for keywords so we'd make a
Length(Undefined) here. We can just can convertToLength without checking, it'll
return Length(Undefined) if nothing matches (and we might want to assert it
doesn't return that here).

[Timothy Loh, 2013-08-27 05:59:53]

https://codereview.chromium.org/22799020/diff/47001/LayoutTests/animations/in...
File LayoutTests/animations/interpolation/clip-interpolation-expected.txt
(right):

https://codereview.chromium.org/22799020/diff/47001/LayoutTests/animations/in...
LayoutTests/animations/interpolation/clip-interpolation-expected.txt:6: clip
from [rect(auto, auto, auto, 10px)] to [rect(20px, 50px, 50px, auto)] was
[rect(20px 50px 50px 0px)] at 0.4
Also we probably want to test animating from auto to rect(...) and vice versa. I
suspect we may be doing the wrong thing in these cases since we aren't calling
setHasClip anywhere here (the StyleBuilder should setHasClip(false) when you
pass it auto, but we won't be setting it to true when we setClip(...) after the
50% step).

[dstockwell, 2013-08-27 06:26:23]

https://codereview.chromium.org/22799020/diff/47001/LayoutTests/animations/in...
File LayoutTests/animations/interpolation/clip-interpolation-expected.txt
(right):

https://codereview.chromium.org/22799020/diff/47001/LayoutTests/animations/in...
LayoutTests/animations/interpolation/clip-interpolation-expected.txt:6: clip
from [rect(auto, auto, auto, 10px)] to [rect(20px, 50px, 50px, auto)] was
[rect(20px 50px 50px 0px)] at 0.4
On 2013/08/27 05:48:17, Timothy Loh wrote:
> Why is this right? Shouldn't we have rect(auto, auto, auto, 10px) here?

This is just the expectation, it's not right. The existing implementation steps
at 0% not 50%.

Our results are in the virtual/web-animations-css directory.

> (Actually it looks like we don't handle getComputedStyle for auto components
> correctly, so rect(0px, 0px, 0px, 10px)). Also we might want a test where
there
> are both unmatched (auto/px) pairs and matched (px/px) pairs.

Done.

https://codereview.chromium.org/22799020/diff/47001/LayoutTests/animations/in...
LayoutTests/animations/interpolation/clip-interpolation-expected.txt:6: clip
from [rect(auto, auto, auto, 10px)] to [rect(20px, 50px, 50px, auto)] was
[rect(20px 50px 50px 0px)] at 0.4
On 2013/08/27 05:59:54, Timothy Loh wrote:
> Also we probably want to test animating from auto to rect(...) and vice versa.
I
> suspect we may be doing the wrong thing in these cases since we aren't calling
> setHasClip anywhere here (the StyleBuilder should setHasClip(false) when you
> pass it auto, but we won't be setting it to true when we setClip(...) after
the
> 50% step).

That's why I was setting the clip: property on all elements ahead of the
animation. I've added a test for this behavior. We currently match the existing
implementation but should be able to fix both at the same time.

[Steve Block, 2013-08-28 03:44:23]

lgtm

https://codereview.chromium.org/22799020/diff/40001/LayoutTests/animations/in...
File LayoutTests/animations/interpolation/clip-interpolation.html (right):

https://codereview.chromium.org/22799020/diff/40001/LayoutTests/animations/in...
LayoutTests/animations/interpolation/clip-interpolation.html:54: });
It would be good to test interpolation to/from 'auto' at fractions outside
[0,1].

https://codereview.chromium.org/22799020/diff/40001/LayoutTests/virtual/web-a...
File
LayoutTests/virtual/web-animations-css/animations/interpolation/clip-interpolation-expected.txt
(right):

https://codereview.chromium.org/22799020/diff/40001/LayoutTests/virtual/web-a...
LayoutTests/virtual/web-animations-css/animations/interpolation/clip-interpolation-expected.txt:12:

Could you mention in the change description that Web Animations does these
'correctly' and that the primary expected result documents current behavior.

https://codereview.chromium.org/22799020/diff/40001/Source/core/animation/Ani...
File Source/core/animation/AnimatableImage.h (right):

https://codereview.chromium.org/22799020/diff/40001/Source/core/animation/Ani...
Source/core/animation/AnimatableImage.h:73: #endif // AnimatableNumber_h
AnimatableImage_h

https://codereview.chromium.org/22799020/diff/40001/Source/core/animation/Ani...
File Source/core/animation/AnimatableLengthBox.h (right):

https://codereview.chromium.org/22799020/diff/40001/Source/core/animation/Ani...
Source/core/animation/AnimatableLengthBox.h:78: #endif // AnimatableNumber_h
AnimatableLengthBox_h

https://codereview.chromium.org/22799020/diff/40001/Source/core/css/resolver/...
File Source/core/css/resolver/AnimatedStyleBuilder.cpp (right):

https://codereview.chromium.org/22799020/diff/40001/Source/core/css/resolver/...
Source/core/css/resolver/AnimatedStyleBuilder.cpp:196:
RELEASE_ASSERT_WITH_MESSAGE(!CSSAnimations::isAnimatableProperty(property),
"Unable to apply AnimatableValue to RenderStyle, not yet implemented: %s",
getPropertyNameString(property).utf8().data());
We should prefix the message with 'Web Animations not yet implemented:'

[dstockwell, 2013-08-28 08:21:21]

https://codereview.chromium.org/22799020/diff/40001/LayoutTests/animations/in...
File LayoutTests/animations/interpolation/clip-interpolation.html (right):

https://codereview.chromium.org/22799020/diff/40001/LayoutTests/animations/in...
LayoutTests/animations/interpolation/clip-interpolation.html:54: });
On 2013/08/28 03:44:23, Steve Block wrote:
> It would be good to test interpolation to/from 'auto' at fractions outside
> [0,1].

Done.

https://codereview.chromium.org/22799020/diff/40001/LayoutTests/virtual/web-a...
File
LayoutTests/virtual/web-animations-css/animations/interpolation/clip-interpolation-expected.txt
(right):

https://codereview.chromium.org/22799020/diff/40001/LayoutTests/virtual/web-a...
LayoutTests/virtual/web-animations-css/animations/interpolation/clip-interpolation-expected.txt:12:

On 2013/08/28 03:44:23, Steve Block wrote:
> Could you mention in the change description that Web Animations does these
> 'correctly' and that the primary expected result documents current behavior.

Done.

https://codereview.chromium.org/22799020/diff/40001/Source/core/animation/Ani...
File Source/core/animation/AnimatableImage.h (right):

https://codereview.chromium.org/22799020/diff/40001/Source/core/animation/Ani...
Source/core/animation/AnimatableImage.h:73: #endif // AnimatableNumber_h
On 2013/08/28 03:44:23, Steve Block wrote:
> AnimatableImage_h

Done.

https://codereview.chromium.org/22799020/diff/40001/Source/core/animation/Ani...
File Source/core/animation/AnimatableLengthBox.h (right):

https://codereview.chromium.org/22799020/diff/40001/Source/core/animation/Ani...
Source/core/animation/AnimatableLengthBox.h:78: #endif // AnimatableNumber_h
On 2013/08/28 03:44:23, Steve Block wrote:
> AnimatableLengthBox_h

Done.

https://codereview.chromium.org/22799020/diff/40001/Source/core/css/resolver/...
File Source/core/css/resolver/AnimatedStyleBuilder.cpp (right):

https://codereview.chromium.org/22799020/diff/40001/Source/core/css/resolver/...
Source/core/css/resolver/AnimatedStyleBuilder.cpp:196:
RELEASE_ASSERT_WITH_MESSAGE(!CSSAnimations::isAnimatableProperty(property),
"Unable to apply AnimatableValue to RenderStyle, not yet implemented: %s",
getPropertyNameString(property).utf8().data());
On 2013/08/28 03:44:23, Steve Block wrote:
> We should prefix the message with 'Web Animations not yet implemented:'

Done.

[Timothy Loh, 2013-08-28 08:26:55]

lgtm

https://codereview.chromium.org/22799020/diff/69001/LayoutTests/animations/in...
File LayoutTests/animations/interpolation/clip-interpolation.html (right):

https://codereview.chromium.org/22799020/diff/69001/LayoutTests/animations/in...
LayoutTests/animations/interpolation/clip-interpolation.html:74: {at: -1, is:
'rect(0px, 0px, 0px, 0px)'},
I think these should expect auto?

[commit-bot: I haz the power, 2013-08-28 08:28:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/22799020/69001

[dstockwell, 2013-08-28 11:35:29]

https://codereview.chromium.org/22799020/diff/69001/LayoutTests/animations/in...
File LayoutTests/animations/interpolation/clip-interpolation.html (right):

https://codereview.chromium.org/22799020/diff/69001/LayoutTests/animations/in...
LayoutTests/animations/interpolation/clip-interpolation.html:74: {at: -1, is:
'rect(0px, 0px, 0px, 0px)'},
On 2013/08/28 08:26:55, Timothy Loh wrote:
> I think these should expect auto?

Done.

[commit-bot: I haz the power, 2013-08-28 11:35:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/22799020/94001

[commit-bot: I haz the power, 2013-08-28 11:38:53]

Failed to trigger a try job on win_layout
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-08-28 11:39:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/22799020/57001

[commit-bot: I haz the power, 2013-08-28 13:00:08]

Change committed as 156846