[webkit-reviews] review denied: [Bug 15799] textPath element does not re-render when referenced path changes : [Attachment 99097] 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 15799: textPath element does not re-render when referenced path changes
https://bugs.webkit.org/show_bug.cgi?id=15799

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

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

Looks good, but I think you can even share more code. Can you please look
again? Thanks!

> Source/WebCore/svg/SVGElement.h:148
> +    static PassRefPtr<SubtreeModificationEventListener>
create(SubtreeModificationEventListenerClient* client, String clientId)

const String& please.

> Source/WebCore/svg/SVGElement.h:168
> +    SubtreeModificationEventListener(SubtreeModificationEventListenerClient*
client, String clientId)

Ditto.

> Source/WebCore/svg/SVGPathElement.cpp:249
> +	   if (parentNode()) {

if (Node* parent = parentNode()) {
...
}

> Source/WebCore/svg/SVGPathElement.cpp:250
> +	       RefPtr<MutationEvent> me =
MutationEvent::create(eventNames().DOMSubtreeModifiedEvent, true);

s/me/mutationEvent/, no abbrevations pls ;-)

> Source/WebCore/svg/SVGTextPathElement.cpp:247
> +    if (Element* target = treeScope()->getElementById(id)) {
> +	   ASSERT(!m_eventListener);
> +	   m_eventListener = SubtreeModificationEventListener::create(this,
id);
> +	   ASSERT(target->parentNode());
> +	  
target->parentNode()->addEventListener(eventNames().DOMSubtreeModifiedEvent,
m_eventListener.get(), false);
> +	   if (renderer())

Shouldn't it be possible to reuse more of this code, at lest the
addEventListener stuff and the event listener creation?

> Source/WebCore/svg/SVGTextPathElement.cpp:248
> +	       renderer()->setNeedsLayout(true);

You have to use markForParentResourceInvalidation (or however it was called),
otherwhise you'll miss certain cases.
eg. <text><textPath>...</text> contained in a resource like <pattern>.

> Source/WebCore/svg/SVGTextPathElement.cpp:261
> +    if (renderer())
> +	   renderer()->setNeedsLayout(true);

Ditto. You can use if (RenderObject* renderer = this->renderer()) ... here

> LayoutTests/svg/custom/textPath-startoffset.svg:12
> +	 tp.setAttributeNS(null, "startOffset", "100");

Why setAttributeNS?