[webkit-reviews] review denied: [Bug 69459] viewBox on nested SVG causes wrong content size for relative values : [Attachment 125325] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 69459: viewBox on nested SVG causes wrong content size for relative values
https://bugs.webkit.org/show_bug.cgi?id=69459

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

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


> Source/WebCore/rendering/svg/RenderSVGContainer.cpp:68
> +    bool selfNeedsLayout = this->selfNeedsLayout();

No need to cache this, as you're calling an inline function.

The whole logic for this should be in RenderSVGViewportContainer only, not in
RenderSVGContainer - you're wasting memory by storing/tracking
m_isLayoutSizeChanged for eg. any <g> now.

> Source/WebCore/rendering/svg/RenderSVGContainer.cpp:74
> +	   SVGSVGElement* svg = static_cast<SVGSVGElement*>(node());
> +	   ASSERT(svg);
> +	   m_isLayoutSizeChanged = svg->hasRelativeLengths() &&
m_needsBoundariesUpdate;

No need to ASsERT(svg) here anymore. Just use m_isLayoutSizeChanged =
static_cast<SVGSVGElement*>(node())->hasRelativeLengths ... but only do this
once you moved this into RenderSVGViewportContainer.

> LayoutTests/svg/custom/dynamic-viewBox-2.svg:14
> +    if (window.layoutTestController)
> +	   layoutTestController.waitUntilDone();
> +
> +    window.setTimeout(function() {
> +	   document.getElementById('s').setAttribute("viewBox", "0 0 200 200");

> +
> +	   if (window.layoutTestController)
> +	       layoutTestController.notifyDone();
> +    },0);

That's considered harmful nowadays, what you want here is:

<script>
if (window.layoutTestController)
     layoutTestController.waitUntilDone();
function runTest() {
    document.rootElement.offsetLeft;
    if (window.layoutTestController)
	layoutTestController.display();

    document.getElementById('s').setAttribute(...)

    if (window.layoutTestController)
	layoutTestController.notifyDone();
}
setTimeout(runTest, 0);
</script>

There's no guarantee otherwise that you're painting before the change --
currently with your test its painted the first time after your
setAttribute("viewBox") not before, so you can't test dynamic changes with this
approach.
I learned this the hard way.