[webkit-reviews] review granted: [Bug 94980] [CSS Shaders] Implement transform parameter animations for CSS Custom Filters : [Attachment 163478] Patch V2

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Chiculita Alexandru
<achicu at adobe.com>'s request for review:
Bug 94980: [CSS Shaders] Implement transform parameter animations for CSS
Custom Filters
https://bugs.webkit.org/show_bug.cgi?id=94980

Attachment 163478: Patch V2
https://bugs.webkit.org/attachment.cgi?id=163478&action=review

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=163478&action=review


> LayoutTests/css3/filters/custom/custom-filter-transforms-animation.html:45
> +	   @-webkit-keyframes custom-mix-multi-transform-anim {
> +	       from { -webkit-filter: custom(url(no-shader-needed.vs),
transform1 scale(0), transform2 scale(100) rotate(0deg)); }
> +	       to   { -webkit-filter: custom(url(no-shader-needed.vs),
transform1 scale(30), transform2 rotate(0deg) scale(100)); }
> +	   }

Could you add two by two more tests (a) where from and to have different
numbers of operations (and reverse) and (b) where one of from and to are
empty/not-present?

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:151
> -	       RefPtr<FilterOperation> blendedOp = toOp ?
toOp->blend(fromOp.get(), progress) : (fromOp ? fromOp->blend(0, progress,
true) : PassRefPtr<FilterOperation>(0));
> +	       RefPtr<FilterOperation> blendedOp = toOp ? blendFunc(anim,
fromOp.get(), toOp.get(), progress) : (fromOp ? blendFunc(anim, 0,
fromOp.get(), progress, true) : PassRefPtr<FilterOperation>(0));

It seems we can replace the PassRefPtr<FilterOperation>(0) with just 0 here now
(i'm not sure what the history here is, although it was also in the code
removed above - PassRefPtr<TransformOperation>(0)).

> Source/WebCore/platform/graphics/filters/CustomFilterTransformParameter.h:61
> +	   RefPtr<CustomFilterTransformParameter> ret =
CustomFilterTransformParameter::create(name());

Nit: need a better name than "ret"

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:78
> +	   RefPtr<TransformOperation> fromOp = (i < fromSize) ?
from.operations()[i].get() : 0;
> +	   RefPtr<TransformOperation> toOp = (i < toSize) ?
operations()[i].get() : 0;
> +	   RefPtr<TransformOperation> blendedOp = toOp ?
toOp->blend(fromOp.get(), progress) : (fromOp ? fromOp->blend(0, progress,
true) : PassRefPtr<TransformOperation>(0));

Nit: I prefer long variable names fromOp -> fromOperation (there are a few
places in other files in the patch that have this too)

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:83
> +	       if (progress > 0.5)

At some point I'd like to work out if this is the right thing to do (it's
copied from the CSS animation blends of filters and transforms), and maybe add
a comment explaining it :) I can't remember where it comes from.

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:93
> +TransformOperations
TransformOperations::blendUsingMatricesInterpolation(const TransformOperations&
from, double progress, const LayoutSize& size) const

This should be blendByUsingMatrixInterpolation

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:99
> +    TransformationMatrix fromT;
> +    TransformationMatrix toT;

Nit: I prefer long variable names like fromT -> fromTransform

> Source/WebCore/platform/graphics/transforms/TransformOperations.h:77
> +    TransformOperations blendByMatchingOperations(const
TransformOperations&, const double& progress) const;
> +    TransformOperations blendUsingMatricesInterpolation(const
TransformOperations& from, double progress, const LayoutSize&) const;
> +    TransformOperations blend(const TransformOperations& from, double
progress, const LayoutSize&) const;

Nit: you're defining "from" on 2 out of 3 of these.