[webkit-reviews] review denied: [Bug 53055] REGRESSION: Rendering artifacts on a rotated, pattern filled shape : [Attachment 105719] patch, fixing Darin's comments

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied  review:
Bug 53055: REGRESSION: Rendering artifacts on a rotated, pattern filled shape
https://bugs.webkit.org/show_bug.cgi?id=53055

Attachment 105719: patch, fixing Darin's comments
https://bugs.webkit.org/attachment.cgi?id=105719&action=review

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


r- because of the build failures and style issues.

>> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:128
>> +	// FIXME67242: Don't round-trip through TransformationMatrix.
> 
> This is not our usual format. Ramming the bug number up against FIXME is not
how we do it.
> 
> I’m not sure this really needs a FIXME. Sure, it would be more elegant, but
what’s the big deal? Probably overkill to include the comment. Unless you said
in the comment *why* it needs to be fixed.

I don't agree. First, I think this should be fixed before landing.
SVGImageBufferTools is not really fast, we shouldn't make it slower with
unnecessary copy operations. We never use TransformationMatrix in SVG, and even
if we have to switch to TransformationMatrix for supporting semi-3D, we can't
use this function as is. Second, if we add a fix me instead of fixing it at
once, it is very likely that it will never get fixed. I don't see a big deal to
add a new decompose function to AffineTransform. The necessary coder for affine
matrices can be found online.