[webkit-reviews] review denied: [Bug 48031] Make AffineTransform and TransformationMatrix do matrix multiplication in the correct order (Column Major) : [Attachment 78495] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Shane Stephens
<shanestephens at google.com>'s request for review:
Bug 48031: Make AffineTransform and TransformationMatrix do matrix
multiplication in the correct order (Column Major)
https://bugs.webkit.org/show_bug.cgi?id=48031

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

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

Looks good, though still some things to resolve IMHO.

> Source/WebCore/platform/graphics/transforms/AffineTransform.h:149
> +	   return this->multiply(t);

s/this->// it's not needed.

> Source/WebCore/platform/graphics/transforms/AffineTransform.h:183
> +    AffineTransform& postMultiply(const AffineTransform&);

Where is postMultiply defined/used??

> Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp:18
> + *

Looks unrelated.

> Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:21
> + *

Ditto.

> Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp:19
> + *

Ditto.

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:46
> -	   absoluteTransform.multiply(current->localToParentTransform());
> +	   absoluteTransform = current->localToParentTransform() *
absoluteTransform;

This is a place, where your postMultiply method would be useful, no?
absoluteTransform.postMultiply(current->localToParentTransform()).

I think this would be easier to read/understand, as the current solution, and
it's for sure faster, as it avoids the assignment operator usage, after the
multiplication.

> Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:87
> -    contentTransformation.multiply(subtreeContentTransformation);
> +    contentTransformation = subtreeContentTransformation *
contentTransformation;
>  

Ditto.

> Source/WebCore/rendering/svg/SVGTextLayoutEngine.cpp:324
> -		   transform.multiply(textBoxTransformation);
> +		   transform = textBoxTransformation * transform;

Ditto. (Several more places which could make use of postMultiply, didn't
mention all of them)