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

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

Some more comments that lead to r-:

> Source/WebCore/svg/SVGPathElement.cpp:226
> +	   else
> +	       notifyReferencingClients();

This approach is flawed, you're "misusing the pending resources concept".
It should only be used if an element references another element, that's not
existant.
If that element appears SVGStyledElement::insertedIntoDocument takes care of
notifying those clients, that depend on the resource.

> Source/WebCore/svg/SVGTextPathElement.cpp:202
> -    Element* targetElement = treeScope()->getElementById(id);
> -    if (!targetElement) {
> -	   document()->accessSVGExtensions()->addPendingResource(id, this);
> -	   return;
> -    }
> +    document()->accessSVGExtensions()->addPendingResource(id, this);

I think this change is wrong.
What if the element named 'id' has already been inserted into the tree?
Then it will say in the pending resources list forever, no?

> Source/WebCore/svg/SVGTextPathElement.cpp:217
> +    String id = SVGURIReference::getTarget(href());
> +    document()->accessSVGExtensions()->addPendingResource(id, this);

Same here.
Ok, I see now why your approach works, you're basically assuring that the
referenced path element is always "in pending state", so that any update of the
dAttr reaches the SVGTextPathElement.
As minimum, I think you should describe why you're forcing the <path> to be in
the pending resources list, otherwhise this code is quite confusing.

> LayoutTests/svg/custom/textPath-startoffset.svg:1
> +<svg xmlns="http://www.w3.org/2000/svg"
xmlns:xlink="http://www.w3.org/1999/xlink" onload="setTimeout(runTest, 0)">

I think we're missing several testcases here:
1. <textPath> references non-existing <path>, <path> added dynamically ->
verify updating works.
2. <textPath> references existing <path>, <path> is removed from DOM -> verify
updating works.
3. <textPath> references existing <path>, change the xlink:href to a
non-existing one
4. <textPath> references non-existing <path>, change xlink:href to an existing
one

Can you add more testcases? I'll bet we still fail 2/3/4.