[webkit-reviews] review granted: [Bug 44025] [Qt] Shadow blur for rectangle fill : [Attachment 64469] Patch

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has granted Ariya Hidayat
<ariya.hidayat at gmail.com>'s request for review:
Bug 44025: [Qt] Shadow blur for rectangle fill
https://bugs.webkit.org/show_bug.cgi?id=44025

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

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
Looks good to me, some minor nits below.

WebCore/platform/graphics/qt/ContextShadow.cpp:70
 +  #define BLUR_SUM_SHIFT 14
Why not use a static const?

WebCore/platform/graphics/qt/ContextShadow.cpp:72
 +  // Note: image must be RGB32 format
You could add an assert for that, I guess

WebCore/platform/graphics/qt/ContextShadow.cpp:88
 +  
why this extra newline?

WebCore/platform/graphics/qt/ContextShadow.cpp:91
 +	    int left, right, pixelCount, prev, next;
We normally do not add most variable declarations on the same line.

WebCore/platform/graphics/qt/ContextShadow.cpp:128
 +			    ((prev > 0) ? *prevPtr : firstAlpha);
why not just keep on one line :-) I guess it is shorter than the above for-one.


WebCore/platform/graphics/qt/ContextShadow.cpp:154
 +	    // Step 3: blur green channel and store the result in the alpha
channel
Add a dot at the end of the comment.

WebCore/platform/graphics/qt/ContextShadow.cpp:190
 +	// "colorize" with the right shadow color
here as well

WebCore/platform/graphics/qt/ContextShadow.cpp:220
 +	}
Here we shouldnt use braces according to the coding style guide