[webkit-reviews] review denied: [Bug 119938] Animate CSS Image filter() function : [Attachment 209116] 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 209116: Patch
https://bugs.webkit.org/attachment.cgi?id=209116&action=review

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


> LayoutTests/fast/filter-image/filter-image-animation.html:58
> +    @-webkit-keyframes no-anim {
> +	   from { background-image: -webkit-filter(url(resources/image.svg),
sepia(1)); }
> +	   to	{ background-image: -webkit-filter(url(resources/blue.svg),
sepia(0)); }
> +    }

You should test a blend between a filter and simple image, and a filter and
gradient, and filter and crossfade (to check for code that behaves badly).

You should also test for blends between filter lists of the same and different
lengths, and maybe with missing filter lists.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:903
> +static inline PassRefPtr<CSSPrimitiveValue> valueForPixel(double length,
const RenderStyle* style, ZoomAdjustedPixel adjust)

This has a very confusing name; it sounds like you're lookup up the RGBA for a
single pixel.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:908
> +static inline PassRefPtr<CSSPrimitiveValue> valueForPixel(const Length&
length, const RenderStyle* style, ZoomAdjustedPixel adjust)

Ditto.

> Source/WebCore/css/CSSComputedStyleDeclaration.h:51
> +enum ZoomAdjustedPixel { AdjustPixel, DoNotAdjustPixel };

Enum types should be descriptive, like AdjustPixelVaulesForZoom {
AdjustPixelValuesForZoom, DoNotAdjustPixelValues }

> Source/WebCore/css/CSSFilterImageValue.cpp:168
> +bool CSSFilterImageValue::equalInputImages(const CSSFilterImageValue& other)
const

Much better than subimage!

> Source/WebCore/css/CSSFilterImageValue.h:49
> +

Blank line.