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

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

The new tests look great, the code changes as well. I'm still concerned about
the mutation event, and I should have brought this up while reviewing the
<tref> patch when the event listener logic was initially added. In the <tref>
patch you only listened for these events, here you're changing
SVGPathElement/SVGStyledElement to actually fire them, manually, in some cases.
See below for my findings:

> LayoutTests/svg/custom/textPath-change-id.svg:7
> +    if (window.layoutTestController)

All tests the just say 'PASS' could be turned into using:
layoutTestController.dumpAsText(true).
This will create expected.txt files that contain the dump-as-text output,
instead of the render tree dump, while still creating pixel test results.
This sounds perfectly fine and will adress Dirks comment as well.
Those expected.txt files should then go right into svg/custom, the pngs in
platform/mac/svg/custom. Sounds good, eh?

> Source/WebCore/svg/SVGPathElement.cpp:254
> +	   if
(document()->hasListenerType(Document::DOMSUBTREEMODIFIED_LISTENER)) {
> +	       // Since setting an already set attribute fires no
MutationEvent, we have to fire it ourselves
> +	       // to notify listeners.
> +	       if (Node* parent = parentNode()) {
> +		   RefPtr<MutationEvent> mutationEvent =
MutationEvent::create(eventNames().DOMSubtreeModifiedEvent, true);
> +		   parent->handleLocalEvents(mutationEvent.get());

This is not sufficient, yet I fear.
Consider you're constructing a testcase like:
<svg xmlns="..">
<script>
    document.addEventListener('DOMSubtreeModified', onSubtreeModified, false);
    function onSubtreeModified(event) { alert('mutation!'); }
</script>
<path d="..."/>
</svg>

Is this event handler fired now, during construction of the <path>? Does it
fire twice? I think it'll do and that's not right.
Remember that the subtree modified event, even though it's deprecated in DOM
Level 3 Events, is exposed to the web.

If I'm right, you need to know whether the svgAttributeChanged call is coming
from SVG DOM (through svg/properties/SVGANimatedProperty*) or via XML DOM from
attributeChanged().
Let's see what your findings are.

> Source/WebCore/svg/SVGStyledElement.cpp:350
> +	   if
(document()->hasListenerType(Document::DOMSUBTREEMODIFIED_LISTENER)) {

Ditto, same concerns as above. Should also be able to construct a testcase that
breaks.