[webkit-reviews] review denied: [Bug 69459] viewBox on nested SVG causes wrong content size for relative values : [Attachment 125348] 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 125348: Patch
https://bugs.webkit.org/attachment.cgi?id=125348&action=review

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


> Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:44
> +    SVGRenderSupport::layoutChildren(this, selfNeedsLayout() ||
SVGRenderSupport::filtersForceContainerLayout(this));

Call the base class here instead of duplicating this.

> Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:45
> +    m_isLayoutSizeChanged = false;

Why is it necessary to reset this? Does anyone query it after
SVGRenderSupport::layoutChildren() ran, I don't think so?

> Source/WebCore/svg/SVGSVGElement.cpp:297
> +	   || attrName == SVGNames::viewBoxAttr
>	   || SVGFitToViewBox::isKnownAttribute(attrName)) {
>	   updateRelativeLengths = true;
>	   updateRelativeLengthsInformation();

Just noticed that this is wrong. You should add a new case below instead of
here:
  if (updateRelativeLengths
    || attrName == SVGNames::viewBoxAttr  
    || SVGLangSpace::isKnownAttribute(attrName)
    || SVGExternalResourcesRequired::isKnownAttribute(attrName)

... as you shouldn't call updateRelativeLengthsInformation() if viewBox
changes, only if x/y/width/height changed.

> LayoutTests/svg/custom/dynamic-viewBox-2.svg:7
> +    if (window.layoutTestController)
> +	    layoutTestController.waitUntilDone();

Test still not updated.