[webkit-reviews] review denied: [Bug 63390] SVG1.1SE test text-tref-03-b.svg fails : [Attachment 98609] 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 98609: Patch
https://bugs.webkit.org/attachment.cgi?id=98609&action=review

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

Brilliant idea to listen to subtree modified events! Some comments

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:25927
> +				GCC_VERSION = com.apple.compilers.llvmgcc42;

You need to remove that from the patch.

> Source/WebCore/svg/SVGTRefElement.cpp:56
> +	   { return adoptRef(new
SubtreeModificationEventListener(trefElement)); }

Wrong indentation.

> Source/WebCore/svg/SVGTRefElement.cpp:61
> +	   return listener->type() == CPPEventListenerType
> +	       ? static_cast<const SubtreeModificationEventListener*>(listener)

> +	       : 0;

No need tow rap lines like this.

> Source/WebCore/svg/SVGTRefElement.cpp:64
> +    virtual bool operator==(const EventListener& other);

You can omit the "other".

> Source/WebCore/svg/SVGTRefElement.cpp:81
> +    if (const SubtreeModificationEventListener* imageEventListener =
SubtreeModificationEventListener::cast(&listener))
> +	   return m_trefElement == imageEventListener->m_trefElement;

imageEvent? Where did you copy that from? ;-)

> Source/WebCore/svg/SVGTRefElement.cpp:215
> +    RefPtr<EventListener> listener =
SubtreeModificationEventListener::create(this);

Don't you want to destruct this listener if the SVGTRefElement gets destructed?
Otherwhise a dangling pointer is still stored in
SubtreeModificationEventListener.