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

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


I've reviewed this again, and I think it goes in the wrong direction. Let's
reconsider why we're using DOMSubtreeModifiedEvents at all for <tref>.
<tref> can reference _any_ kind of node, whose text content should be extracted
(especially non-SVG nodes). We'd need to add lots of boilerplate code to both
dom/ and html/ if we want to track textContent changes of arbitrary Nodes. To
avoid that we've used the DOMSubtreeModifiedEvent listener based concept, which
works just fine for <tref>.

For <textPath> those difficulties aren't present. xlink:href for
SVGTextPathElement definition:
An IRI reference to the ‘path’ element onto which the glyphs will be rendered.
If <iri> is an invalid reference (e.g., no such element exists, or the
referenced element is not a ‘path’), then the ‘textPath’ element is in error
and its entire contents shall not be rendered by the user agent.

So it's all about referencing SVPathElements - local or remote <path> elements,
depending on the type of the IRI reference.
Let's only focus about local references like xlink:href="#foo" which is
intended to be fixed by your patch.

Recently I've faced the same problem, properly tracking xlink:href changes for
SVGFEImageElement. I've started from the SVGTRefElement solution, ripped off
the event listener specific parts, and added a generic way to
SVGDocumentExtensions to associate an element 'foo' with a target 'bar' it
references.
Snippet from SVGFEImageElement:buildPendingResource()

	// Register us with the target in the dependencies map. Any change of
hrefElement
	// that leads to relayout/repainting now informs us, so we can react to
it.
	document()->accessSVGExtensions()->addElementReferencingTarget(this,
static_cast<SVGElement*>(target));

I think you should just switch to that approach from SVGFEImageElement and
generalize that, and leave the SVGTRefElement code private.
Also as DOMSubtreeModifiedEvents are deprecated in their current form, we
should rather not depend on it in a broader scope.

Sorry to bring this up late, but I think you'll agree :(

> Source/WebCore/svg/SVGElementRareData.cpp:33
> +    ASSERT(!m_eventListener);

I'd also assert both client and target, and !id.isEmpty().
If this is not guaranteed by the calling site, add early exits instead.

> Source/WebCore/svg/SVGElementRareData.h:116
> +    void
createAndInstallEventListener(SubtreeModificationEventListenerClient*, const
String&, Element*);

createAndInstallSubtreeModificationListener? The current name is too generic.

> Source/WebCore/svg/SVGElementRareData.h:117
> +    void removeEventListenerIfNeeded(TreeScope*);

Ditto.

> LayoutTests/svg/custom/textPath-change-id-pattern.svg:11
> +	 layoutTestController.waitUntilDone();

Is that needed? Is DRT dumping before onload fires?

> LayoutTests/svg/custom/textPath-change-reference-pattern.svg:15
> +	 var path = document.createElementNS("http://www.w3.org/2000/svg",
"path");

That variable is unused.

> LayoutTests/svg/custom/textPath-change-reference-using-baseval-pattern.svg:15

> +	 var path = document.createElementNS("http://www.w3.org/2000/svg",
"path");

Ditto.

> LayoutTests/svg/custom/textPath-change-reference-using-baseval-pattern.svg:18

> +	 tp.href.baseVal = "#thePath";
> +	 tp.setAttributeNS("http://www.w3.org/1999/xlink","href","#thePath");

I guess you only want to change using tp.href.baseVal here.

> LayoutTests/svg/custom/textPath-change-reference.svg:12
> +	 var path = document.createElementNS("http://www.w3.org/2000/svg",
"path");

Unused.

> LayoutTests/svg/custom/textPath-change-reference2-pattern.svg:16
> +	 var path = document.createElementNS("http://www.w3.org/2000/svg",
"path");

Unused.

> LayoutTests/svg/custom/textPath-change-reference2-using-baseval.svg:13
> +	 var path = document.createElementNS("http://www.w3.org/2000/svg",
"path");

Ditto.

> LayoutTests/svg/custom/textPath-change-reference2.svg:13
> +	 var path = document.createElementNS("http://www.w3.org/2000/svg",
"path");

Ditto.

> LayoutTests/svg/custom/textPath-path-change-pattern.svg:13
> +	 layoutTestController.dumpAsText(true);

Can't you make all of these reftests? All tests that currently use
dumpAsText(true).
That'll reduce the chromium diffs to a minimum, if not zero.

> LayoutTests/svg/custom/textPath-set-id.svg:10
> +    var subtreeModifiedEventsSeen = 0;
> +

Do we currently (without your patch) fire mutation events for id changes?

> LayoutTests/svg/custom/textPath-set-id.svg:22
> +	 path.setAttribute("id","foo");

here