Bug 145386 – Backdrop filters don't animate

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

Backdrop filters don't animate

[Radar WebKit Bug Importer]

<rdar://problem/21110037>

[Dean Jackson]

Created attachment 253736 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 253736 [details]
Patch

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

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:204
> +    if (anim->filterFunctionListsMatch() || anim->backdropFilterFunctionListsMatch())
>          result = blendFilterOperations(anim, from, to, progress);

This doesn't seem right. Can't a KeyframeAnimaiton have a matching filter list, and non-matching backdrop filter list or vice versa?

> Source/WebCore/page/animation/ImplicitAnimation.cpp:328
> +    if (val->operations().isEmpty())
> +        val = toVal;
> +
> +    if (val->operations().isEmpty())
> +        return;
> +
> +    // An empty filter list matches anything.
> +    if (val != toVal && !toVal->operations().isEmpty() && !val->operationsMatch(*toVal))
> +        return;

We should share this part of the code with filters.

[Darin Adler]

Comment on attachment 253736 [details]
Patch

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

> Source/WebCore/page/animation/ImplicitAnimation.cpp:318
> +    const FilterOperations* val = &m_fromStyle->backdropFilter();
> +    const FilterOperations* toVal = &m_toStyle->backdropFilter();

I think we could spring for the two extra letters "ue" and call these value instead of val.

I suggest that toValue be a reference instead of a pointer.

> Source/WebCore/page/animation/KeyframeAnimation.cpp:461
> +    const FilterOperations* firstVal = &m_keyframes[firstNonEmptyFilterKeyframeIndex].style()->backdropFilter();

I suggest a reference instead of a pointer. Also suggest using the word "value" instead of the non-word "val". Also seems that auto& would work well here.

> Source/WebCore/page/animation/KeyframeAnimation.cpp:464
> +        const KeyframeValue& currentKeyframe = m_keyframes[i];

Doesn’t seem like we need this local variable. Should just use m_keyframes[i] directly.

> Source/WebCore/page/animation/KeyframeAnimation.cpp:465
> +        const FilterOperations* val = &currentKeyframe.style()->backdropFilter();

I suggest a reference instead of a pointer. Also suggest using the word "value" instead of the non-word "val". Also seems that auto& would work well here.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:245
> +        return "backdropFilters";

Should use ASCIILiteral here.

In fact, I think the return type of this function should be ASCIILiteral instead of String.

[Dean Jackson]

(In reply to comment #4)
> Comment on attachment 253736 [details]
> Patch

Thanks Darin and Simon.

[Dean Jackson]

Created attachment 253795 [details]
Patch

[Dean Jackson]

Committed r184908: <http://trac.webkit.org/changeset/184908>