Bug 94980

Created attachment 161300 [details] Patch

N.B. the above patch depends on https://bugs.webkit.org/show_bug.cgi?id=71401. I'm posting this here for review so I can make changes before my last day Friday, if necessary.

Created attachment 161310 [details] Patch

Rebased.

Created attachment 161315 [details] Patch

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.

The makes a lot more sense. Will do.

Comment on attachment 161315 [details] Patch Attachment 161315 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13693444

Comment on attachment 161315 [details] Patch Attachment 161315 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13693475

Comment on attachment 161315 [details] Patch Attachment 161315 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13682478

Created attachment 161577 [details] Patch

Comment on attachment 161577 [details] Patch Attachment 161577 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13687882

Comment on attachment 161577 [details] Patch Attachment 161577 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13688953

Created attachment 161590 [details] Patch

Created attachment 161594 [details] Patch

Comment on attachment 161594 [details] Patch Attachment 161594 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13693950

Comment on attachment 161594 [details] Patch Attachment 161594 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13688981

View in context: https://bugs.webkit.org/attachment.cgi?id=161594&action=review Thanks! Looks better. Still some comments on the tests. > Source/WebCore/platform/graphics/filters/CustomFilterTransformParameter.h:64 > + if (!from.size() || !to.size()) { I think there's no way we could have empty transform operations here. The parser takes care of that, so maybe an assert would be good enough. > LayoutTests/css3/filters/custom/custom-filter-animation.html:151 > + from { -webkit-filter: custom(url(../resources/vertex-transform-parameter.vs), transform rotateX(0deg) scale3d(1, 1, 1)); } I wish we had a test with two different transform attributes like: -webkit-filter: custom(url(../resources/vertex-transform-parameter.vs), transform1 rotateX(0deg), transform2 rotateY(90deg)); > LayoutTests/css3/filters/resources/custom-filter-parser.js:157 > + if (m.ahead().isA("(") && token.value != "matrix" && token.value != "matrix3d") Why isn't parseFunction good enough to parse the matrix? > LayoutTests/css3/filters/resources/custom-filter-parser.js:178 > + } else if (token.isA(transform)) { Why do you need this? You should not be needed to parse the parameter names.

Comment on attachment 161594 [details] Patch Attachment 161594 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13682963

Comment on attachment 161594 [details] Patch Attachment 161594 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13693969

Comment on attachment 161594 [details] Patch Attachment 161594 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13681933

Comment on attachment 161594 [details] Patch Attachment 161594 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13720323

Created attachment 163474 [details] Patch V1

Attachment 163474 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/anim..." exit_code: 1 Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:29: Use 'using namespace std;' instead of 'using std::max;'. [build/using_std] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 163478 [details] Patch V2 Fixed style.

Comment on attachment 163478 [details] Patch V2 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.

Thanks Dean! I will take a look tomorrow.

Landed in http://trac.webkit.org/changeset/128380 .