Animation: FilterStyleInterpolation for animating filter

Animation: FilterStyleInterpolation for animating filter

-webkit-filter is now interpolated using FilterStyleInterpolation.

Interpolation of shadows using interpolable values is not yet
implemented. For now, we fall back to AnimatableFilterValue when
any of the filters is a drop-shadow.

Credit to jadeg for unsubmitted implementation 869823002.

BUG=451242
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194788

[Eric Willigers, 2015-04-08 07:53:48]

ericwilligers@chromium.org changed reviewers:
+ alancutter@chromium.org

[Timothy Loh, 2015-04-08 07:59:43]

https://codereview.chromium.org/1069313002/diff/1/Source/core/animation/Filte...
File Source/core/animation/FilterStyleInterpolation.cpp (right):

https://codereview.chromium.org/1069313002/diff/1/Source/core/animation/Filte...
Source/core/animation/FilterStyleInterpolation.cpp:31: ASSERT_NOT_REACHED();
Do we need to have this defensive check if it'll never get hit?
toCSSFunctionValue already has an assertion

https://codereview.chromium.org/1069313002/diff/1/Source/core/animation/Filte...
Source/core/animation/FilterStyleInterpolation.cpp:86: return nullptr;
Not sure about the interpolation coding style but I think this is unreachable?

https://codereview.chromium.org/1069313002/diff/1/Source/core/animation/Filte...
Source/core/animation/FilterStyleInterpolation.cpp:94: return nullptr;
and this

https://codereview.chromium.org/1069313002/diff/1/Source/core/animation/Filte...
Source/core/animation/FilterStyleInterpolation.cpp:108: return
start.isFunctionValue() && end.isFunctionValue()
I think these are always functionvalues?

https://codereview.chromium.org/1069313002/diff/1/Source/core/css/CSSValueList.h
File Source/core/css/CSSValueList.h (right):

https://codereview.chromium.org/1069313002/diff/1/Source/core/css/CSSValueLis...
Source/core/css/CSSValueList.h:33: using const_iterator =
WillBeHeapVector<RefPtrWillBeMember<CSSValue>, 4>::const_iterator;
needs a rebase? I think we have these already

[Eric Willigers, 2015-04-08 08:30:40]

https://codereview.chromium.org/1069313002/diff/1/Source/core/animation/Filte...
File Source/core/animation/FilterStyleInterpolation.cpp (right):

https://codereview.chromium.org/1069313002/diff/1/Source/core/animation/Filte...
Source/core/animation/FilterStyleInterpolation.cpp:31: ASSERT_NOT_REACHED();
On 2015/04/08 07:59:43, Timothy Loh wrote:
> Do we need to have this defensive check if it'll never get hit?
> toCSSFunctionValue already has an assertion

Acknowledged.

https://codereview.chromium.org/1069313002/diff/1/Source/core/animation/Filte...
Source/core/animation/FilterStyleInterpolation.cpp:86: return nullptr;
On 2015/04/08 07:59:43, Timothy Loh wrote:
> Not sure about the interpolation coding style but I think this is unreachable?

I'm allowing for 'inherit' and 'initial'.

https://codereview.chromium.org/1069313002/diff/1/Source/core/animation/Filte...
Source/core/animation/FilterStyleInterpolation.cpp:108: return
start.isFunctionValue() && end.isFunctionValue()
On 2015/04/08 07:59:43, Timothy Loh wrote:
> I think these are always functionvalues?

Acknowledged.

https://codereview.chromium.org/1069313002/diff/1/Source/core/css/CSSValueList.h
File Source/core/css/CSSValueList.h (right):

https://codereview.chromium.org/1069313002/diff/1/Source/core/css/CSSValueLis...
Source/core/css/CSSValueList.h:33: using const_iterator =
WillBeHeapVector<RefPtrWillBeMember<CSSValue>, 4>::const_iterator;
On 2015/04/08 07:59:43, Timothy Loh wrote:
> needs a rebase? I think we have these already

Done.

[alancutter (OOO until 2018), 2015-04-30 04:55:44]

https://codereview.chromium.org/1069313002/diff/20001/Source/core/animation/F...
File Source/core/animation/FilterStyleInterpolation.cpp (right):

https://codereview.chromium.org/1069313002/diff/20001/Source/core/animation/F...
Source/core/animation/FilterStyleInterpolation.cpp:16: PassRefPtr<CSSValueList>
extendFilterList(const CSSValue& shortFilterValue, const CSSValue&
otherFilterValue)
These parameters can be CSSValueLists.

https://codereview.chromium.org/1069313002/diff/20001/Source/core/animation/F...
Source/core/animation/FilterStyleInterpolation.cpp:56: return nullptr;
ASSERT_NOT_REACHED here.

