[webkit-reviews] review denied: [Bug 28362] SVG Filter feComposite implementation is missing : [Attachment 34934] feComposite implementation

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 28362: SVG Filter feComposite implementation is missing
https://bugs.webkit.org/show_bug.cgi?id=28362

Attachment 34934: feComposite implementation
https://bugs.webkit.org/attachment.cgi?id=34934&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
r- for various issues:


> +#if !PLATFORM(CG) && !PLATFORM(SKIA)
> +void inline operatorIn(const PassRefPtr<CanvasPixelArray>& srcPixelArrayA,
PassRefPtr<CanvasPixelArray>& srcPixelArrayB)
s/void inline/inline void/
Pass CanvasPixelArray pointers here.

>  {
> +    for (unsigned pixelOffset = 0; pixelOffset < srcPixelArrayA->length();
pixelOffset++) {
s/pixelOffset++/++pixelOffset/

> +	   unsigned pixelByteOffset = pixelOffset * 4;
> +	   unsigned char alphaA = srcPixelArrayA->get(pixelByteOffset + 3);
> +	   unsigned char alphaB = srcPixelArrayB->get(pixelByteOffset + 3);
> +
> +	   unsigned char resultB = (alphaA * alphaB) / 255;
> +	   srcPixelArrayB->set(pixelByteOffset + 3, resultB);
> +    }
> +}
> +#endif

It should be made more clear why CG/Skia support clipToImageBuffer, where the
rest doesn't.

> + void inline arithmetic(const PassRefPtr<CanvasPixelArray>& srcPixelArrayA,
PassRefPtr<CanvasPixelArray>& srcPixelArrayB,
s/void inline/inline void/
Pass CanvasPixelArray pointers here.

> +    for (unsigned pixelOffset = 0; pixelOffset < srcPixelArrayA->length();
pixelOffset++) {
s/pixelOffset++/++pixelOffset/

> +	   unsigned pixelByteOffset = pixelOffset * 4;
> +	   for (unsigned channel = 0; channel < 4; channel++) {
s/channel++/++channel/

> +#if PLATFORM(CG) || PLATFORM(SKIA)
> +		   filterContext->save();
> +		  
filterContext->clipToImageBuffer(calculateDrawingRect(m_in2->subRegion()),
m_in2->resultImage());
> +		   filterContext->drawImage(m_in->resultImage()->image(),
calculateDrawingRect(m_in->subRegion()));
> +		   filterContext->restore();
> +#else
> +		   IntRect effectADrawingRect =
calculateDrawingIntRect(m_in2->subRegion());
> +		   PassRefPtr<CanvasPixelArray>
srcPixelArrayA(m_in2->resultImage()->getPremultipliedImageData(effectADrawingRe
ct)->data());
> +
> +		   IntRect effectBDrawingRect =
calculateDrawingIntRect(m_in->subRegion());
> +		   PassRefPtr<ImageData>
imageData(m_in->resultImage()->getPremultipliedImageData(effectBDrawingRect));
> +		   PassRefPtr<CanvasPixelArray>
srcPixelArrayB(imageData->data());
> +
> +		   operatorIn(srcPixelArrayA, srcPixelArrayB);
> +		   resultImage()->putPremultipliedImageData(imageData.get(),
IntRect(IntPoint(), resultImage()->size()), IntPoint());
> +#endif

This code should be encapsulated in a helper function, to avoid spreading
CG/Skia defines.
Don't use "PassRefPtr<CanvasPixelArray> = ...". As the name indicates, it
should only be used when passing around RefPtrs, which is not the case here.
Also imageData->data() returns a plain pointer, no need for you to ref/deref
it.

There's also a style issue in the FECOMPOSITE_OPERATOR_ARITHMETIC case, you're
indenting too much there.