[webkit-reviews] review denied: [Bug 6033] WebKit+SVG does not support color-interpolation-filters or draw filters in correct colorspace : [Attachment 130444] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Florin Malita <fmalita at google.com>'s
request for review:
Bug 6033: WebKit+SVG does not support color-interpolation-filters or draw
filters in correct colorspace
https://bugs.webkit.org/show_bug.cgi?id=6033

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=130444&action=review


Hi. Great that you work on that. r- because of the lack of tests and
description.

> Source/WebCore/ChangeLog:4
> +
> +	   WebKit+SVG does not support color-interpolation-filters or draw
filters in correct colorspace
> +	   https://bugs.webkit.org/show_bug.cgi?id=6033

You need a detailed description what you do here, how you do it, and how it
affects the rendering.

> Source/WebCore/ChangeLog:8
> +	   No new tests. (OOPS!)

We need tests for new behaviors. In the case for color-interpolation-filters we
would need a lot of pixel results: What happens if you apply sRGB to the
<filter> element? What if you add it to a filter effect? What if you do both in
different orders? How does it work together with our logic for premultiplied
and unpremultiplied effect results? Remember that effects won't use the
ImageBuffer results of previous effects, if previous effects already have
provide the bit-array.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:-288
> -#if !USE(CG)
> -	   if (filterData->sourceGraphicBuffer)
> -	      
filterData->sourceGraphicBuffer->transformColorSpace(ColorSpaceDeviceRGB,
ColorSpaceLinearRGB);
> -#endif

Have you checked that it does not cause regressions on repainting? IIRC it was
added here to prevent calling the transformation multiple times on every
repainting (for instance on scrolling, when the filter is not in the backing
store any more).