[webkit-reviews] review denied: [Bug 79068] SVG should support transform-origin and relative values : [Attachment 130882] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Hans Muller
<giles_joplin at yahoo.com>'s request for review:
Bug 79068: SVG should support transform-origin and relative values
https://bugs.webkit.org/show_bug.cgi?id=79068

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

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


Nice patch Hans - it's gettin much closer, still some new comments:

> Source/WebCore/ChangeLog:28
> +	   * rendering/style/RenderStyle.cpp:
> +	   (WebCore):
> +	   (WebCore::optimizeForTransformOrigin):
> +	   (WebCore::RenderStyle::applyTransform):

The interesting parts are not described :( It's helpful to know here that
you've added a second applyTransform() variant, which now takes a FloatSize.
You should describe the differences between the borderBoxSize and the
boundingRect, and why you have to add the boundingRect.x()/y() offset when
applying this.
I'm aware of the differences (HTML only requires the borderBoxSize, for SVG we
have the objectBoundingBox() only and thus need to calculate percentages
relative to that box, which requires the x/y translation to be respected), but
I guess not anyone is, so this would be helpful to describe.

896    void applyTransform(TransformationMatrix&, const LayoutSize&
borderBoxSize, ApplyTransformOrigin = IncludeTransformOrigin) const;
897    void applyTransform(TransformationMatrix&, const FloatRect& boundingBox,
ApplyTransformOrigin = IncludeTransformOrigin) const;

> Source/WebCore/css/svg.css:68
> +/* transform-origin on SVG elements */

That's obvious ;-)

> Source/WebCore/css/svg.css:74
> +html|* > svg {

I'd add here // Restore default transform origin behavior for html name-spaced
elements in SVG documents.
It's not immediately clear for anyone that 50%, 50% is the HTML default :-)

> Source/WebCore/rendering/style/RenderStyle.cpp:796
> +void RenderStyle::applyTransform(TransformationMatrix& transform, const
LayoutSize& borderBoxSize, ApplyTransformOrigin applyOrigin) const

I woud have made this just call, applyTransform(transform,
FloatRect(FloatPoint(), borderBoxSize), applyOrigin) instead of duplicating
code and eventually mark both inline.
Or do you need to call the non-float calcFloatValue variant here, explicitely?
I'd just try it, run all tests, if nothing new is broken, just go ahead and
avoid code dup.

> LayoutTests/svg/transforms/dynamic-transform-origin.svg:7
> +	   document.getElementById('rect').style.webkitTransformOrigin = "50%
50%";

I'd like to see a copy of this test that does the same but using the
transform-origin attribute. The patch is in that maps the transform-origin
attribute to -webkit-transform-origin CSS property, no? So this should work.