[webkit-reviews] review denied: [Bug 119938] Animate CSS Image filter() function : [Attachment 209079] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 119938: Animate CSS Image filter() function
https://bugs.webkit.org/show_bug.cgi?id=119938

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=209079&action=review


Looks OK but I'd like to see a patch with the enum and other comments
addressed.

> Source/WebCore/css/CSSComputedStyleDeclaration.h:65
> +    static PassRefPtr<CSSValue> valueForFilter(const RenderObject*, const
RenderStyle*, const FilterOperations&, bool adjust = true);

It's not clear what "adjust" means in this context. You should use an enum.

> Source/WebCore/css/CSSComputedStyleDeclaration.h:80
> -    PassRefPtr<CSSValue> valueForShadow(const ShadowData*, CSSPropertyID,
const RenderStyle*) const;
> +    static PassRefPtr<CSSValue> valueForShadow(const ShadowData*,
CSSPropertyID, const RenderStyle*, bool adjust = true);

It's not clear what "adjust" means in this context. You should use an enum.

> Source/WebCore/css/CSSFilterImageValue.h:73
> +    bool equalSubimages(const CSSFilterImageValue&) const;

subimage is confusing; we should rename this ("input image"?)

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:225
> +    blendFilterOperations(
> +	   anim,
> +	   filterOperationsResult,
> +	   fromValue->filterOperations(),
> +	   toValue->filterOperations(),
> +	   progress);

This doesn't need to be so aggressively wrapped.

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:236
> +    RefPtr<CSSValue> filterValue = ComputedStyleExtractor::valueForFilter(
> +	   anim->renderer(),
> +	   anim->renderer()->style(),
> +	   filterOperationsResult,
> +	   false);

Ditto. 
Congratulations, you just fell into the boolean trap.

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:323
> +	   ASSERT(fromGenerated);
> +	   ASSERT(toGenerated);

Not sure these assertions are useful, since (if filters are enabled) you'll
crash on the next line anyway.

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:327
> +	       // Animation of generated images just possible if subimages are
equal.

s/just/is only
I don't really know what a subimage is in this context. It seems different from
our use of the term in other places.