Bug 15799

I have a reduction.

Created attachment 17047 [details] test case

I expect we'll need some <use>-like notification system. I expect we can just re-use that system and wire it into textPath (like we do for pattern, marker, etc.

Created attachment 27683 [details] First attempt This covers not just the bug itself, but also changing attributes on the textPath. Cheers, Rob.

(In reply to comment #4) > Created an attachment (id=27683) [review] > First attempt > > This covers not just the bug itself, but also changing attributes on the > textPath. > Cheers, > Hi Rob, the patch looks nice! Though I think the testcase should be rewritten, because it's not very obvious what happens first, what next etc.. I'd love to see more testcases: - one for a textPath attribute change (ie. startOffset) - one for a textPath child node change (modifying text child of <textPath>). - one which covers updating multiple clients (two references of the textPath) Your testcase doesn't look very obvious (what happens first, it's not clear from a first sight, what is executed at which time, etc..) Also we shouldn't add new 'custom' testcases, but you should use the make-js-test-wrappers framework for this. Again the patch is great, just lacking some extensive tests. Cheers, Niko

CCing Rob, in case he doesn't track the bug.

Hi Niko, (In reply to comment #5) > (In reply to comment #4) > > Created an attachment (id=27683) [review] [review] > > First attempt > > > > This covers not just the bug itself, but also changing attributes on the > > textPath. > > Cheers, > > > Hi Rob, > > the patch looks nice! Though I think the testcase should be rewritten, because > it's not very obvious what happens first, what next etc.. Ok. The thing wss that the reduction was not enough, i.e. at one point I fixed it but the original bug url still failed. > I'd love to see more testcases: > - one for a textPath attribute change (ie. startOffset) > - one for a textPath child node change (modifying text child of <textPath>). > - one which covers updating multiple clients (two references of the textPath) Sounds reasonable. I'll have to check whether the second case is not actually covered somewhere already, since the childrenChanged code is already there. > Your testcase doesn't look very obvious (what happens first, it's not clear > from a first sight, what is executed at which time, etc..) Also we shouldn't > add new 'custom' testcases, but you should use the make-js-test-wrappers > framework for this. Ok, I am not familiar with make-js-test-wrappers, will have a look. > Again the patch is great, just lacking some extensive tests. Thanks! I'll try to improve the testcases as you pointed out an post a new patch this week. Cheers, Rob.

Created attachment 27854 [details] More tests I created the testcases Niko stated. However I see no way to use make-js-test-wrappers as I see no way in js to calculate the position of the textPath after the tests, for instance when the path or startoffset is changed. AFAICS we still need pixel tests for that, so I am doing it the old way for now unless someone can tell me how to make it work with make-js-test-wrappers. Cheers, Rob.

Comment on attachment 27854 [details] More tests I don't understand why your tests need to click? are you trying to select the text? If so there are better APIs for that. :) I'm also not sure why your tests need to use setTimeout? Why can't they run the js during the parse? Is the setTimeout there to try and make sure webkit does a layout before running the second set of javascript?

Comment on attachment 27854 [details] More tests r- based on my above comments. This needs a reply from Rob.

Created attachment 95436 [details] Patch

(In reply to comment #11) > Created an attachment (id=95436) [details] > Patch So this patch is updated to modern times and I tried to improve the tests to address Eric's concerns. Let me know what you think :) Cheers, Rob.

Comment on attachment 95436 [details] Patch 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.

Created attachment 99097 [details] Patch

Comment on attachment 99097 [details] Patch Attachment 99097 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8959261 New failing tests: svg/custom/textPath-insert-path.svg svg/custom/textPath-remove-path.svg svg/custom/textPath-path-change2.svg svg/custom/textPath-startoffset.svg svg/custom/textPath-change-reference.svg svg/custom/textPath-path-change.svg svg/custom/textPath-change-reference2.svg

Created attachment 99104 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 99097 [details] Patch 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?

Hi Niko, (In reply to comment #17) > (From update of attachment 99097 [details]) > 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. Fixed. > > Source/WebCore/svg/SVGElement.h:168 > > + SubtreeModificationEventListener(SubtreeModificationEventListenerClient* client, String clientId) > > Ditto. Fixed. > > Source/WebCore/svg/SVGPathElement.cpp:249 > > + if (parentNode()) { > > if (Node* parent = parentNode()) { > ... > } Fixed. > > Source/WebCore/svg/SVGPathElement.cpp:250 > > + RefPtr<MutationEvent> me = MutationEvent::create(eventNames().DOMSubtreeModifiedEvent, true); > > s/me/mutationEvent/, no abbrevations pls ;-) Fixed. > > 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? I am not sure how? Do you mean create can add the event listener itself? > > 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>. Fixed. > > Source/WebCore/svg/SVGTextPathElement.cpp:261 > > + if (renderer()) > > + renderer()->setNeedsLayout(true); > > Ditto. You can use if (RenderObject* renderer = this->renderer()) ... here Fixed. > > LayoutTests/svg/custom/textPath-startoffset.svg:12 > > + tp.setAttributeNS(null, "startOffset", "100"); > > Why setAttributeNS? You are right, not needed, fixed. Cheers, Rob.

Created attachment 99152 [details] Patch

Comment on attachment 99152 [details] Patch Attachment 99152 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8966169

Comment on attachment 99152 [details] Patch Attachment 99152 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8958388

Comment on attachment 99152 [details] Patch Attachment 99152 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8960309

Created attachment 99164 [details] Patch

Comment on attachment 99164 [details] Patch Attachment 99164 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8960331 New failing tests: svg/custom/textPath-insert-path.svg svg/custom/textPath-remove-path.svg svg/custom/textPath-path-change2.svg svg/custom/textPath-startoffset.svg svg/custom/textPath-change-reference.svg svg/custom/textPath-path-change.svg svg/custom/textPath-change-reference2.svg

Created attachment 99172 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 99164 [details] Patch 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.

Created attachment 99588 [details] Patch

Comment on attachment 99588 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99588&action=review Good job! Some comments: In general patch doesn't apply so you'll need to reupload it, so we get EWS results. > LayoutTests/ChangeLog:6 > + Reviewed by NOBODY (OOPS!). > + > + Bug 15799: textPath element does not re-render when referenced path changes > + https://bugs.webkit.org/show_bug.cgi?id=15799 According to a recent thread on webkit-dev, these blocks need to be swapped. Bug block comes first. > Source/WebCore/ChangeLog:5 > + Need a short description and bug URL (OOPS!) Should be removed. > Source/WebCore/ChangeLog:8 > + Bug 15799: textPath element does not re-render when referenced path changes > + https://bugs.webkit.org/show_bug.cgi?id=15799 Swap blocks. > Source/WebCore/svg/SVGElementRareData.h:24 > +#include "Element.h" > +#include "EventNames.h" Can we move the implementation of createAndInstallEventListener/removeEventListenerIfNeeded to the .cpp file, to avoid having to include EventNames everywhere (this adds a huge dependency for all SVG*Element files) > Source/WebCore/svg/SVGElementRareData.h:25 > +#include "SubtreeModificationEventListener.h" Isn't a class forward enough here? > Source/WebCore/svg/SVGPathElement.cpp:252 > + if (Node* parent = parentNode()) { > + RefPtr<MutationEvent> mutationEvent = MutationEvent::create(eventNames().DOMSubtreeModifiedEvent, true); > + parent->handleLocalEvents(mutationEvent.get()); > + } Why does only SVGPathElement need this logic? This will immediately come to my mind when reading this. So you have to explain it with a comment, also it's not quite clear for me yet. > Source/WebCore/svg/SVGStyledElement.cpp:353 > + if (Node* parent = parentNode()) { > + RefPtr<MutationEvent> mutationEvent = MutationEvent::create(eventNames().DOMSubtreeModifiedEvent, true); > + parent->handleLocalEvents(mutationEvent.get()); > + } Why do you need to fire mutation events manually anyway? I guess when the change is triggered from SVG DOM? When it comes from XML DOM via Element::attributeChanged, mutation events should have been dispatched already, no? > Source/WebCore/svg/SVGTRefElement.cpp:95 > + ensureRareSVGData()->removeEventListenerIfNeeded(treeScope()); Only call removeEventListener at all, when hasRareSVGData is true. > Source/WebCore/svg/SVGTRefElement.cpp:186 > + ensureRareSVGData()->removeEventListenerIfNeeded(treeScope()); See comments below, check hasRareSVGData first. > Source/WebCore/svg/SVGTextPathElement.cpp:35 > + One \n too much. > Source/WebCore/svg/SVGTextPathElement.cpp:114 > + ensureRareSVGData()->removeEventListenerIfNeeded(treeScope()); Check hasRareSVGData first, to see whether you need to call a method or not. > Source/WebCore/svg/SVGTextPathElement.cpp:120 > + Element* target = treeScope()->getElementById(id); > + if (!target) > + document()->accessSVGExtensions()->addPendingResource(id, this); > + else > + ensureRareSVGData()->createAndInstallEventListener(this, id, target); That could be made more readable: if (Element* target = treeScope...) ensureRareSVGData()->createAndInstall.. else document()... > Source/WebCore/svg/SVGTextPathElement.cpp:224 > + ensureRareSVGData()->removeEventListenerIfNeeded(treeScope()); Why ensureRareSVGData here? If hasRareSVGData is false, you don't need to do anything. Especially not forcing to create a rare svg data object, if not present.

Created attachment 99652 [details] Patch

Hi Niko, (In reply to comment #28) > (From update of attachment 99588 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=99588&action=review > > Good job! Some comments: In general patch doesn't apply so you'll need to reupload it, so we get EWS results. I noticed, fixed. > > LayoutTests/ChangeLog:6 > > + Reviewed by NOBODY (OOPS!). > > + > > + Bug 15799: textPath element does not re-render when referenced path changes > > + https://bugs.webkit.org/show_bug.cgi?id=15799 > > According to a recent thread on webkit-dev, these blocks need to be swapped. Bug block comes first. Fixed. > > Source/WebCore/ChangeLog:5 > > + Need a short description and bug URL (OOPS!) > > Should be removed. Fixed. > > Source/WebCore/ChangeLog:8 > > + Bug 15799: textPath element does not re-render when referenced path changes > > + https://bugs.webkit.org/show_bug.cgi?id=15799 > > Swap blocks. Fixed. > > Source/WebCore/svg/SVGElementRareData.h:24 > > +#include "Element.h" > > +#include "EventNames.h" > > Can we move the implementation of createAndInstallEventListener/removeEventListenerIfNeeded to the .cpp file, to avoid having to include EventNames everywhere (this adds a huge dependency for all SVG*Element files) I tried to clean this up as much as possible. > > Source/WebCore/svg/SVGElementRareData.h:25 > > +#include "SubtreeModificationEventListener.h" > > Isn't a class forward enough here? No, because of the RefPtr usage. > > Source/WebCore/svg/SVGPathElement.cpp:252 > > + if (Node* parent = parentNode()) { > > + RefPtr<MutationEvent> mutationEvent = MutationEvent::create(eventNames().DOMSubtreeModifiedEvent, true); > > + parent->handleLocalEvents(mutationEvent.get()); > > + } > > Why does only SVGPathElement need this logic? This will immediately come to my mind when reading this. So you have to explain it with a comment, also it's not quite clear for me yet. I added a comment. Basically no subtree modified event is sent for attr change when there already was an attr value. > > Source/WebCore/svg/SVGStyledElement.cpp:353 > > + if (Node* parent = parentNode()) { > > + RefPtr<MutationEvent> mutationEvent = MutationEvent::create(eventNames().DOMSubtreeModifiedEvent, true); > > + parent->handleLocalEvents(mutationEvent.get()); > > + } > > Why do you need to fire mutation events manually anyway? I guess when the change is triggered from SVG DOM? > When it comes from XML DOM via Element::attributeChanged, mutation events should have been dispatched already, no? See above. > > Source/WebCore/svg/SVGTRefElement.cpp:95 > > + ensureRareSVGData()->removeEventListenerIfNeeded(treeScope()); > > Only call removeEventListener at all, when hasRareSVGData is true. Fixed. > > Source/WebCore/svg/SVGTRefElement.cpp:186 > > + ensureRareSVGData()->removeEventListenerIfNeeded(treeScope()); > > See comments below, check hasRareSVGData first. Fixed. > > Source/WebCore/svg/SVGTextPathElement.cpp:35 > > + > > One \n too much. Fixed. > > Source/WebCore/svg/SVGTextPathElement.cpp:114 > > + ensureRareSVGData()->removeEventListenerIfNeeded(treeScope()); > > Check hasRareSVGData first, to see whether you need to call a method or not. Fixed. > > Source/WebCore/svg/SVGTextPathElement.cpp:120 > > + Element* target = treeScope()->getElementById(id); > > + if (!target) > > + document()->accessSVGExtensions()->addPendingResource(id, this); > > + else > > + ensureRareSVGData()->createAndInstallEventListener(this, id, target); > > That could be made more readable: > if (Element* target = treeScope...) > ensureRareSVGData()->createAndInstall.. > else > document()... True, fixed. > > Source/WebCore/svg/SVGTextPathElement.cpp:224 > > + ensureRareSVGData()->removeEventListenerIfNeeded(treeScope()); > > Why ensureRareSVGData here? If hasRareSVGData is false, you don't need to do anything. Especially not forcing to create a rare svg data object, if not present. Fixed! Cheers, Rob.

Comment on attachment 99652 [details] Patch Attachment 99652 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8983399

Comment on attachment 99652 [details] Patch Attachment 99652 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8979900

Created attachment 99655 [details] Patch

Comment on attachment 99655 [details] Patch Attachment 99655 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8983408

Comment on attachment 99655 [details] Patch Attachment 99655 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/8984211

Comment on attachment 99655 [details] Patch Attachment 99655 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8987088

Created attachment 99656 [details] Patch

Comment on attachment 99656 [details] Patch Attachment 99656 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/8987094

Created attachment 99657 [details] Patch

Comment on attachment 99657 [details] Patch Attachment 99657 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8986245 New failing tests: svg/custom/textPath-path-change-pattern.svg svg/custom/textPath-startoffset-pattern.svg svg/custom/textPath-insert-path.svg svg/custom/textPath-change-id2-pattern.svg svg/custom/textPath-path-change2-pattern.svg svg/custom/textPath-change-id-pattern.svg svg/custom/textPath-remove-path.svg svg/custom/textPath-path-change2.svg svg/custom/textPath-remove-path-pattern.svg svg/custom/textPath-insert-path-pattern.svg svg/custom/textPath-change-reference-pattern.svg svg/custom/textPath-change-id2.svg svg/custom/textPath-change-reference.svg svg/custom/textPath-startoffset.svg svg/custom/textPath-change-reference2.svg svg/custom/textPath-change-reference2-pattern.svg svg/custom/textPath-change-id.svg svg/custom/textPath-path-change.svg

Created attachment 99665 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 99657 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99657&action=review Not meant as a real review but r- because of the following: > LayoutTests/ChangeLog:6 > + Bug 15799: textPath element does not re-render when referenced path changes > + https://bugs.webkit.org/show_bug.cgi?id=15799 Is it not possible to move some of the tests to dumpText tests (if not all)? Looks like all tests just draw text. > Source/WebCore/ChangeLog:9 > + Support textPath updating to changes on the referenced path. To make this possible > + refactor the event listener logic from SVGTRefElement into SubtreeModificationEventListener. You added new files, moved code, changed functionality of certain things... You should describe all that in detail. You don't have to do that in the description body. You can comment the changes line by line below. Missing descriptions make it harder to get the context and review your patch.

Comment on attachment 99657 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99657&action=review Next rounds of comments! Poor Rob :-) >> LayoutTests/ChangeLog:6 >> + Reviewed by NOBODY (OOPS!). >> + >> + Bug 15799: textPath element does not re-render when referenced path changes >> + https://bugs.webkit.org/show_bug.cgi?id=15799 > > Is it not possible to move some of the tests to dumpText tests (if not all)? Looks like all tests just draw text. Swap these blocks, the ChangeLog style has been changed. > LayoutTests/ChangeLog:19 > + These tests are duplicated for <pattern>. You mean, these tests are duplicated for <textPath> inside a <pattern>, right? > LayoutTests/platform/mac/svg/custom/textPath-change-reference-pattern-expected.txt:4 > + RenderSVGRoot {svg} at (0,0) size 201x201 This area is a bit too small in width. > LayoutTests/svg/custom/textPath-change-id-pattern.svg:1 > +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" onload="setTimeout(runTest, 0)"> Dirk had concerns these tests should be dumpAsText style. But I think we really want to test repainting here, especially stuff like change-id-of-textPath-within-a-pattern is hard to test without pixel tests, if not impossible. I rather worry about the tests themselves, it's very hard to read them, you don't immediately know what's the expected result. (in contrary to cases like a 100x100 green rect is the correct rendering result - easy to see that). While it's again some work to put into these tests, I think we'd benefit a lot of you'd do it as follows: <!-- Testcase --> <g transform="translate(0, 0)"> AllYourStuffGoesHere </g> <line.. vertical line that seperates the left (actual results) and right (expected results) side... <!-- Expected rendering --> <g transform="translate(200, 0)"> <text><textPath ... construct valid testcase here, that doesn't change or modify anything. </g> If there's any doubt how the test should look like I either: a) add a comment in the testcase to say how it should look like, or if that is too complex... b) do it as I explained above. What do you think? Do you agree its worth to do that, or am I crazy? :-) > LayoutTests/svg/custom/textPath-change-id.svg:1 > +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" onload="setTimeout(runTest, 0)"> Compare this test vs. change-id2.svg -- it's hard to tell why LEFT/RIGHT is correct here and LEFT correct in the other test as result, w/o inspecting closely what's happening (change to valid vs. change to invalid). > LayoutTests/svg/custom/textPath-change-reference-pattern.svg:15 > + tp.setAttributeNS("http://www.w3.org/1999/xlink","href","#thePath"); You want to duplicate this testcase and check the same using tp.href.baseVal = "#... > LayoutTests/svg/custom/textPath-change-reference2-pattern.svg:17 > + tp.setAttributeNS("http://www.w3.org/1999/xlink","href","#wrongPath"); Ditto. > LayoutTests/svg/custom/textPath-change-reference2.svg:14 > + tp.setAttributeNS("http://www.w3.org/1999/xlink","href","#wrongPath"); Ditto. > LayoutTests/svg/custom/textPath-modify-child.svg:1 > +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" onload="runTest()"> Are you omiting the waitUntilDone here on purpose? Why is this one different? (onload runTest vs. onload setTimeout(runTest... >> Source/WebCore/ChangeLog:9 >> + refactor the event listener logic from SVGTRefElement into SubtreeModificationEventListener. > > You added new files, moved code, changed functionality of certain things... You should describe all that in detail. You don't have to do that in the description body. You can comment the changes line by line below. Missing descriptions make it harder to get the context and review your patch. Dirk is right, I always ask Dirk to do that as well :-) Otherwise we'll hear complaints that we work too private! > Source/WebCore/ChangeLog:32 > + * CMakeLists.txt: I'll start: ": Add new files to build." > Source/WebCore/ChangeLog:33 > + * GNUmakefile.list.am: : Ditto. > Source/WebCore/ChangeLog:40 > + (WebCore::SVGElementRareData::createAndInstallEventListener): Add helper function shared by .... etc.. > Source/WebCore/WebCore.vcproj/WebCore.vcproj:65341 > + RelativePath="..\svg\SubtreeModificationEventListener.h" You should add the cpp files as well here, not just in SVGAllInOne.cpp, but exclude them from the build. There are examples of other files that do it this way. > Source/WebCore/svg/SVGElementRareData.h:24 > +#include "SubtreeModificationEventListener.h" > +#include <wtf/HashMap.h> Good! > Source/WebCore/svg/SVGElementRareData.h:77 > + void createAndInstallEventListener(SubtreeModificationEventListenerClient*, const WTF::String&, Element*); You can omit WTF::. > Source/WebCore/svg/SVGPathElement.cpp:251 > + // Since setting an already set attribute fires no MutationEvent, we have to fire it ourselves > + // to notify listeners. > + if (Node* parent = parentNode()) { #1. I think this is really a performance killer -- you're sending mutation events all the time, regardless of whether any listener is actually interessted in these events? SVGPathElement::svgAttributeChanged() is a hot function! Check Node::dispatchSubtreeModifiedEvent() - can you reuse this? (important bit of it: if (!document()->hasListenerType(Document::DOMSUBTREEMODIFIED_LISTENER)) return; ) #2: What test breaks if you disable this? I'm really worried about SVG special mutation event firing. I think this is not the right place to do that. To be able to really judge what's going on, I'd need to know the exact cases where it breaks (fooAttr already set to a non-non value, setting to another non-null value?) I guess the testcases that are affected by this are the ones where you are changing an existing d attribute? We definitely need another test case where you modify the SVGPathSegList directly via SVG DOM, and not using setAttribute('d', '...'). Any SVG DOM change of a SVGPathSegList object will trigger svgAttributeChanged(SVGNames::xAttr ... calls internally, so I think these testcases will also work with your approach - but we should test it works. #3: Aren't you firing this twice now, _if_ the attribute is set initially? Argl, this is really complex, I knew why I hold off working on this ;-) > Source/WebCore/svg/SVGStyledElement.cpp:355 > buildPendingResourcesIfNeeded(); > + // 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()); > + } Ditto.

Created attachment 99752 [details] Patch

Hi Niko, (In reply to comment #43) > (From update of attachment 99657 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=99657&action=review > > Next rounds of comments! Poor Rob :-) > > >> LayoutTests/ChangeLog:6 > >> + Reviewed by NOBODY (OOPS!). > >> + > >> + Bug 15799: textPath element does not re-render when referenced path changes > >> + https://bugs.webkit.org/show_bug.cgi?id=15799 > > > > Is it not possible to move some of the tests to dumpText tests (if not all)? Looks like all tests just draw text. > > Swap these blocks, the ChangeLog style has been changed. Fixed. > > LayoutTests/ChangeLog:19 > > + These tests are duplicated for <pattern>. > > You mean, these tests are duplicated for <textPath> inside a <pattern>, right? Right, fixed. > > LayoutTests/platform/mac/svg/custom/textPath-change-reference-pattern-expected.txt:4 > > + RenderSVGRoot {svg} at (0,0) size 201x201 > > This area is a bit too small in width. I don't get this one? Too small for what? > > LayoutTests/svg/custom/textPath-change-id-pattern.svg:1 > > +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" onload="setTimeout(runTest, 0)"> > > Dirk had concerns these tests should be dumpAsText style. But I think we really want to test repainting here, especially stuff like change-id-of-textPath-within-a-pattern is hard to test without pixel tests, if not impossible. > I rather worry about the tests themselves, it's very hard to read them, you don't immediately know what's the expected result. (in contrary to cases like a 100x100 green rect is the correct rendering result - easy to see that). > > While it's again some work to put into these tests, I think we'd benefit a lot of you'd do it as follows: > > <!-- Testcase --> > <g transform="translate(0, 0)"> > AllYourStuffGoesHere > </g> > > <line.. vertical line that seperates the left (actual results) and right (expected results) side... > > <!-- Expected rendering --> > <g transform="translate(200, 0)"> > <text><textPath ... construct valid testcase here, that doesn't change or modify anything. > </g> > > If there's any doubt how the test should look like I either: > a) add a comment in the testcase to say how it should look like, or if that is too complex... > b) do it as I explained above. > > What do you think? Do you agree its worth to do that, or am I crazy? :-) I removed the LEFT/RIGHT stuff and just used PASS a lot more now. > > LayoutTests/svg/custom/textPath-change-id.svg:1 > > +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" onload="setTimeout(runTest, 0)"> > > Compare this test vs. change-id2.svg -- it's hard to tell why LEFT/RIGHT is correct here and LEFT correct in the other test as result, w/o inspecting closely what's happening (change to valid vs. change to invalid). Fixed. > > LayoutTests/svg/custom/textPath-change-reference-pattern.svg:15 > > + tp.setAttributeNS("http://www.w3.org/1999/xlink","href","#thePath"); > > You want to duplicate this testcase and check the same using tp.href.baseVal = "#... Fixed. > > LayoutTests/svg/custom/textPath-change-reference2-pattern.svg:17 > > + tp.setAttributeNS("http://www.w3.org/1999/xlink","href","#wrongPath"); > > Ditto. Fixed. > > LayoutTests/svg/custom/textPath-change-reference2.svg:14 > > + tp.setAttributeNS("http://www.w3.org/1999/xlink","href","#wrongPath"); > > Ditto. Fixed. > > LayoutTests/svg/custom/textPath-modify-child.svg:1 > > +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" onload="runTest()"> > > Are you omiting the waitUntilDone here on purpose? Why is this one different? (onload runTest vs. onload setTimeout(runTest... It is not needed here. I removed setTimeout since it is not needed either :) > >> Source/WebCore/ChangeLog:9 > >> + refactor the event listener logic from SVGTRefElement into SubtreeModificationEventListener. > > > > You added new files, moved code, changed functionality of certain things... You should describe all that in detail. You don't have to do that in the description body. You can comment the changes line by line below. Missing descriptions make it harder to get the context and review your patch. > > Dirk is right, I always ask Dirk to do that as well :-) Otherwise we'll hear complaints that we work too private! Understood. > > Source/WebCore/ChangeLog:32 > > + * CMakeLists.txt: > > I'll start: ": Add new files to build." > > > Source/WebCore/ChangeLog:33 > > + * GNUmakefile.list.am: > > : Ditto. > > > Source/WebCore/ChangeLog:40 > > + (WebCore::SVGElementRareData::createAndInstallEventListener): > > Add helper function shared by .... > etc.. I tried to add a lot more of comments... > > Source/WebCore/WebCore.vcproj/WebCore.vcproj:65341 > > + RelativePath="..\svg\SubtreeModificationEventListener.h" > > You should add the cpp files as well here, not just in SVGAllInOne.cpp, but exclude them from the build. > There are examples of other files that do it this way. Fixed. > > Source/WebCore/svg/SVGElementRareData.h:24 > > +#include "SubtreeModificationEventListener.h" > > +#include <wtf/HashMap.h> > > Good! > > > Source/WebCore/svg/SVGElementRareData.h:77 > > + void createAndInstallEventListener(SubtreeModificationEventListenerClient*, const WTF::String&, Element*); > > You can omit WTF::. Fixed. > > Source/WebCore/svg/SVGPathElement.cpp:251 > > + // Since setting an already set attribute fires no MutationEvent, we have to fire it ourselves > > + // to notify listeners. > > + if (Node* parent = parentNode()) { > > #1. > I think this is really a performance killer -- you're sending mutation events all the time, regardless of whether any listener is actually interessted in these events? SVGPathElement::svgAttributeChanged() is a hot function! > Check Node::dispatchSubtreeModifiedEvent() - can you reuse this? > (important bit of it: > if (!document()->hasListenerType(Document::DOMSUBTREEMODIFIED_LISTENER)) > return; > ) I incorporated the check. > #2: What test breaks if you disable this? I'm really worried about SVG special mutation event firing. I think this is not the right place to do that. > To be able to really judge what's going on, I'd need to know the exact cases where it breaks (fooAttr already set to a non-non value, setting to another non-null value?) > I guess the testcases that are affected by this are the ones where you are changing an existing d attribute? Yep, anything changing d attribute. It is unfortunate but I think we can work around it like this. > We definitely need another test case where you modify the SVGPathSegList directly via SVG DOM, and not using setAttribute('d', '...'). > > Any SVG DOM change of a SVGPathSegList object will trigger svgAttributeChanged(SVGNames::xAttr ... calls internally, so I think these testcases will also work with your approach - but we should test it works. Fixed. > #3: Aren't you firing this twice now, _if_ the attribute is set initially? I don't think so since at that time nobody is listening :) > Argl, this is really complex, I knew why I hold off working on this ;-) > > > Source/WebCore/svg/SVGStyledElement.cpp:355 > > buildPendingResourcesIfNeeded(); > > + // 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()); > > + } > > Ditto. Fixed like for SVGPathElement.cpp Cheers, Rob.

Comment on attachment 99752 [details] Patch Attachment 99752 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8983839 New failing tests: svg/custom/textPath-path-change-pattern.svg svg/custom/textPath-path-change-using-svg-dom-pattern.svg svg/custom/textPath-change-id2-pattern.svg svg/custom/textPath-change-reference2-using-baseval.svg svg/custom/textPath-change-id-pattern.svg svg/custom/textPath-path-change2.svg svg/custom/textPath-change-id2.svg svg/custom/textPath-change-id.svg svg/custom/textPath-change-reference-pattern.svg svg/custom/textPath-change-reference-using-baseval.svg svg/custom/textPath-change-reference.svg svg/custom/textPath-change-reference-using-baseval-pattern.svg svg/custom/textPath-change-reference2-using-baseval-pattern.svg svg/custom/textPath-path-change2-pattern.svg svg/custom/textPath-change-reference2-pattern.svg svg/custom/textPath-insert-path-pattern.svg svg/custom/textPath-path-change-using-svg-dom.svg svg/custom/textPath-change-reference2.svg

Created attachment 99772 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

(In reply to comment #45) > I don't get this one? Too small for what? The text in the expected.png was very close to the right end of the document. Not very important. > I removed the LEFT/RIGHT stuff and just used PASS a lot more now. Ok, will check. > > #2: What test breaks if you disable this? I'm really worried about SVG special mutation event firing. I think this is not the right place to do that. > > To be able to really judge what's going on, I'd need to know the exact cases where it breaks (fooAttr already set to a non-non value, setting to another non-null value?) > > > I guess the testcases that are affected by this are the ones where you are changing an existing d attribute? > > Yep, anything changing d attribute. It is unfortunate but I think we can work around it like this. Ok. > > #3: Aren't you firing this twice now, _if_ the attribute is set initially? > > I don't think so since at that time nobody is listening :) How about a DOM defined event listener? <textPath onattrmodified=".." xlink:href=... > > > > > Source/WebCore/svg/SVGStyledElement.cpp:355 > > > buildPendingResourcesIfNeeded(); > > > + // 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()); > > > + } > > > > Ditto. > > Fixed like for SVGPathElement.cpp I'll have a look.

Comment on attachment 99752 [details] Patch 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.

Created attachment 99882 [details] Patch

Hi Niko, >(In reply to comment #49) > (From update of attachment 99752 [details]) > 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? Yes, unfortunately it does not work :( Some test really need to test the positioning (startOffset, path d change). And for something like textPath-change-id2.svg the framework does not seem clever enough, the text dump doesn't get the id change, it still says FAIL for the textPath that has an invalid id. > > 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. Good point. I changed the test locally to: <svg xmlns="http://www.w3.org/2000/svg" onload="runTest()"> <script> document.documentElement.addEventListener('DOMSubtreeModified', onSubtreeModified, false); function onSubtreeModified(event) { alert('mutation!'); } </script> <path id='p' d="M 0 0"/> </svg> So now I instead test whether rareSVGData is set in the first place. This fixes it (I added printf's to test), let me know if an extra test is needed. > > 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. Fixed like above. Cheers, Rob.

(In reply to comment #51) > > 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? > > Yes, unfortunately it does not work :( Some test really need to test the positioning (startOffset, path d change). And for something like textPath-change-id2.svg the framework does not seem clever enough, the text dump doesn't get the id change, it still says FAIL for the textPath that has an invalid id. I don't think I understand. There should be no difference in any way, when you use "layoutTestController.dumpAsText(true)" - it will still generate pixel test results, that should do all the invalidations/repainting you eventually rely on. Just no render tree dump. If that affects a test, there's a bug! > Good point. I changed the test locally to: > > <svg xmlns="http://www.w3.org/2000/svg" onload="runTest()"> > <script> > document.documentElement.addEventListener('DOMSubtreeModified', onSubtreeModified, false); > function onSubtreeModified(event) { alert('mutation!'); } > </script> > <path id='p' d="M 0 0"/> > </svg> > > So now I instead test whether rareSVGData is set in the first place. This fixes it (I added printf's to test), let me know if an extra test is needed. Hrm, I don't think that's enough. You can trigger the creation of rareSVGData in other ways. eg. <path id='p' cursor='...' d="..."> When you reference an existing <cursor> element through the CSS cursor property, a SVGElementRareData object is constructed -- you'd then fire twice again. I don't think relying on SVGRareDatas presence is enough :( How about extending svgAttributeChanged by: enum AttributeChangeOrigin { XMLDOMOrigin, SVGDOMOrigin }; The call to svgAttributeChanged from SVGElement::attributeChanged, will pass XMLDOMOrigin, while the other users of svgAttributeChanged (just grep for it in svg/) will pass SVGDOMOrigin. This way you can be sure if XMLDOMOrigin is true, that you don't need to fire mutation events. > > > > 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. Ditto :-) I hope the XMLDOMOrigin/SVGDOMOrigin information can be used to fully fix the problem.

Comment on attachment 99882 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99882&action=review Setting r- because of the previous comments. > Source/WebCore/svg/SVGElementRareData.cpp:57 > + RefPtr<MutationEvent> mutationEvent = MutationEvent::create(eventNames().DOMSubtreeModifiedEvent, true); > + parent->handleLocalEvents(mutationEvent.get()); Why can't you just use dispatchSubtreeModifiedEvent here? > Source/WebCore/svg/SVGPathElement.cpp:250 > + rareSVGData()->fireEventIfNeeded(this); fireMutationEventIfNeeded ?

Created attachment 100043 [details] Patch

Hi Niko, (In reply to comment #53) > (From update of attachment 99882 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=99882&action=review > > Setting r- because of the previous comments. > > > Source/WebCore/svg/SVGElementRareData.cpp:57 > > + RefPtr<MutationEvent> mutationEvent = MutationEvent::create(eventNames().DOMSubtreeModifiedEvent, true); > > + parent->handleLocalEvents(mutationEvent.get()); > > Why can't you just use dispatchSubtreeModifiedEvent here? Right, I thought it did bubbling/capuring but it looks like it doesn't correct? If so then it does the same so I use it now. Fixed. > > Source/WebCore/svg/SVGPathElement.cpp:250 > > + rareSVGData()->fireEventIfNeeded(this); > > fireMutationEventIfNeeded ? Better, fixed. Cheers, Rob.

Created attachment 100047 [details] Patch

Hi Niko, (In reply to comment #52) > (In reply to comment #51) > > > 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? > > > > Yes, unfortunately it does not work :( Some test really need to test the positioning (startOffset, path d change). And for something like textPath-change-id2.svg the framework does not seem clever enough, the text dump doesn't get the id change, it still says FAIL for the textPath that has an invalid id. > I don't think I understand. There should be no difference in any way, when you use "layoutTestController.dumpAsText(true)" - it will still generate pixel test results, that should do all the invalidations/repainting you eventually rely on. Just no render tree dump. > If that affects a test, there's a bug! Oops, turns out I was doing dumpAsText(), not dumpAsText(true) all the time! Fixed. > > Good point. I changed the test locally to: > > > > <svg xmlns="http://www.w3.org/2000/svg" onload="runTest()"> > > <script> > > document.documentElement.addEventListener('DOMSubtreeModified', onSubtreeModified, false); > > function onSubtreeModified(event) { alert('mutation!'); } > > </script> > > <path id='p' d="M 0 0"/> > > </svg> > > > > So now I instead test whether rareSVGData is set in the first place. This fixes it (I added printf's to test), let me know if an extra test is needed. > Hrm, I don't think that's enough. You can trigger the creation of rareSVGData in other ways. > eg. <path id='p' cursor='...' d="..."> > When you reference an existing <cursor> element through the CSS cursor property, a SVGElementRareData object is constructed -- you'd then fire twice again. > > I don't think relying on SVGRareDatas presence is enough :( > How about extending svgAttributeChanged by: > enum AttributeChangeOrigin { > XMLDOMOrigin, > SVGDOMOrigin > }; > > The call to svgAttributeChanged from SVGElement::attributeChanged, will pass XMLDOMOrigin, while the other users of svgAttributeChanged (just grep for it in svg/) will pass SVGDOMOrigin. > This way you can be sure if XMLDOMOrigin is true, that you don't need to fire mutation events. I dont see why we need to differentiate at all, both deserve firing events AFAICS? I fixed it now by first checking whether there is an event listener waiting for the event at all. > > > > 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. > Ditto :-) > > I hope the XMLDOMOrigin/SVGDOMOrigin information can be used to fully fix the problem. See above. Cheers, Rob.

(In reply to comment #57) > Hi Niko, > Oops, turns out I was doing dumpAsText(), not dumpAsText(true) all the time! Fixed. Great! > I dont see why we need to differentiate at all, both deserve firing events AFAICS? I fixed it now by first checking whether there is an event listener waiting for the event at all. Well if the origin of the svgAttributeChanged call is XML DOM (aka. called via SVGElement::attributeChanged) then you already called the mutation event listener! That means while constructing the element during parsing the onsubtreemodified listener defined in markup would be called twice, instead of once. Lemme rephrase: rect.x.baseVal.value = 150; --> will fire the subtreemodified listener once with your patch, and never without your patch (so your patch can be seen as a progression here) rect.setAttribute("x", "150") --> used to fire subtreemodified listener once without your patch and twice now with your patch Is that right? (I didn't check your last patch yet, will do now, maybe it resolves my concerns).

Comment on attachment 100047 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100047&action=review Code looks good, won't set r-/r+ yet, as I have to check with the DOM Level 3 Events spec whether you're allowed to fire the mutation event if an already set attr value cahnges. > Source/WebCore/svg/SVGElement.cpp:407 > +{ > + // Since setting an already set attribute fires no MutationEvent, we have to fire it ourselves > + // to notify listeners. > + if (hasRareSVGData() && rareSVGData()->hasEventListener()) > + dispatchSubtreeModifiedEvent(); Thinking about it, this indicates that: a) the generic XML DOM setAttribute call is buggy, it's supposed to fire this listener but doesn't if the attribute is already set b) the generic XML DOM setAttribute is right, and you're now firing a listener, that can be received from JS in any webpage, where we shouldn't! I'll check DOM 3 Events to get this resolved. > Source/WebCore/svg/SVGElementRareData.h:79 > + bool hasEventListener() const { return m_eventListener; } Aha you're now tracking and deciding whether to call this listener only if we have one, that's definately better, but doesn't fully fix the problem, if it turns out we're not supposed to fire mutation events that can be captured via JS, if the attribute was already set.

Sme background from DOM Level 3 Events, 5.2.8 Mutation events: <spec> Many single modifications of the tree can cause multiple mutation events to be dispatched. Rather than attempt to specify the ordering of mutation events due to every possible modification of the tree, the ordering of these events is left to the implementation. </spec> Most interessting is: http://www.w3.org/TR/DOM-Level-3-Events/#event-type-DOMSubtreeModified <spec> This is a general event for notification of all changes to the document. It can be used instead of the more specific mutation and mutation name events. It may be dispatched after a single modification to the document or, at the implementation's discretion, after multiple changes have occurred. The latter case should generally be used to accommodate multiple changes which occur either simultaneously or in rapid succession. The target of this event must be the lowest common parent of the changes which have taken place. This event must be dispatched after any other events caused by the mutation(s) have occurred. </spec> In other words: we can do whatever we want :-) We should be sure that rect.x.baseVal.value = 150; and rect.setAttribute("x", "150") deliver the same number of mutation events. Using the testcase which installs a subtreemodified listener at document with a <script> you should check that the number of mutation events are the same for rect.x.baseVal.value / rect.setAttribute(x, .. in following cases: #1) A simple <rect> which has no SVGElementRareData object after it has been parsed. 1.1: Check mutation event is fired by inline script, via XML DOM: <rect width="50" height="100"/> <script>document.getElementsByTagName("rect")[0].setAttribute('width', '100');.... 1.2: Check mutation event is fired by inline script, via SVG DOM: <rect width="50" height="100"/> <script>document.getElementsByTagName("rect")[0].width.baseVal.value = 100; ... #2) A <rect> which has a SVGElementRareData object after it has been parsed (eg. by setting the cursor attribute, pointing to a valid <cursor element> 2.1: Check mutation event is fired by inline script, via XML DOM: <rect cursor="#someCursor" width="50" height="100"/> <script>document.getElementsByTagName("rect")[0].setAttribute('width', '100');.... 2.2: Check mutation event is fired by inline script, via SVG DOM: <rect cursor="#someCursor" width="50" height="100"/> <script>document.getElementsByTagName("rect")[0].width.baseVal.value = 100; ... Then duplicate the four tests from #1) and #2) to run the scripts async, after parsing finished via <svg onload="setTimeout(runTest, 0)">. I'd include a simple counter in the mutationEvent listener, so you can check the number of mutation event calls are the same in all those cases (should be possible to fold all these subtests into 1 single test). If it turns out all these numbers are equal with your patch, I'm happy to set r+ - though you might want to include this new testcase with a new version of your patch :-)

Hi Niko, (In reply to comment #60) > Sme background from DOM Level 3 Events, 5.2.8 Mutation events: > <spec> > Many single modifications of the tree can cause multiple mutation events to be dispatched. Rather than attempt to specify the ordering of mutation events due to every possible modification of the tree, the ordering of these events is left to the implementation. > </spec> > > Most interessting is: http://www.w3.org/TR/DOM-Level-3-Events/#event-type-DOMSubtreeModified > <spec> > This is a general event for notification of all changes to the document. It can be used instead of the more specific mutation and mutation name events. It may be dispatched after a single modification to the document or, at the implementation's discretion, after multiple changes have occurred. The latter case should generally be used to accommodate multiple changes which occur either simultaneously or in rapid succession. The target of this event must be the lowest common parent of the changes which have taken place. This event must be dispatched after any other events caused by the mutation(s) have occurred. > </spec> > > In other words: we can do whatever we want :-) > We should be sure that rect.x.baseVal.value = 150; and rect.setAttribute("x", "150") deliver the same number of mutation events. Using the testcase which installs a subtreemodified listener at document with a <script> you should check that the number of mutation events are the same for rect.x.baseVal.value / rect.setAttribute(x, .. in following cases: > > #1) A simple <rect> which has no SVGElementRareData object after it has been parsed. > 1.1: Check mutation event is fired by inline script, via XML DOM: > <rect width="50" height="100"/> > <script>document.getElementsByTagName("rect")[0].setAttribute('width', '100');.... > > 1.2: Check mutation event is fired by inline script, via SVG DOM: > <rect width="50" height="100"/> > <script>document.getElementsByTagName("rect")[0].width.baseVal.value = 100; ... > > #2) A <rect> which has a SVGElementRareData object after it has been parsed > (eg. by setting the cursor attribute, pointing to a valid <cursor element> > > 2.1: Check mutation event is fired by inline script, via XML DOM: > <rect cursor="#someCursor" width="50" height="100"/> > <script>document.getElementsByTagName("rect")[0].setAttribute('width', '100');.... > > 2.2: Check mutation event is fired by inline script, via SVG DOM: > <rect cursor="#someCursor" width="50" height="100"/> > <script>document.getElementsByTagName("rect")[0].width.baseVal.value = 100; ... > > Then duplicate the four tests from #1) and #2) to run the scripts async, after parsing finished via <svg onload="setTimeout(runTest, 0)">. > > I'd include a simple counter in the mutationEvent listener, so you can check the number of mutation event calls are the same in all those cases (should be possible to fold all these subtests into 1 single test). > > If it turns out all these numbers are equal with your patch, I'm happy to set r+ - though you might want to include this new testcase with a new version of your patch :-) I tried to add such a testcase, but things are maybe a bit different then you think ; my patch does not send out mutation event when setting the same attribute again on a <rect>. All I did was send them out when either setting id on any styled SVG element or setting d attribute on <path> ; up till now these are the only ones we are interested in, I did this work for <tref> and <textPath> referencing cases. So let me know how to proceed, I want to get this out of the way soon. Cheers, Rob.

Comment on attachment 100047 [details] Patch current trunk behavior: WildFox: for <rect id="foo"> - if I change the id dynamically, you'll not get a mutation event at all ever, if the value was already specified in markup WildFox: if id="foo" was omitted from the markup, I'd only get a mutation event the first time "id" is touched ... with Robs patch: [17:37] WildFox: rbuis: ok, very easy to construct the testcase now, where it fails [17:37] WildFox: rbuis: imagine <defs> <path d="..." /> </defs> (note, the <path> has no id) [17:38] WildFox: rbuis: <text> <textPath xlink:href="#foo" [17:38] WildFox: now with a <script> call path.setAttribute('id', 'foo'); [17:38] WildFox: NamedNodeMap::addAttribute -> Node::dispatchSubtreeModifiedEvent, would fire the listener the first time [17:38] WildFox: then SVGStyledElement::svgAttributeChanged -> SVGElement::fireMutationEventIfNeeded, would fire the listener the second time ... We nailed down the remaining problem, Rob will come up with a fix! :-)

Rob, can you have a look at that patch again - we both forgot about it :(

Created attachment 143034 [details] Patch

Hi Niko, (In reply to comment #63) > Rob, can you have a look at that patch again - we both forgot about it :( Yes, it is on my TODO list again :) As a first step I made it compile again.

Attachment 143034 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Source/WebCore/svg/SVGTextPathElement.cpp:117: Missing space before ( in if( [whitespace/parens] [5] Total errors found: 1 in 73 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 143034 [details] Patch Attachment 143034 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12728960

Comment on attachment 143034 [details] Patch Attachment 143034 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12742222

Created attachment 143158 [details] Patch

Created attachment 143314 [details] Patch

Comment on attachment 143314 [details] Patch Attachment 143314 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12767016 New failing tests: svg/text/tref-event-listener-crash.svg svg/custom/textPath-change-reference2-using-baseval.svg svg/custom/textPath-path-change-using-svg-dom.svg fast/loader/loadInProgress.html svg/custom/textPath-insert-path-pattern.svg svg/custom/textPath-change-reference-pattern.svg svg/custom/textPath-change-reference.svg http/tests/xmlhttprequest/zero-length-response.html svg/custom/textPath-change-reference2.svg svg/custom/textPath-path-change-using-svg-dom-pattern.svg svg/custom/textPath-insert-path.svg svg/custom/textPath-change-id-pattern.svg svg/custom/textPath-change-id.svg http/tests/security/script-crossorigin-loads-correctly.html svg/custom/textPath-change-reference-using-baseval.svg svg/custom/textPath-path-change.svg svg/custom/textPath-change-reference-using-baseval-pattern.svg svg/custom/textPath-change-reference2-pattern.svg svg/custom/textPath-path-change-pattern.svg svg/custom/textPath-change-id2-pattern.svg svg/custom/textPath-path-change2-pattern.svg svg/custom/textPath-change-reference2-using-baseval-pattern.svg fast/loader/unload-form-post-about-blank.html svg/custom/textPath-path-change2.svg svg/custom/textPath-change-id2.svg

Created attachment 143363 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 143436 [details] Patch

Comment on attachment 143436 [details] Patch Attachment 143436 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12775086 New failing tests: svg/text/tref-event-listener-crash.svg svg/custom/textPath-path-change-pattern.svg svg/custom/textPath-path-change-using-svg-dom-pattern.svg svg/custom/textPath-insert-path.svg svg/custom/textPath-startoffset-pattern.svg svg/custom/textPath-path-change2-pattern.svg svg/custom/textPath-path-change-using-svg-dom.svg svg/custom/textPath-remove-path.svg svg/custom/textPath-path-change2.svg svg/custom/textPath-remove-path-pattern.svg svg/custom/textPath-insert-path-pattern.svg svg/custom/textPath-startoffset.svg svg/custom/textPath-path-change.svg

Created attachment 143453 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 143436 [details] Patch 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

Heh, I just realized that the code in SVGFEImageElement was born after Robs last patch in 2011 - same for reftests, they didn't exist back then :-)

Hi Niko, Thanks for the review! (In reply to comment #77) > Heh, I just realized that the code in SVGFEImageElement was born after Robs last patch in 2011 - same for reftests, they didn't exist back then :-) Right :) So I started converting them, but there are quite some tests so last night I only got that far. I agree we should convert all to reftests if possible though, I'll continue working on this patch today. Cheers, Rob.

Created attachment 143598 [details] Patch

Hi Niko, (In reply to comment #76) > (From update of attachment 143436 [details]) > 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 :( Yes, likely, I'll have a look. > > LayoutTests/svg/custom/textPath-change-id-pattern.svg:11 > > + layoutTestController.waitUntilDone(); > > Is that needed? Is DRT dumping before onload fires? Nope, copy/paste error, fixed. > > LayoutTests/svg/custom/textPath-change-reference-pattern.svg:15 > > + var path = document.createElementNS("http://www.w3.org/2000/svg", "path"); > > That variable is unused. Right, fixed. > > LayoutTests/svg/custom/textPath-change-reference-using-baseval-pattern.svg:15 > > + var path = document.createElementNS("http://www.w3.org/2000/svg", "path"); > > Ditto. Fixed. > > 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. Yes, copy/paste error, fixed :) > > LayoutTests/svg/custom/textPath-change-reference.svg:12 > > + var path = document.createElementNS("http://www.w3.org/2000/svg", "path"); > > Unused. Fixed. > > LayoutTests/svg/custom/textPath-change-reference2-pattern.svg:16 > > + var path = document.createElementNS("http://www.w3.org/2000/svg", "path"); > > Unused. Fixed. > > LayoutTests/svg/custom/textPath-change-reference2-using-baseval.svg:13 > > + var path = document.createElementNS("http://www.w3.org/2000/svg", "path"); > > Ditto. Fixed. > > LayoutTests/svg/custom/textPath-change-reference2.svg:13 > > + var path = document.createElementNS("http://www.w3.org/2000/svg", "path"); > > Ditto. Fixed. > > 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. Yes, it took me a while, but all should be converted now. A big imporvement :) > > LayoutTests/svg/custom/textPath-set-id.svg:10 > > + var subtreeModifiedEventsSeen = 0; > > + > > Do we currently (without your patch) fire mutation events for id changes? Have to check... > > LayoutTests/svg/custom/textPath-set-id.svg:22 > > + path.setAttribute("id","foo"); > > here Ok, I did not change code yet, just wanted to make sure the tests are fine now, not up for review. Code changes are next... Cheers, Rob.

Created attachment 143856 [details] Patch

Comment on attachment 143856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143856&action=review Impressive how much easier this new concept is! Can you reupload your patch? EWS bots don't apply it. I'll give r+ later, if it builds everywhere, and chromium sees no errors. > Source/WebCore/ChangeLog:11 > + Tests: svg/custom/textPath-change-id-expected.svg The most exhaustive set of tests ever landed: rob++ :-) > Source/WebCore/svg/SVGTextPathElement.cpp:65 > + Newline. > Source/WebCore/svg/SVGTextPathElement.cpp:73 > + ASSERT(document()); > + document()->accessSVGExtensions()->removeAllTargetReferencesForElement(this); What if the SVGTextPathElement got removed from the Document before destruction? Won't this assertion fire?

Created attachment 144062 [details] Patch

Comment on attachment 144062 [details] Patch Attachment 144062 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12795527

Comment on attachment 144062 [details] Patch Attachment 144062 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12799543

Comment on attachment 144062 [details] Patch Attachment 144062 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12808388

Comment on attachment 144062 [details] Patch Attachment 144062 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12799546

Comment on attachment 144062 [details] Patch Attachment 144062 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12794514

Created attachment 144072 [details] Patch

Hi Niko, (In reply to comment #82) > (From update of attachment 143856 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=143856&action=review > > Impressive how much easier this new concept is! Can you reupload your patch? EWS bots don't apply it. > I'll give r+ later, if it builds everywhere, and chromium sees no errors. Re-uploaded a patch that should compile now. > > Source/WebCore/ChangeLog:11 > > + Tests: svg/custom/textPath-change-id-expected.svg > > The most exhaustive set of tests ever landed: rob++ :-) I was amazed too, when I looked again at this old patch :) > > Source/WebCore/svg/SVGTextPathElement.cpp:65 > > + > > Newline. Fixed. > > Source/WebCore/svg/SVGTextPathElement.cpp:73 > > + ASSERT(document()); > > + document()->accessSVGExtensions()->removeAllTargetReferencesForElement(this); > > What if the SVGTextPathElement got removed from the Document before destruction? Won't this assertion fire? Still thinking about this one... I think SVGFEImageElement is the same in this regard. Cheers, Rob.

Comment on attachment 144072 [details] Patch Attachment 144072 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12793511 New failing tests: svg/custom/textPath-modify-child-pattern.svg svg/custom/textPath-modify-child.svg

Created attachment 144081 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 144093 [details] Patch

Hi Niko, (In reply to comment #90) > > > Source/WebCore/svg/SVGTextPathElement.cpp:73 > > > + ASSERT(document()); > > > + document()->accessSVGExtensions()->removeAllTargetReferencesForElement(this); > > > > What if the SVGTextPathElement got removed from the Document before destruction? Won't this assertion fire? > > Still thinking about this one... I think SVGFEImageElement is the same in this regard. Ok I do not think it is a problem. At least, in HTMLInputElement dtor for instance document() is accessed (for radio buttons). The tests should all PASS now. Cheers, Rob.

(In reply to comment #94) > > > > Still thinking about this one... I think SVGFEImageElement is the same in this regard. > > Ok I do not think it is a problem. At least, in HTMLInputElement dtor for instance document() is accessed (for radio buttons). // Returns the document associated with this node. This method never returns NULL, except in the case // of a DocumentType node that is not used with any Document yet. A Document node returns itself. Document* document() const It will never return null, so its fine.

Comment on attachment 144093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144093&action=review r=me! Great work! > Source/WebCore/ChangeLog:9 > + Support textPath updating to changes on the referenced path. To make this possible > + refactor the event listener logic from SVGTRefElement into SubtreeModificationEventListener. Heh the ChangeLog is still wrong. > LayoutTests/svg/custom/textPath-change-id.svg:8 > + layoutTestController.dumpAsText(true); I don't think you need dumpAsText(true) for ref tests - you can omit this line everywhere. ref tests always compare pixels as well.

Committed r118609: <http://trac.webkit.org/changeset/118609>

I seem to have encountered this bug in Safari v16.1 https://stackoverflow.com/questions/75187206/safari-does-not-update-textpath-when-path-data-is-changed-dynamically