[webkit-reviews] review denied: [Bug 54538] symbol display <use> at wrong scale : [Attachment 83280] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Leo Yang
<leo.yang at torchmobile.com.cn>'s request for review:
Bug 54538: symbol display <use> at wrong scale
https://bugs.webkit.org/show_bug.cgi?id=54538

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

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

Great work, r- because we need more test coverage:

> Source/WebCore/svg/SVGElementInstance.h:63
> -    void clearUseElement() { m_useElement = 0; }
> +    void clearCorrespondingUseElement() { m_correspondingUseElement = 0; }
> +    void clearDirectUseElement() { m_directUseElement = 0; }

I'd leave clearUseElement() as is, and let it clear both
m_correspdondingUseElement and directUseElement, as you always want to clear
both of them in parallel.

> Source/WebCore/svg/SVGUseElement.cpp:395
> +    String directUseElementName = directUseElement ?
directUseElement->nodeName() : "null";

This is fine, as it's just debugging code.

> Source/WebCore/svg/SVGUseElement.cpp:551
> +    m_targetElementInstance->setDirectUseElement(this);

I'd remove this line, and directly initialize m_directUseElement to 'this' in
the SVGElementInstance constructor, by using the first constructor param, just
like it's done for m_correspondingUseElement.

> Source/WebCore/svg/SVGUseElement.cpp:756
> +   
newInstancePtr->setDirectUseElement(static_cast<SVGUseElement*>(target));

Very good catch!

I guess correspondingUseElement should stay as is:
"
The corresponding ‘use’ element to which this SVGElementInstance object
belongs. When ‘use’ elements are nested (e.g., a ‘use’ references another ‘use’
which references a graphics element such as a ‘rect’), then the
correspondingUseElement is the outermost ‘use’ (i.e., the one which indirectly
references the ‘rect’, not the one with the direct reference)."

Can you please extend the testcase, to check that the generated
SVGElementInstance is just like SVG describes it here?