Bug 54538 – symbol display <use> at wrong scale

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[jay]

compare Opera or Mozilla,

open URL

where <use> references another <use> the item is displayed at an alternate scale.

is there a standards or specification reason for this?
please also note that some parts are significantly displaced,
ie note small rain drop near true scale 'weather' symbol,
the very last one...

almost half are missing parts, and most are wrong scale!

[jay]

checked on nightly builds safari & chrome, mac only

[Leo Yang]

Created attachment 83279 [details]
Patch

[Leo Yang]

Created attachment 83280 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 83280 [details]
Patch

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?

[Leo Yang]

(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.

[Nikolas Zimmermann]

(In reply to comment #5)
> > This is fine, as it's just debugging code.
> Can you make me clear about this comment?
Well, this code is not efficient, but it's not a problem, as it's only the dumpInstanceTree debug code, which is not compiled in by default, just wanted to leave a 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.
No need to add a parameter, the first param is already containing 'this'.

> > 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.
Great.

> 
> > 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.
Looking forward to these...

[Leo Yang]

(In reply to comment #6)
> (In reply to comment #5)
> > > This is fine, as it's just debugging code.
> > Can you make me clear about this comment?
> Well, this code is not efficient, but it's not a problem, as it's only the dumpInstanceTree debug code, which is not compiled in by default, just wanted to leave a comment :-)
> 
Thanks.

> > > > 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.
> No need to add a parameter, the first param is already containing 'this'.
> 
'this' is corresponding use element instead of direct referencing use element.
corresponding use element is not same as direct referencing use element in some cases.

> > > 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.
> Great.
> 
> > 
> > > 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.
> Looking forward to these...

[Nikolas Zimmermann]

(In reply to comment #7)
> 'this' is corresponding use element instead of direct referencing use element.
> corresponding use element is not same as direct referencing use element in some cases.
What you currently do is: initialize m_directUseElement to 0 in the constructor.
Then you call setDirectUseElement(this) from SVGUseElement _or_ setDirectUseElement(target).
So it's perfectly fine to use the same values as correspondingUseElement as default for directUseElement.
This just saves to call setDirectUseElement(this) in one place.

[Leo Yang]

(In reply to comment #8)
> (In reply to comment #7)
> > 'this' is corresponding use element instead of direct referencing use element.
> > corresponding use element is not same as direct referencing use element in some cases.
> What you currently do is: initialize m_directUseElement to 0 in the constructor.
> Then you call setDirectUseElement(this) from SVGUseElement _or_ setDirectUseElement(target).
> So it's perfectly fine to use the same values as correspondingUseElement as default for directUseElement.
> This just saves to call setDirectUseElement(this) in one place.

If we initialize m_directUseElement to correspondingUseElement, we need to call setDirectUseElement(0) for instances which have no direct use element. This will be a lot of calls in for loop in buildInstanceTree(). So I think adding a parameter for direct use element in constructor and removing setDirectUseElement is more efficient. In this way, all callers should pass direct use element explicitly.

[Leo Yang]

Created attachment 83435 [details]
Revised patch

[Dirk Schulze]

Comment on attachment 83435 [details]
Revised patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83435&action=review

The patch looks great. Can you please update the tests? See comments:

> LayoutTests/svg/custom/use-transfer-width-height-properties-to-svg.svg:2
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.0" width="200" height="200">

Can you use 400x400 here to get a 100x100 rect?

> LayoutTests/svg/custom/use-transfer-width-height-properties-to-svg2.svg:2
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.0" width="200" height="200">

Ditto.

> LayoutTests/svg/custom/use-transfer-width-height-properties-to-symbol.svg:2
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.0" width="200" height="200">

Ditto.

> LayoutTests/svg/custom/use-transfer-width-height-properties-to-symbol2.svg:2
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.0" width="200" height="200">

Ditto.

[Leo Yang]

Created attachment 84851 [details]
Revised patch version 2

Updated. Attach pixel results for qt platform.

[Dirk Schulze]

(In reply to comment #12)
> Created an attachment (id=84851) [details]
> Revised patch version 2
> 
> Updated. Attach pixel results for qt platform.

Ah, I'm really sorry. I waited for the update and didn't notice that you already upload the patch.

[Dirk Schulze]

Comment on attachment 84851 [details]
Revised patch version 2

LGTM. Testing the patch locally and create pixel tests for Mac here.

[Dirk Schulze]

Committed r81578: <http://trac.webkit.org/changeset/81578>

[Leo Yang]

Thanks all for the comments and helps to the patch.

[jay]

wfm tx ~:"