[webkit-reviews] review granted: [Bug 68476] Make -webkit-filter animatable : [Attachment 119224] Patch

[bugzilla-daemon at webkit.org]

Chris Marrin <cmarrin at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 68476: Make -webkit-filter animatable
https://bugs.webkit.org/show_bug.cgi?id=68476

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

------- Additional Comments from Chris Marrin <cmarrin at apple.com>
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?