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

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Renata Hodovan
<reni at webkit.org>'s request 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 Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=130373&action=review


Discussed some things with reni on IRC, for the record:

> Source/WebCore/svg/SVGUseElement.cpp:163
> +	       if (isExternalURIReference(href())) {

Oh, just noticing that this is in parseAttribute, not in svgAttributeChanged.
You won't react properly to SVG DOM changes like
myUseElement.href.baseVal.value = "some-external-reference"; it only works
through setAttribute() using this way.
Move it into svgAttributeChanged, and it should work - please add a testcase
covering this.

> Source/WebCore/svg/SVGUseElement.cpp:248
> +	       || SVGExternalResourcesRequired::isKnownAttribute(attrName)) {

Please restore the formatting.

> Source/WebCore/svg/SVGUseElement.cpp:270
> +//	     SVGUseElement* correspondingUseElement =
static_cast<SVGUseElement*>(element);

Commented code should go away.

> Source/WebCore/svg/SVGUseElement.cpp:919
> +    for (SVGElementInstance* node = targetElementInstance->firstChild();
node; node = node->nextSibling()) {

I'd s/node/instance/ here otherwise this is confusing. You're walking a
SVGElementInstance tree here, not a DOM tree.

> Source/WebCore/svg/SVGUseElement.h:1
> +/* Copyright (C) 2004, 2005, 2006, 2007, 2008 Nikolas Zimmermann
<zimmermann at kde.org>

Hm?

> Source/WebCore/svg/SVGUseElement.h:107
> +    Document* referencedDocument() const;
> +    virtual void notifyFinished(CachedResource*);

For beauty, reorder this :-)