[webkit-reviews] review denied: [Bug 49907] Better result passing in filter primitives : [Attachment 74848] patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Zoltan Herczeg
<zherczeg at webkit.org>'s request for review:
Bug 49907: Better result passing in filter primitives
https://bugs.webkit.org/show_bug.cgi?id=49907

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

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

The style bot is red, did you check the style manually on your machine?

> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:179
> +    in->asPremultipliedImage(resultImage, effectDrawingRect);

Can you explain how this works? the resultImage is possibly larger than the
sourceImage, in every direction. How do you copy the data to the resultImage?

> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:192
> +    RefPtr<ImageData> tmpImageData =
ImageData::create(absolutePaintRect().width(), absolutePaintRect().height());
>      ByteArray* tmpPixelArray = tmpImageData->data()->data();
>  
> -    int stride = 4 * imageRect.width();
> +    IntSize paintSize = absolutePaintRect().size();

Can you move the intsize some line above and replace the absolutePainRect()
calls?

> WebCore/platform/graphics/filters/FETile.cpp:84
> +    resultImage->context()->setFillPattern(pattern);
> +    resultImage->context()->fillRect(FloatRect(FloatPoint(),
absolutePaintRect().size()));

save context in a local var before using it here.

> WebCore/platform/graphics/filters/FilterEffect.cpp:78
> +    // This function is allowed to return with NULL.

I think this sentence is unnecessary.

> WebCore/platform/graphics/filters/FilterEffect.cpp:94
> +    PassRefPtr<ImageData> imageData = ImageData::create(rect.width(),
rect.height());

You have to take the ownership first: RefPtr<ImageData>

> WebCore/platform/graphics/filters/FilterEffect.cpp:101
> +    PassRefPtr<ImageData> imageData = ImageData::create(rect.width(),
rect.height());

Ditto.

> WebCore/platform/graphics/filters/FilterEffect.cpp:223
> +    if (!m_imageBufferResult->context()) {
> +	   // All filter effects, which allocates an imageBufferResult
> +	   // will use its context. If we would not destroy the image here
> +	   // the filter would return a black rectangle, which is not desired.
> +	   // (I would prefer an ASSERT here, since I think the context must be
exists)
> +	   m_imageBufferResult.clear();
> +    }

I agree. ;)