https://codereview.chromium.org/1069313002/diff/20001/Source/core/animation/F...
Source/core/animation/FilterStyleInterpolation.cpp:66:
PassRefPtrWillBeRawPtr<StyleInterpolation>
FilterStyleInterpolation::maybeCreate(const CSSValue& start, const CSSValue&
end, CSSPropertyID property)
The return type can be more explicit about what gets returned
(ListStyleInterpolation<FilterStyleInterpolation>). Since a different class is
returned this should be called maybeCreateList().

https://codereview.chromium.org/1069313002/diff/20001/Source/core/animation/F...
Source/core/animation/FilterStyleInterpolation.cpp:81: return nullptr; //
inherit or initial
This comment should be an ASSERT, here and below.

https://codereview.chromium.org/1069313002/diff/20001/Source/core/animation/F...
Source/core/animation/FilterStyleInterpolation.cpp:104:
ASSERT(end.isFunctionValue());
No need to ASSERT when calling the cast function.

https://codereview.chromium.org/1069313002/diff/20001/Source/core/animation/F...
Source/core/animation/FilterStyleInterpolation.cpp:127: result->set(i,
InterpolableNumber::create(toCSSPrimitiveValue(filterValue.item(i))->getDoubleValue()));
I suspect you'll need to collapse units into a canonical form. Please make sure
different units are tested.

[Eric Willigers, 2015-04-30 06:43:19]

ericwilligers@chromium.org changed reviewers:
+ timloh@chromium.org

[Eric Willigers, 2015-04-30 06:43:23]

https://codereview.chromium.org/1069313002/diff/20001/Source/core/animation/F...
File Source/core/animation/FilterStyleInterpolation.cpp (right):

https://codereview.chromium.org/1069313002/diff/20001/Source/core/animation/F...
Source/core/animation/FilterStyleInterpolation.cpp:16: PassRefPtr<CSSValueList>
extendFilterList(const CSSValue& shortFilterValue, const CSSValue&
otherFilterValue)
On 2015/04/30 04:55:44, alancutter wrote:
> These parameters can be CSSValueLists.

Done.

https://codereview.chromium.org/1069313002/diff/20001/Source/core/animation/F...
Source/core/animation/FilterStyleInterpolation.cpp:56: return nullptr;
On 2015/04/30 04:55:44, alancutter wrote:
> ASSERT_NOT_REACHED here.

Done.

https://codereview.chromium.org/1069313002/diff/20001/Source/core/animation/F...
Source/core/animation/FilterStyleInterpolation.cpp:66:
PassRefPtrWillBeRawPtr<StyleInterpolation>
FilterStyleInterpolation::maybeCreate(const CSSValue& start, const CSSValue&
end, CSSPropertyID property)
On 2015/04/30 04:55:44, alancutter wrote:
> The return type can be more explicit about what gets returned
> (ListStyleInterpolation<FilterStyleInterpolation>). Since a different class is
> returned this should be called maybeCreateList().

Done.

https://codereview.chromium.org/1069313002/diff/20001/Source/core/animation/F...
Source/core/animation/FilterStyleInterpolation.cpp:81: return nullptr; //
inherit or initial
On 2015/04/30 04:55:44, alancutter wrote:
> This comment should be an ASSERT, here and below.

Done.

https://codereview.chromium.org/1069313002/diff/20001/Source/core/animation/F...
Source/core/animation/FilterStyleInterpolation.cpp:104:
ASSERT(end.isFunctionValue());
On 2015/04/30 04:55:44, alancutter wrote:
> No need to ASSERT when calling the cast function.

Done.

https://codereview.chromium.org/1069313002/diff/20001/Source/core/animation/F...
Source/core/animation/FilterStyleInterpolation.cpp:127: result->set(i,
InterpolableNumber::create(toCSSPrimitiveValue(filterValue.item(i))->getDoubleValue()));
On 2015/04/30 04:55:44, alancutter wrote:
> I suspect you'll need to collapse units into a canonical form. Please make
sure
> different units are tested.

Done.

[alancutter (OOO until 2018), 2015-05-01 01:26:09]

lgtm

https://codereview.chromium.org/1069313002/diff/60001/Source/core/animation/F...
File Source/core/animation/FilterStyleInterpolation.cpp (right):

https://codereview.chromium.org/1069313002/diff/60001/Source/core/animation/F...
Source/core/animation/FilterStyleInterpolation.cpp:132: default:
default should be an ASSERT_NOT_REACHED case and cases that are handled should
be listed explicitly. Similar to fromInterpolableValue().

https://codereview.chromium.org/1069313002/diff/60001/Source/core/animation/F...
File Source/core/animation/FilterStyleInterpolationTest.cpp (right):

https://codereview.chromium.org/1069313002/diff/60001/Source/core/animation/F...
Source/core/animation/FilterStyleInterpolationTest.cpp:19:
ASSERT(FilterStyleInterpolation::canCreateFrom(*value, *value));
Should use ASSERT_TRUE.

https://codereview.chromium.org/1069313002/diff/60001/Source/core/animation/F...
Source/core/animation/FilterStyleInterpolationTest.cpp:24: static void
testPrimitiveValue(RefPtrWillBeRawPtr<CSSValue> value, CSSValueID functionType,
double expected, CSSPrimitiveValue::UnitType unitType)
No need for this helper, just use CSSValue::equals().

