[Webkit-unassigned] [Bug 54538] symbol display <use> at wrong scale

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=54538





--- Comment #5 from Leo Yang <leo.yang at torchmobile.com.cn>  2011-02-22 02:16:28 PST ---
(In reply to comment #4)
> (From update of attachment 83280 [details])
> 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.
Yes, I think changing clearUseElement() to clearUseElements() and clearing the 2 variable is more precise.

> 
> > Source/WebCore/svg/SVGUseElement.cpp:395
> > +    String directUseElementName = directUseElement ? directUseElement->nodeName() : "null";
> 
> This is fine, as it's just debugging code.
Can you make me clear about this comment?

> 
> > 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.
OK. Will remove setDirectUseElement and add a parameter to constructor.

> 
> > 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)."
> 
Yes, corresondingUseElement stays as is. This patch doesn't alter it except change its name from m_useElement to m_correspondingUseElement to make it more
clear.

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

OK. Will add more test cases.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.