Bug 74749 - Runtime changes to requiredExtensions don't update the enclosing <switch>
Summary: Runtime changes to requiredExtensions don't update the enclosing <switch>
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 88188
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-16 14:30 PST by Tim Horton
Modified: 2014-01-13 01:22 PST (History)
7 users (show)

See Also:


Attachments
repro (1.30 KB, application/xhtml+xml)
2011-12-16 14:30 PST, Tim Horton
no flags Details
repro (first one was broken) (1.15 KB, application/xhtml+xml)
2011-12-19 12:26 PST, Tim Horton
no flags Details
patch (10.55 KB, patch)
2011-12-20 13:55 PST, Tim Horton
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
testcase (516 bytes, text/html)
2013-09-23 10:33 PDT, Frédéric Wang (:fredw)
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2011-12-16 14:30:10 PST
Created attachment 119671 [details]
repro

The attached repro demonstrates that removing an extension requirement (requiredExtensions attribute) at runtime doesn't affect a <switch> which had previously used that attribute to skip over an element.

<rdar://problem/10582174>
Comment 1 Tim Horton 2011-12-19 12:26:23 PST
Created attachment 119901 [details]
repro (first one was broken)
Comment 2 Tim Horton 2011-12-20 13:55:46 PST
Created attachment 120073 [details]
patch
Comment 3 Tim Horton 2011-12-20 13:56:19 PST
Also removes a related comment in SVGSwitchElement which to my eye looks completely false.
Comment 4 WebKit Review Bot 2011-12-20 18:11:28 PST
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
Comment 5 Tim Horton 2011-12-20 21:35:57 PST
(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 6 Nikolas Zimmermann 2011-12-21 01:07:21 PST
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.
Comment 7 Tim Horton 2011-12-21 10:44:13 PST
(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?
Comment 8 Frédéric Wang (:fredw) 2013-09-23 10:33:31 PDT
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.
Comment 9 Frédéric Wang (:fredw) 2013-09-23 10:48:34 PDT
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)
Comment 10 Frédéric Wang (:fredw) 2014-01-06 08:46:57 PST
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.