[Timothy Loh, 2015-05-01 01:41:32]

I had a comment from a while ago which I never sent out...

https://codereview.chromium.org/1069313002/diff/20001/Source/core/animation/F...
File Source/core/animation/FilterStyleInterpolation.cpp (right):

https://codereview.chromium.org/1069313002/diff/20001/Source/core/animation/F...
Source/core/animation/FilterStyleInterpolation.cpp:68: size_t startLength =
start.isValueList() ? toCSSValueList(start).length() : 0;
Maybe clearer to write this as

// you'll need to add this function
if (start.isCSSWideKeyword() || end.isCSSWideKeyword())
 return nullptr;
if (!start.isValueList() && !end.isValueList())
 return nullptr; // Maybe this is weird, how about none -> none?

RefPtrWillBeRawPtr<CSSValueList> startList = start.isValueList() ?
toCSSValueList(start) : CSSValueList::createSpaceSeparated();
.. endList = ..

if (startList->length() < endList->length())
 startList = extendFilterList(startList, endList)
else if (endList ..)
 endList = ..

return ..

[Eric Willigers, 2015-05-01 01:51:42]

https://codereview.chromium.org/1069313002/diff/60001/Source/core/animation/F...
File Source/core/animation/FilterStyleInterpolation.cpp (right):

https://codereview.chromium.org/1069313002/diff/60001/Source/core/animation/F...
Source/core/animation/FilterStyleInterpolation.cpp:132: default:
On 2015/05/01 01:26:09, alancutter wrote:
> default should be an ASSERT_NOT_REACHED case and cases that are handled should
> be listed explicitly. Similar to fromInterpolableValue().

Done.

https://codereview.chromium.org/1069313002/diff/60001/Source/core/animation/F...
File Source/core/animation/FilterStyleInterpolationTest.cpp (right):

https://codereview.chromium.org/1069313002/diff/60001/Source/core/animation/F...
Source/core/animation/FilterStyleInterpolationTest.cpp:24: static void
testPrimitiveValue(RefPtrWillBeRawPtr<CSSValue> value, CSSValueID functionType,
double expected, CSSPrimitiveValue::UnitType unitType)
On 2015/05/01 01:26:09, alancutter wrote:
> No need for this helper, just use CSSValue::equals().

Done.

[Eric Willigers, 2015-05-01 03:30:58]

https://codereview.chromium.org/1069313002/diff/20001/Source/core/animation/F...
File Source/core/animation/FilterStyleInterpolation.cpp (right):

https://codereview.chromium.org/1069313002/diff/20001/Source/core/animation/F...
Source/core/animation/FilterStyleInterpolation.cpp:68: size_t startLength =
start.isValueList() ? toCSSValueList(start).length() : 0;
> RefPtrWillBeRawPtr<CSSValueList> startList = start.isValueList() ?
> toCSSValueList(start) : CSSValueList::createSpaceSeparated();

This won't compile as toCSSValueList returns a pointer, not a
RefPtrWillBeRawPtr.

We can use a slight variant of this approach by using references as the
arguments of the ternary operator, and passing the result of the ternary
operator to a function. See uploaded code.

[Timothy Loh, 2015-05-01 03:58:58]

rs-lgtm

https://codereview.chromium.org/1069313002/diff/140001/Source/core/animation/...
File Source/core/animation/FilterStyleInterpolation.cpp (right):

https://codereview.chromium.org/1069313002/diff/140001/Source/core/animation/...
Source/core/animation/FilterStyleInterpolation.cpp:77: if (!start.isValueList())
{
Reads weirdly, maybe ASSERT(start.isValueList() ||
toCSSPrimitiveValue(start).getValueID() == CSSValueNone)?

[Eric Willigers, 2015-05-01 04:14:05]

https://codereview.chromium.org/1069313002/diff/140001/Source/core/animation/...
File Source/core/animation/FilterStyleInterpolation.cpp (right):

https://codereview.chromium.org/1069313002/diff/140001/Source/core/animation/...
Source/core/animation/FilterStyleInterpolation.cpp:77: if (!start.isValueList())
{
On 2015/05/01 03:58:58, Timothy Loh wrote:
> Reads weirdly, maybe ASSERT(start.isValueList() ||
> toCSSPrimitiveValue(start).getValueID() == CSSValueNone)?

Done.

[Eric Willigers, 2015-05-01 04:14:10]

The CQ bit was checked by ericwilligers@chromium.org

[Eric Willigers, 2015-05-01 04:14:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
alancutter@chromium.org, timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1069313002/#ps160001
(title: "ASSERT isValueList || CSSValueNone")

[commit-bot: I haz the power, 2015-05-01 04:14:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1069313002/160001

[commit-bot: I haz the power, 2015-05-01 06:34:15]

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194788