[webkit-reviews] review denied: [Bug 63390] SVG1.1SE test text-tref-03-b.svg fails : [Attachment 98626] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Rob Buis
<rwlbuis at gmail.com>'s request for review:
Bug 63390: SVG1.1SE test text-tref-03-b.svg fails
https://bugs.webkit.org/show_bug.cgi?id=63390

Attachment 98626: Patch
https://bugs.webkit.org/attachment.cgi?id=98626&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=98626&action=review

Looks much better, still some comments:

> Source/WebCore/svg/SVGTRefElement.cpp:59
> +    }
> +    static const SubtreeModificationEventListener* cast(const EventListener*
listener)

Style, add a newline here inbetween.

> Source/WebCore/svg/SVGTRefElement.cpp:68
> +	  
target->parentNode()->removeEventListener(eventNames().DOMSubtreeModifiedEvent,
this, false);

Can you guarantee target is always valid? ASSERT(target) then; or add an early
return.
Same for the parentNode. ASSERT(target->parentNode()) or early return.
I guess you want to add assertions.

> Source/WebCore/svg/SVGTRefElement.cpp:136
> +    String id = SVGURIReference::getTarget(href());
> +    Element* oldTarget = treeScope()->getElementById(id);
>      if (SVGURIReference::parseMappedAttribute(attr)) {
> +	   id = SVGURIReference::getTarget(href());
> +	   Element* target = treeScope()->getElementById(id);
> +	   if (!target) {
> +	       document()->accessSVGExtensions()->addPendingResource(id, this);

> +	       return;
> +	   }
> +	   if (m_eventListener) {
> +	       m_eventListener->removeFromTarget(oldTarget);
> +	       m_eventListener = 0;
> +	   }
> +	   m_eventListener = SubtreeModificationEventListener::create(this);
> +	  
target->parentNode()->addEventListener(eventNames().DOMSubtreeModifiedEvent,
m_eventListener.get(), false);

This logic belongs in "svgAttributeChanged", not in parseMappedAttribute, as
you only handle XML DOM changes now.
Remember that you can also change the xlink:href attribute via SVG DOM,
tref.href.baseVal = "someRef";

> Source/WebCore/svg/SVGTRefElement.cpp:137
>	   updateReferencedText();

This also has to go into svgAttributeChanged. It clearly shows xlink:href
changes on <tref> via SVG DOM don't work as expected (never trigger a change)
even without your patch in trunk.
Please move it as well, and duplicate your test, but use SVG DOM to modify that
href property.

> Source/WebCore/svg/SVGTRefElement.cpp:230
> +    // set up subtree listener

This seems obvious, I think you can omit this.

Any specific reason you have to call updateReferencedText() _first? ? You did
it the other way round in parseMappedAttribute.

> Source/WebCore/svg/SVGTRefElement.cpp:239
> +    SVGStyledElement::removedFromDocument();

Is it safe to call into the base class first and then destruct your event
listener? Just asking to be sure.

> Source/WebCore/svg/SVGTRefElement.cpp:246
> +    Element* target =
treeScope()->getElementById(SVGURIReference::getTarget(href()));
> +    if (target) {

Combine this into one line.

> LayoutTests/svg/custom/text-tref-03-b-change-href.svg:19
> +	    tref.setAttributeNS("http://www.w3.org/1999/xlink", "href",
"#hello");

You want to duplicate this test using SVG DOM.