Bug 74749

Created attachment 119901 [details] repro (first one was broken)

Created attachment 120073 [details] patch

Also removes a related comment in SVGSwitchElement which to my eye looks completely false.

Comment on attachment 120073 [details] patch Attachment 120073 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10992386 New failing tests: http/tests/inspector/resource-tree/resource-tree-document-url.html

(In reply to comment #4) > (From update of attachment 120073 [details]) > Attachment 120073 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/10992386 > > New failing tests: > http/tests/inspector/resource-tree/resource-tree-document-url.html That seems exceedingly unlikely.

Comment on attachment 120073 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=120073&action=review Great catches, I still have some questions: > LayoutTests/svg/custom/requiredExtensions-runtime-changes.svg:36 > + var removeElements = document.getElementsByClassName("removeRequiredExtensions"); > + var addElements = document.getElementsByClassName("addRequiredExtensions"); Clever idea, I keep that in mind for further tests of mine :-) Makes the test itself much more readable/hackable. > Source/WebCore/ChangeLog:10 > + SVGStringList, because SVGStringList::reset() adds a single empty string to the list. Good catch - but I can't recall why reset() behaves this way. Can we just change the behavior of reset? > Source/WebCore/ChangeLog:13 > + Invalid elements inside <switch> are prevented from creating renderers, > + but are still attached; SVGTests assumes that invalid elements will always be detached. Why is this? I assumed that attached() returns false, if there's no renderer. I just checked the code, and indeed this assumption is just wrong. We're differentiating between two states in dom/Node.cpp (see attach()): * attach() called no renderer created: attached() returns true and renderer() zero. * attach() called renderer got created: attached() returns true and renderer() non-zero. Does anyone know where exactly the former is needed? When reading common code in eg. ContainerNode, it seems attached() is used to check whether the elements was attached to the rendering tree (which is what I assumed): ... // Add child to the rendering tree. if (attached() && !child->attached() && child->parentNode() == this) { if (shouldLazyAttach) child->lazyAttach(); else ... // Remove from rendering tree if (oldChild->attached()) oldChild->detach(); ... If theres a need to differentiate between those states, I'm fine with your approach, otherwise we should rethink it.

(In reply to comment #6) > (From update of attachment 120073 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=120073&action=review > > Great catches, I still have some questions: > > > LayoutTests/svg/custom/requiredExtensions-runtime-changes.svg:36 > > + var removeElements = document.getElementsByClassName("removeRequiredExtensions"); > > + var addElements = document.getElementsByClassName("addRequiredExtensions"); > > Clever idea, I keep that in mind for further tests of mine :-) Makes the test itself much more readable/hackable. > > > Source/WebCore/ChangeLog:10 > > + SVGStringList, because SVGStringList::reset() adds a single empty string to the list. > > Good catch - but I can't recall why reset() behaves this way. Can we just change the behavior of reset? I'd love to! I couldn't come up with a good reason (as it stands, this makes the empty-hasn't-been-touched list and the empty-was-touched-but-got-cleared list different, and I can't see a good reason for that, at all). > > Source/WebCore/ChangeLog:13 > > + Invalid elements inside <switch> are prevented from creating renderers, > > + but are still attached; SVGTests assumes that invalid elements will always be detached. > > Why is this? I assumed that attached() returns false, if there's no renderer. > I just checked the code, and indeed this assumption is just wrong. > > We're differentiating between two states in dom/Node.cpp (see attach()): > * attach() called no renderer created: attached() returns true and renderer() zero. > * attach() called renderer got created: attached() returns true and renderer() non-zero. > > Does anyone know where exactly the former is needed? > > When reading common code in eg. ContainerNode, it seems attached() is used to check whether the elements was attached to the rendering tree (which is what I assumed): > ... > // Add child to the rendering tree. > if (attached() && !child->attached() && child->parentNode() == this) { > if (shouldLazyAttach) > child->lazyAttach(); > else > ... > > // Remove from rendering tree > if (oldChild->attached()) > oldChild->detach(); > ... > > If theres a need to differentiate between those states, I'm fine with your approach, otherwise we should rethink it. I have no idea if there's a reason for this. I assumed there was, and just wanted to make our assumptions fit. Do you know who might know?

Created attachment 212362 [details] testcase Here is another testcase. I can help with fixing that bug, since it is similar to the implementation of maction/semantics I'm currently working on.

So more precisely: - update to <switch> when foreignObject@requiredExtensions changes is similar to update to <semantics> when annotation-xml@encoding changes (bug 100626) - update to <switch> when its children or attributes change is similar to what I have done to update <semantics>/<maction> (bug 120058)

Finally this will be a bit more complicated than the MathML case, since SVG attributes can be animated. So I don't think I'll spend some time on it in the short term.