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!
checked on nightly builds safari & chrome, mac only
Created attachment 83279 [details] Patch
Created attachment 83280 [details] Patch
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?
(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.
(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...
(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...
(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.
(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.
Created attachment 83435 [details] Revised patch
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.
Created attachment 84851 [details] Revised patch version 2 Updated. Attach pixel results for qt platform.
(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.
Comment on attachment 84851 [details] Revised patch version 2 LGTM. Testing the patch locally and create pixel tests for Mac here.
Committed r81578: <http://trac.webkit.org/changeset/81578>
Thanks all for the comments and helps to the patch.
wfm tx ~:"