Bug 68476

Summary: Make -webkit-filter animatable
Product: WebKit Reporter: Dean Jackson <dino>
Component: CSSAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, dglazkov, gns, simon.fraser, thorton, webkit-bug-importer, webkit.review.bot, xan.lopez
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 68475    
Bug Blocks:    
Attachments:
Description Flags
Work in progresss. Only blur() animates.
none
Testcase
none
Fixed testcase
none
Patch
none
Patch
none
Patch cmarrin: review+

Description Dean Jackson 2011-09-20 14:25:47 PDT
We'll need some logic to animate filters, similar to the way transforms animate.
Comment 1 Radar WebKit Bug Importer 2011-09-20 14:26:30 PDT
<rdar://problem/10155818>
Comment 2 Simon Fraser (smfr) 2011-12-11 23:04:46 PST
Created attachment 118736 [details]
Work in progresss. Only blur() animates.
Comment 3 Dean Jackson 2011-12-12 10:09:12 PST
Cool
Comment 4 Simon Fraser (smfr) 2011-12-12 18:06:39 PST
Created attachment 118928 [details]
Testcase
Comment 5 Simon Fraser (smfr) 2011-12-13 22:57:09 PST
Created attachment 119158 [details]
Fixed testcase
Comment 6 Simon Fraser (smfr) 2011-12-13 23:13:14 PST
Created attachment 119160 [details]
Patch
Comment 7 WebKit Review Bot 2011-12-13 23:17:58 PST
Comment on attachment 119160 [details]
Patch

Attachment 119160 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10871121
Comment 8 Early Warning System Bot 2011-12-13 23:19:21 PST
Comment on attachment 119160 [details]
Patch

Attachment 119160 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10876139
Comment 9 Simon Fraser (smfr) 2011-12-13 23:27:47 PST
Created attachment 119166 [details]
Patch
Comment 10 WebKit Review Bot 2011-12-13 23:52:52 PST
Comment on attachment 119166 [details]
Patch

Attachment 119166 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10872146
Comment 11 Gyuyoung Kim 2011-12-14 00:51:30 PST
Comment on attachment 119166 [details]
Patch

Attachment 119166 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10874155
Comment 12 Gustavo Noronha (kov) 2011-12-14 02:10:20 PST
Comment on attachment 119166 [details]
Patch

Attachment 119166 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10872192
Comment 13 Early Warning System Bot 2011-12-14 03:05:09 PST
Comment on attachment 119166 [details]
Patch

Attachment 119166 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10871180
Comment 14 Csaba Osztrogon√°c 2011-12-14 03:26:24 PST
Comment on attachment 119166 [details]
Patch

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

> Source/WebCore/Target.pri:3265
>          rendering/style/FilterOperations.cpp
> +        rendering/style/FilterOperation.cpp

rendering/style/FilterOperations.cpp \
rendering/style/FilterOperation.cpp
Comment 15 Simon Fraser (smfr) 2011-12-14 08:38:55 PST
Created attachment 119224 [details]
Patch
Comment 16 Simon Fraser (smfr) 2011-12-14 09:40:56 PST
Patch needs:

diff --git a/Source/WebCore/page/animation/KeyframeAnimation.cpp b/Source/WebCore/page/animation/KeyframeAnimation.cpp
index 9393e4a..f0231e2 100644
--- a/Source/WebCore/page/animation/KeyframeAnimation.cpp
+++ b/Source/WebCore/page/animation/KeyframeAnimation.cpp
@@ -452,7 +452,7 @@ void KeyframeAnimation::checkForMatchingFilterFunctionLists()
 
     for (size_t i = 0; i < numKeyframes; ++i) {
         const KeyframeValue& currentKeyframe = m_keyframes[i];
-        if (currentKeyframe.style()->transform().operations().size()) {
+        if (currentKeyframe.style()->filter().operations().size()) {
             firstNonEmptyFilterKeyframeIndex = i;
             break;
         }
Comment 17 Chris Marrin 2011-12-14 09:58:58 PST
Comment on attachment 119224 [details]
Patch

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

Looks good. Just a few nits

> Source/WebCore/page/animation/AnimationBase.cpp:201
> +                RefPtr<FilterOperation> identityOp = PassthroughFilterOperation::create();

You should avoid creating this object when not needed

> Source/WebCore/page/animation/ImplicitAnimation.cpp:273
> +    // Transform lists match.

Add a FIXME here so we can change this variable and get rid of this useless comment when isTransformFunctionListValid gets renamed

> Source/WebCore/rendering/style/FilterOperation.cpp:35
> +PassRefPtr<FilterOperation> BasicColorMatrixFilterOperation::blend(const FilterOperation* from, double progress, bool blendToPassthrough)

Why "Basic"? I don't see any other forms of this object that would cause you to have to discriminate it like this.

> Source/WebCore/rendering/style/FilterOperation.cpp:63
> +PassRefPtr<FilterOperation> BasicComponentTransferFilterOperation::blend(const FilterOperation* from, double progress, bool blendToPassthrough)

Same here

> Source/WebCore/rendering/style/FilterOperation.h:-113
> -

Spurious change?
Comment 18 Simon Fraser (smfr) 2011-12-14 13:16:53 PST
Landed with a test: 
http://trac.webkit.org/changeset/102815