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

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

Poor soul, I still have comments.... sorry :-)

> Source/WebCore/svg/SVGElement.h:148
> +class SubtreeModificationEventListenerClient {
> +public:
> +    virtual ~SubtreeModificationEventListenerClient() { }
> +    virtual void subtreeModifiedEvent(Event*) = 0;
> +};
> +
> +class SubtreeModificationEventListener : public EventListener {
> +public:
> +    static PassRefPtr<SubtreeModificationEventListener>
create(SubtreeModificationEventListenerClient* client, const String& clientId)

Sorry, I didn't see it earlier. Why is this in SVGElement? It's quite heave
already... Doesn't this deserve its own file?

> Source/WebCore/svg/SVGTRefElement.cpp:185
> +void SVGTRefElement::createAndInstallEventListener(const String& id,
Element* target)
> +{
> +    ASSERT(!m_eventListener);
> +    m_eventListener = SubtreeModificationEventListener::create(this, id);
> +    ASSERT(target->parentNode());
> +   
target->parentNode()->addEventListener(eventNames().DOMSubtreeModifiedEvent,
m_eventListener.get(), false);
> +}

This is a good stop in the right direction. But still you're duplicating this
code in both SVGTRefElement and SVGTextPathElement.
Yesterday, I only had time for a quick review, sorry. What I think is
reasonable would be to store the "m_eventListener" in SVGElementRareData, so it
can be used by all SVG*Element derived classes, but only allocating memory if
its really needed.

The createAndInstallEventListener function could be a member of
SVGElementRareData, and thus be shared between tref/textpath.

> Source/WebCore/svg/SVGTextPathElement.cpp:124
> +	   if (m_eventListener) {
> +	       m_eventListener->removeFromTarget(treeScope());
> +	       m_eventListener = 0;
> +	   }
> +	   String id = SVGURIReference::getTarget(href());
> +	   Element* target = treeScope()->getElementById(id);
> +	   if (!target)
> +	       document()->accessSVGExtensions()->addPendingResource(id, this);

> +	   else 
> +	       createAndInstallEventListener(id, target);

This could also live in SVGElementRareData as new function, with a parameter:
enum ShouldAddToPendingResources {
    AddToPendingResources,
   DontAddToPendingResources
};
This way both tref and textpath could use the same function here in
svgAttributeChanged, no code dup.

> Source/WebCore/svg/SVGTextPathElement.cpp:247
> +    if (Element* target = treeScope()->getElementById(id)) {

If there's more than one line, inside the if branch, an early return suits
better.

> LayoutTests/ChangeLog:16
> +

Ok I'm evil, but I have to ask for more testcases, now that we store a "String
m_clientId" somewhere.
Say <textPath> references an non-existing <path> with id "foo". An existing
path with id "bar", is changed via setAttribute("id", "foo"). Does the textPath
notify that change?
Counter testcase: <textPath> references an existing <path> with id "foo". If I
change id foo to "bar", will it notice this change?

Can you also add another testcase where a <text><textPath> construct is inside
a <pattern>, and then you change the d attribute of the referenced path, change
the textPath id, etc. etc. (basically all exsiting testcases duplicated, and
contained in  <pattern>). I know this is lots of work - though we assured when
rewriting the pending resources concept for resources
(gradient/pattern/mask/clipPath) that we have tests covering all these
situations.
We need the same for textPath/tref.