[webkit-reviews] review denied: [Bug 77245] [Chromium] SVG Composite of Offset crashes : [Attachment 129963] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Stephen Chenney
<schenney at chromium.org>'s request for review:
Bug 77245: [Chromium] SVG Composite of Offset crashes
https://bugs.webkit.org/show_bug.cgi?id=77245

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=129963&action=review


Patch looks great! I still have some suggestions. First: The testcase is
perfectly suited for a ref test, and I encourage you to rewrite it.

> Source/WebCore/platform/graphics/filters/FEComposite.h:62
> +    virtual bool requiresValidPreMulRGBA() OVERRIDE { return m_type !=
FECOMPOSITE_OPERATOR_ARITHMETIC; }

We shouldn't use abbreviations in WebKit per coding style guidelines, how about
requiresValidPremultipliedRGBA? or PreMultiplied if you prefer that.
(I'd grep for similar names in WebCore, and see what's used)

> Source/WebCore/platform/graphics/filters/FEComposite.h:63
> +    virtual bool isInvalidPreMulRGBA() OVERRIDE { return m_type ==
FECOMPOSITE_OPERATOR_ARITHMETIC; }

This method is not really needed, you always use it like this:
for (unsigned i = 0; i < size; ++i) {
    FilterEffect* in = m_inputEffects.at(i).get();
    if (in->isInvalidPreMulRGBA())
	in->forceValidPreMulRGBA();
}
How about switching to:

for (unsigned i = 0; i < size; ++i)
    m_inputEffects[i]->forceValidPreMultipliedRGBAOutputIfNeeded();

Or even "sanitzeEffectIfNeeded" to hide the detail, what that involves.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:96
>	   return;

You could leave that out.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:127
> +

This function should be commented!

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:136
> +    int numPixels = pixelArrayLength / 4;
> +    while (--numPixels >= 0) {
> +	   unsigned char a = *(pixelData + 3);
> +	   for (int i = 0; i < 3; ++i) {

While I verified this is correct, comments would be helpful :-)

> LayoutTests/platform/chromium/test_expectations.txt:943
> +BUGWK77245 : svg/filters/feComposite-arithmetic-invalid-rgba.svg = PASS FAIL


This could be fully avoided with a reftest.

> LayoutTests/svg/filters/feComposite-arithmetic-invalid-rgba.svg:1
> +<svg xmlns="http://www.w3.org/2000/svg"
xmlns:xlink="http://www.w3.org/1999/xlink" width="200" height="100"
style="background:white;">

Heh, background: white works? Never tried that :-) Is it needed?