[webkit-reviews] review denied: [Bug 78631] getCTM() on SVG root element with borders, paddings, and viewbox returns incorrect values : [Attachment 127210] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Max Vujovic
<mvujovic at adobe.com>'s request for review:
Bug 78631: getCTM() on SVG root element with borders, paddings, and viewbox
returns incorrect values
https://bugs.webkit.org/show_bug.cgi?id=78631

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

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


Nice patch Max, almost there!

> LayoutTests/svg/dom/SVGLocatable-getCTM-svg-root.html:12
> +	   <script src="../../fast/js/resources/js-test-pre.js"></script>

I think it would be helpful, if this testcase would use
"window.enablePixelTesting = true" before including js-test-pre.js, to dump
pixels as well.

> LayoutTests/svg/dom/SVGLocatable-getCTM-svg-root.html:32
> +	   <svg id="svgRoot1" xmlns="http://www.w3.org/2000/svg"

Inside a HTML5 document, no xmlns is needed here.

> LayoutTests/svg/dom/SVGLocatable-getCTM-svg-root.html:34
> +		viewbox="0 0 100 100"">

Oh, when the HTML5 parses parses this file, lower case attributes are allowed??
The correct name is 'viewBox'.
I'm not sure if HTML5 explicitly allows this, if not please use "viewBox".

> Source/WebCore/ChangeLog:22
> +	       SVGSVGElement::currentViewportSize was using the SVG root
element's frameRect to compute
> +	       the viewport size. However, the frameRect includes borders and
paddings, which should
> +	       not be considered part of the SVG viewport.
> +
> +	       SVGSVGElement::currentViewportSize now uses the contentBoxRect
instead of the frameRect.
> +	       The contentBoxRect corresponds to the SVG viewport and does not
include borders and
> +	       paddings.

Double explanation, either is okay.

> Source/WebCore/svg/SVGSVGElement.cpp:557
>      FloatRect frameRect =
toRenderSVGViewportContainer(renderer())->viewport();

I'd rename this as well for consistency to avoid confusion.