Bug 237590

Created attachment 454086 [details] Patch, v1

Comment on attachment 454086 [details] Patch, v1 View in context: https://bugs.webkit.org/attachment.cgi?id=454086&action=review > Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1536 > +TransformationMatrix& TransformationMatrix::multiplyAffineTransform(const AffineTransform& mat) We probably should not abbreviate here, so either 'matrix' or 'affineTransform'.

Comment on attachment 454086 [details] Patch, v1 View in context: https://bugs.webkit.org/attachment.cgi?id=454086&action=review > Source/WebCore/rendering/style/RenderStyle.cpp:1474 > + if (transformOperations.size()) { > + for (auto& operation : transformOperations.operations()) > + operation->apply(transform, boundingBox.size()); > + } else if (externalAffineTransformationSource) { > + // SVG may provide additional non-CSS transformation operations. > + transform.multiplyAffineTransform(externalAffineTransformationSource()); > + } It's weird that this is either-or. From the function signature I would expect that the result of externalAffineTransformationSource would get multiplied with the style's transforms. Especially since this function DOES use the individual transform properties.

Comment on attachment 454086 [details] Patch, v1 View in context: https://bugs.webkit.org/attachment.cgi?id=454086&action=review >> Source/WebCore/rendering/style/RenderStyle.cpp:1474 >> + } > > It's weird that this is either-or. From the function signature I would expect that the result of externalAffineTransformationSource would get multiplied with the style's transforms. Especially since this function DOES use the individual transform properties. One option would be to add another function that knows how to apply/unapply transform origin, and call it from this function, and to add a third function for the custom SVG case.

(In reply to Simon Fraser (smfr) from comment #3) > Comment on attachment 454086 [details] > Patch, v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=454086&action=review > > > Source/WebCore/rendering/style/RenderStyle.cpp:1474 > > + if (transformOperations.size()) { > > + for (auto& operation : transformOperations.operations()) > > + operation->apply(transform, boundingBox.size()); > > + } else if (externalAffineTransformationSource) { > > + // SVG may provide additional non-CSS transformation operations. > > + transform.multiplyAffineTransform(externalAffineTransformationSource()); > > + } > > It's weird that this is either-or. From the function signature I would > expect that the result of externalAffineTransformationSource would get > multiplied with the style's transforms. Especially since this function DOES > use the individual transform properties. Good catch, the intention was to only look at 'externalAffineTransformSource' if neither the transform property nor the individual transform properties are given, but the "if (transformOperatations.size())" case only catches the transform attribute, not the rest.

(In reply to Simon Fraser (smfr) from comment #4) > > It's weird that this is either-or. From the function signature I would expect that the result of externalAffineTransformationSource would get multiplied with the style's transforms. Especially since this function DOES use the individual transform properties. > > One option would be to add another function that knows how to apply/unapply > transform origin, and call it from this function, and to add a third > function for the custom SVG case. +1 that sounds much nicer.

Created attachment 454212 [details] Patch, v2

Created attachment 454217 [details] Patch, v3

(In reply to Rob Buis from comment #2) > Comment on attachment 454086 [details] > Patch, v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=454086&action=review > > > Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1536 > > +TransformationMatrix& TransformationMatrix::multiplyAffineTransform(const AffineTransform& mat) > > We probably should not abbreviate here, so either 'matrix' or > 'affineTransform'. Missed that comment, I can apply s/mat/matrix/ before landing.

Created attachment 454348 [details] Patch, v4

(In reply to Nikolas Zimmermann from comment #9) > (In reply to Rob Buis from comment #2) > > Comment on attachment 454086 [details] > > Patch, v1 > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=454086&action=review > > > > > Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1536 > > > +TransformationMatrix& TransformationMatrix::multiplyAffineTransform(const AffineTransform& mat) > > > > We probably should not abbreviate here, so either 'matrix' or > > 'affineTransform'. > > Missed that comment, I can apply s/mat/matrix/ before landing. Even easier, the changes are not needed here anymore, leaving this out for another LBSE patch.

Simon, can you have another look? This slightly deviates from your proposal -- it might be helpful to look at this patch and also at https://bugs.webkit.org/show_bug.cgi?id=237711, which is the patch that enables SVG/CSS transform support for LBSE (RenderLayerModelObject::applySVGTransform() makes use of the new methods introduced here: applyTransformOrigin/unapplyTransformOrigin/affectedByTransformOrigin/etc.) The patch on ticket 237711 depends on all of these tickets: 237553 (done), 237554 (r?), 237590 (this one), 237701 (depends on 2357554). Once these are in, I can proceed to the patches that enable transform-box support for composited elements -- which is trivial after the cleanups are all in.

<rdar://problem/90248514>

Comment on attachment 454348 [details] Patch, v4 View in context: https://bugs.webkit.org/attachment.cgi?id=454348&action=review > Source/WebCore/rendering/style/RenderStyle.cpp:1508 > + // 8. Translate by the negated computed X, Y and Z values of transform-origin. (see unapplyTransformOrigin) Weird to have this dangling comment here.

(In reply to Simon Fraser (smfr) from comment #14) > Comment on attachment 454348 [details] > Patch, v4 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=454348&action=review > > > Source/WebCore/rendering/style/RenderStyle.cpp:1508 > > + // 8. Translate by the negated computed X, Y and Z values of transform-origin. (see unapplyTransformOrigin) > > Weird to have this dangling comment here. I've added (see unapplyTransformOrigin) -- modified to: + // 8. Translate by the negated computed X, Y and Z values of transform-origin. + // (implemented in unapplyTransformOrigin) before landing. I hope that's better :-)

Committed r291338 (248477@trunk): <https://commits.webkit.org/248477@trunk>