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

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.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 74995: patch 2
https://bugs.webkit.org/attachment.cgi?id=74995&action=review

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

I love the general idea of this patch! Some comments below, Dirk might have
additional ones:

> WebCore/ChangeLog:8
> +	   SVN filter primitives can use the output of other filters. The

s/SVN/SVG/

> WebCore/ChangeLog:11
> +	   primitive result was converted to image buffer before, which

s/result was/results were/, s/buffer/buffers/

> WebCore/ChangeLog:15
> +	   this patch. All apply() methods updated according to this new

s/updated/are updated/

> WebCore/platform/graphics/filters/FEBlend.cpp:-135
> -    resultImage()->putPremultipliedImageData(imageData.get(), imageRect,
IntPoint());

Ah, this is correct, as you're using the resultImage instead of creating a new
one.

> WebCore/platform/graphics/filters/FEComponentTransfer.cpp:172
> +    in->asUnmultipliedImage(imageData, drawingRect);

Hm, why is the return value ignored?
(Ignore this, I was confused as there are two variants of asUnmultipliedImage,
this one returns void.... see below)

> WebCore/platform/graphics/filters/FilterEffect.cpp:84
> +    m_imageBufferResult = ImageBuffer::create(m_absolutePaintRect.size(),
ColorSpaceLinearRGB);
> +    IntRect dst(IntPoint(), m_absolutePaintRect.size());

line 84: s/dst/destinationRect/

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

A call to .release() is missing here!

> WebCore/platform/graphics/filters/FilterEffect.cpp:103
> +    return imageData;

Ditto.

> WebCore/platform/graphics/filters/FilterEffect.cpp:106
> +inline void FilterEffect::copyImageBytes(ImageData* src, ImageData* dst,
const IntRect& rect)

Avoid abbrevations, while it is convienent here, I'd prefer source and
destination.

> WebCore/platform/graphics/filters/FilterEffect.cpp:113
> +    int originx = rect.x();

I'd prefer originX or xOrigin here.

> WebCore/platform/graphics/filters/FilterEffect.cpp:114
> +    int destx = 0;

Same here, destX or xDest (I'd prefer the latter ;-)

> WebCore/platform/graphics/filters/FilterEffect.cpp:119
> +    int endx = rect.x() + rect.width();

rect.right() ?

> WebCore/platform/graphics/filters/FilterEffect.cpp:129
> +    int endy = rect.y() + rect.height();

rect.bottom() ?

> WebCore/platform/graphics/filters/FilterEffect.cpp:137
> +    int dstScanline = rect.width() * 4;
> +    int srcScanline = m_absolutePaintRect.width() * 4;
> +    unsigned char *dstPixel = dst->data()->data()->data() + (((desty *
rect.width()) + destx) * 4);
> +    unsigned char *srcPixel = src->data()->data()->data() + (((originy *
m_absolutePaintRect.width()) + originx) * 4);

unsigned char* destinationPixel,  and one useless pair of braces at the end.
Same for sourcePixel.
dstScanline should be "destinationScanline", same for srcScanline.

> WebCore/platform/graphics/filters/FilterEffect.cpp:143
> +	   originy++;

I'd prefer ++originy.

> WebCore/platform/graphics/filters/FilterEffect.cpp:151
> +    if (!m_unmultipliedImageResult) {

Can you rewrite this with a early return for m_unmultipliedImageResult=true.

> WebCore/platform/graphics/filters/FilterEffect.cpp:159
> +	       unsigned char* src =
m_premultipliedImageResult->data()->data()->data();
> +	       unsigned char* dst =
m_unmultipliedImageResult->data()->data()->data();
> +	       unsigned char* end = src + (m_absolutePaintRect.width() *
m_absolutePaintRect.height() * 4);

Again, src -> source, dst -> destination etc.

> WebCore/platform/graphics/filters/FilterEffect.cpp:184
> +    if (!m_premultipliedImageResult) {

Same comments as abobe, early return style please, and variable renamings.

> WebCore/platform/graphics/filters/FilterEffect.h:58
> +    PassRefPtr<ImageData> asUnmultipliedImage(const IntRect&);
> +    PassRefPtr<ImageData> asPremultipliedImage(const IntRect&);
> +    void asUnmultipliedImage(ImageData* dst, const IntRect&);
> +    void asPremultipliedImage(ImageData* dst, const IntRect&);

The naming of these methods is misleading, I stared at 172    
in->asUnmultipliedImage(imageData, drawingRect); before, wondering why you
ignore the returned PassRefPtr<ImageData> until I realized there are two
variants of the same method, one which returns void and the other with the
PassRefPtr.

Can you rename the last two methods, to something like "convertToXXXImage" or
maybe even a better name..
Also the "dst" parameter name is not needed.

> WebCore/platform/graphics/filters/FilterEffect.h:132
> +    inline void copyImageBytes(ImageData* src, ImageData* dst, const
IntRect&);

s/src/source/ s/dst/destination/