[webkit-reviews] review denied: [Bug 19243] [CAIRO] Masker in SVG : [Attachment 21333] Masking Cairo/SVG

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied 's request for review:
Bug 19243: [CAIRO] Masker in SVG
http://bugs.webkit.org/show_bug.cgi?id=19243

Attachment 21333: Masking Cairo/SVG
http://bugs.webkit.org/attachment.cgi?id=21333&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
This could be cleaner.

For:
+    // TODO: check if there is a difference between little and big endian
machines (for this code :-))

Just check the docs for CAIRO_FORMAT_ARGB32.  They should tell you what format
you are to expect the pixels in.

+	     int alpha = (*pixel & 0xff000000) >> 24;

Should really be encapsulated into static inline funtions (or use the existing
ones in Cairo).

unsigned char alpha = alphaValue(pixel);

etc.

+	     int luma = (int) (red * 0.3 + green * 0.59 + blue * 0.11);

Please add a comment to explain why you're choosing those multipliers.

Also, if you're going to be using alpha, red, green, etc. as floats, you might
as well store them in floats to begin with (instead of the ints you're
currently using, or the unsigned chars in my above example)

Again, something like this would be clearer:
*pixel = pixelFromValues(alpha, luma, luma, luma);

you could even have used something like this above:
float alpha, red, green, blue;
valuesFromPixel(alpha, red, green, blue);

I think some simple inlines like that would make this code much more readable
(and less error prone).

m_mask.set(m_ownerElement->drawMaskerContent(boundingBox,
m_maskRect).release())
drawMaskerContent returns a PassRefPtr, no need to call release() on it, as it
will automatically release into the RefPtr (that's the point of a PRP).

This comment doesn't help:
+    // Transform the mask-surface for use of cairo_mask()


Otherwise looks OK.  I'd rather see the TODOs just fixed now, instead of
landing this partial version.

r- due the fact that this code should be cleaner/more readable.