[webkit-reviews] review denied: [Bug 77903] Repaint issues on changing 'viewBox' of inner SVG : [Attachment 127037] Updated Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Bear Travis <betravis at adobe.com>'s
request for review:
Bug 77903: Repaint issues on changing 'viewBox' of inner SVG
https://bugs.webkit.org/show_bug.cgi?id=77903

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=127037&action=review


I think this is the right approach. In general a viewBox is nothing else than a
transform. r- because of some snippets.

> LayoutTests/svg/repaint/inner-svg-change-viewBox-contract.svg:10
> +<!-- blue rect's lower right edges should be visible after viewbox resizing
-->

It is not. Is that because of the repaint logic?

> Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:82
> +    bool needsUpdate = m_needsTransformUpdate;
> +    if (needsUpdate) {
> +	   m_localToParentTransform =
AffineTransform::translation(m_viewport.x(), m_viewport.y()) *
viewportTransform();
> +	   // If this class were ever given a localTransform(), then the above
would read:
> +	   // return viewportTranslation * localTransform() *
viewportTransform()
> +	   m_needsTransformUpdate = false;
> +    }
> +    return needsUpdate;

why not just
if (m_needsTransformUpdate) {
 ...
 return true;
}
return false;

Also, should it be If this class _was_ ever given? :P I am not sure about that
atm :)

> Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:-85
> -    m_localToParentTransform = AffineTransform::translation(m_viewport.x(),
m_viewport.y()) * viewportTransform();
>      return m_localToParentTransform;
> -    // If this class were ever given a localTransform(), then the above
would read:
> -    // return viewportTranslation * localTransform() * viewportTransform()

Did you run all tests to verify that we don't get a regression?

> Source/WebCore/svg/SVGSVGElement.cpp:297
> +	   if (renderer())

Why not:
if (RenderObject* object = renderer())
    object->setNeedsTransformUpdate();

saves a renderer() call.

Also, don't we just need to call it for view box updates?

What about:

if (attrName == SVGNames::viewBoxAttr) {
    if (RenderObject* object = renderer())
	object->setNeedsTransformUpdate();
    updateRelativeLengths = true;
}

After the current if clause?

Of course it would violate the name updateRelativeLengths. What about renaming
it to updateRelativeLengthsOrViewBox?

Of course you have to remove the attName check for viewBox in the later if
clause.