[Webkit-unassigned] [Bug 94980] [CSS Shaders] Implement transform parameter animations for CSS Custom Filters

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=94980





--- Comment #6 from Chiculita Alexandru <achicu at adobe.com>  2012-08-29 14:32:59 PST ---
View in context: https://bugs.webkit.org/attachment.cgi?id=161300&action=review

Thanks for looking into this. I have a few comments on how we do the blending. I'm not a reviewer, so feel free to ignore my comments.

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:-54
> -static inline int blendFunc(const AnimationBase*, int from, int to, double progress)

Why do you need all these changes?

> Source/WebCore/platform/graphics/filters/CustomFilterOperation.h:34
> +#include "CSSPropertyAnimation.h"

Don't add this to the header file. You just need to forward define the AnimationBase.

> Source/WebCore/platform/graphics/filters/CustomFilterTransformParameter.h:34
> +#include "CSSPropertyAnimation.h"

Ditto.

> Source/WebCore/platform/graphics/filters/CustomFilterTransformParameter.h:50
> +    virtual PassRefPtr<CustomFilterParameter> blend(const AnimationBase* anim, const CustomFilterParameter* fromParameter, double progress)

This function will pull in the AnimationBase class, so I think we would better move it to CustomFilterTransformParameter.cpp.

> Source/WebCore/platform/graphics/filters/CustomFilterTransformParameter.h:58
> +        TransformOperations result = CSSPropertyAnimation::blendFunc(anim, from, to, progress);

This function uses isTransformFunctionListValid internally, which is only set for the -webkit-transform property. For CSS Custom Filters, I think we would need to check for matching transforms everytime, as there's no place where we could store that info right now.

I think it would be better if we moved the blending of the transforms to the TransformOperations class: 
transformOperations.blendByMatchingOperations(const TransformOperations& from, double progress) - this one can assert if the operations are not matching and it's the caller responsibility to make sure that the operations are matching.
transformOperations.blendByMatrix(const TransformOperations& from, double progress, const LayoutSize&)
and finally
transformOperations.blend(const TransformOperations& from, double progress, const LayoutSize&) - will do the decision inside

> LayoutTests/css3/filters/custom/custom-filter-animation.html:105
> +        @-webkit-keyframes custom-mix-transforms-anim {

You should have tests checking for number of rotations. Ie. rotateZ(0deg) to rotateZ(360deg) should make a full spin. In the current patch it wouldn't animate at all I suppose.

> LayoutTests/css3/filters/custom/custom-filter-animation.html:107
> +            to   { -webkit-filter: custom(url(../resources/vertex-transform-parameter.vs), transform rotateZ(10deg)); }

Add a test with multiple transforms in the same custom function.

> LayoutTests/css3/filters/resources/custom-filter-parser.js:67
> +function transform(token)

You should be looking for matrix and matrix3d instead, you need to add tests for multiple transform attributes.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.