[webkit-reviews] review requested: [Bug 12499] External <use> xlink:href references do not work : [Attachment 130373] Proposed second part

[bugzilla-daemon at webkit.org]

Renata Hodovan <reni at webkit.org> has asked  for review:
Bug 12499: External <use> xlink:href references do not work
https://bugs.webkit.org/show_bug.cgi?id=12499

Attachment 130373: Proposed second part
https://bugs.webkit.org/attachment.cgi?id=130373&action=review

------- Additional Comments from Renata Hodovan <reni at webkit.org>
> > LayoutTests/svg/custom/struct-use-recursion-02-t.svg:1
> > +<?xml version="1.0" encoding="UTF-8"?>
> 
> Where did you grab these from? If its SVG 1.1 2nd edition testsuite, these
should go to svg/W3C-SVG-1.1-SE/
They were moved into a new W3C-SVG-1.2-Tiny directory because they come from
that testsuite.

> > LayoutTests/svg/dynamic-updates/SVGUseElement-dom-href2-attr-expected.txt:1

> > +CONSOLE MESSAGE: line 10: ReferenceError: Can't find variable: repaintTest

> 
> Oops. This testcase doesn't work.
Sorry, updated.

> > Source/WebCore/svg/SVGUseElement.cpp:268
> > +	 SVGUseElement* correspondingUseElement =
static_cast<SVGUseElement*>(correspondingElement);
> > +	 if (correspondingUseElement->cachedDocumentIsStillLoading())
> 
> Huh, how can you be sure its a SVGUseElement? Even if this is debug code,
this crashes :-) You need to check the hasTagName first, before casting.
Right. Fixed.

> > Source/WebCore/svg/SVGUseElement.cpp:746
> > +	     RefPtr<SVGSVGElement> svgElement =
SVGSVGElement::create(SVGNames::svgTag, document);
> 
> How about you move the assert(document) into referencedDocument. Then you
could just do a s/document()/referencedDocument()/ and save introducing local
variables here.
Good idea. Done.

> > Source/WebCore/svg/SVGUseElement.cpp:921
> > +	 Element* parent = parentElement();
> > +	 if (!parent->renderer())
> > +	     return;
> > +
> > +	
RenderSVGResource::markForLayoutAndParentResourceInvalidation(parent->renderer(
));
> 
> I asked this before I think: why the parent->renderer() only? Why not this?
> Is this needed at all, when doing invalidateShadowTree(). I would have
expected that this is enough. If not, then
markForLayoutAndParentResourceInvalidation(renderer()), but not
parent->renderer().
Really, we don't need it anymore. It's thrown off.

> > Source/WebCore/svg/SVGUseElement.cpp:934
> > +	     if (node->correspondingUseElement()) {
> 
> if (SVGUseElement* use = node->correspondingUseElement()) {
> ..
> Saves calling it twice.
Done.