Bug 40887 – requiredFeatures does not adapt to SVGStringList changes

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Rob Buis]

The test changes the SVGStringList for the requiredFeatures attribute, the element should then become invalid and not be rendered. Currently we don't react to SVGStringList changes at all.

[Rob Buis]

Created attachment 59200 [details]
Patch

[WebKit Review Bot]

Attachment 59200 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/js/JSSVGStringListCustom.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 59200 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3324482

[WebKit Review Bot]

Attachment 59200 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3307457

[Rob Buis]

(In reply to comment #1)
> Created an attachment (id=59200) [details]
> Patch

Some additional comments, I did not add a test yet because I thought we'd maybe import the tests referenced in the url? They look like a great improvement to me compared to the old W3c tests.
I can take a look at making the patch work for other platforms, as it seems to be just a question of finding the right build file(s) to add the new file JSSVGStringListCustom.cpp to.
Cheers,

Rob.

[Rob Buis]

(In reply to comment #1)
> Created an attachment (id=59200) [details]
> Patch

Some additional comments, I did not add a test yet because I thought we'd maybe import the tests referenced in the url? They look like a great improvement to me compared to the old W3c tests.
I can take a look at making the patch work for other platforms, as it seems to be just a question of finding the right build file(s) to add the new file JSSVGStringListCustom.cpp to.
Cheers,

Rob.

[Rob Buis]

Created attachment 59271 [details]
All buildfiles updated

A quick grep indicates that this should be all build files that need to be updated.

[Nikolas Zimmermann]

Comment on attachment 59271 [details]
All buildfiles updated

WebCore/svg/SVGCircleElement.cpp:93
 +          if (!isValid())
I'd reverse the logic, to avoid the not operator:
if (isValid())
    renderer->setNeedsLayout(true);
else
    detach();

Hm, this pattern seems to repeat quite often, how about creating a new static helper function for that in SVGTests, which gets just the SVGElement* passed?
if (SVGTests::processTestAttributeChange(this))
    return;

processTestAttributeChange (maybe you can come up with a better name) would just contain the logic spread around several files.
WebCore/svg/SVGUseElement.cpp:178
 +              invalidateShadowTree();
An exception would be SVGUseElement, here you could not use the new static helper method from SVGTests, because it doesn't use setNeedsLayout(true).

WebCore/svg/SVGTextContentElement.cpp:211
 +  void SVGTextContentElement::svgAttributeChanged(const QualifiedName& attrName)
You need to be careful here, SVGTextContentElement::isKnownAttribute should not recognize SVGTests' attributes anymore, if you override svgAttributeChanged here.

Some further comments:
I am not sure how the updates should actually work, if you're not adapting CodeGeneratorJS and friends.
If you check "JSSVGLengthList.cpp" you'll see that it contains code like:

EncodedJSValue JSC_HOST_CALL jsSVGLengthListPrototypeFunctionGetItem(ExecState* exec)
{
    JSValue thisValue = exec->hostThisValue();
    if (!thisValue.inherits(&JSSVGLengthList::s_info))
        return throwVMTypeError(exec);
    JSSVGLengthList* castedThis = static_cast<JSSVGLengthList*>(asObject(thisValue));
    return JSValue::encode(JSSVGPODListCustom::getItem<JSSVGLengthList, SVGLength>(castedThis, exec, toSVGLength));
}

You basically have to change SVGStringList, to be a SVGPODList<String> instead of SVGList<String>. Then you do not need any custom JSSVGStringList bindings at all.
In CodeGenerator.pm, the "podTypeHashWithWritableProperties" would need to recognize "DOMString", otherwhise there will be need JSSVGPODListCustom* wrappers created in JSSVGStringList.cpp.
I am not sure how easy it is to support strings at all, just give it a try :-)

Ah and of course this needs tests tests tests :-) Ideally all within the new "script-tests" framework.

[Nikolas Zimmermann]

Patch 48898 landed, SVGStringList now notifies its owner element about any list changes.
Rob do you want to revisit this patch? The only thing left to do is extend the if (SVGTests::isKnownAttribute(...)) cases, to handle isValid checks.

There's already a testcase in svg/W3C-SVG-1.1-SE/types-dom-06-f.svg that covers this (we currently fail it, the list operations work fine, though we don't update the renderers, if eg. requiredFeatures changes).

[Rob Buis]

Moin Niko,

Nice! Yes, I'll revisit this patch asap!
Cheers,

Rob.
(In reply to comment #9)
> Patch 48898 landed, SVGStringList now notifies its owner element about any list changes.
> Rob do you want to revisit this patch? The only thing left to do is extend the if (SVGTests::isKnownAttribute(...)) cases, to handle isValid checks.
> 
> There's already a testcase in svg/W3C-SVG-1.1-SE/types-dom-06-f.svg that covers this (we currently fail it, the list operations work fine, though we don't update the renderers, if eg. requiredFeatures changes).

[Nikolas Zimmermann]

(In reply to comment #10)
> Moin Niko,
> 
> Nice! Yes, I'll revisit this patch asap!
> Cheers,
> 

Excellent. I've looked again through your old patch, and it's only partially correct.
If you set requiredFeatures to an invalid value, the rendered would be detached, but if you'd then restore it to a "correct value" it wouldn't reattach. That would need a new testcase, as the svg/W3C-SVG-1.1-SE/ doesn't handle this case.

I guess something like:
if (!attached() && isValid()) attach() else if (attached() && !isValid()) detach() is needed.

It feels odd to have this code in all SVG*Element classes though. Maybe we can merge it with the general logic for display="none", which detaches the renderer as well, and handles reattaching.

void Element::recalcStyle(StyleChange change)

handles the case for display changes (through the RenderStyle::diff() logic).
What we'd need is a reimplementation in SVGElement::recalcStyle, that does the same but with SVGTests::isValid(), then all we need to do is call "setNeedsStyleRecalc()" whenever any of the SVGTests attributes change.

[Rob Buis]

Created attachment 73197 [details]
Patch

[Dirk Schulze]

Comment on attachment 73197 [details]
Patch

r=me

[Nikolas Zimmermann]

(In reply to comment #13)
> (From update of attachment 73197 [details])
> r=me

The code is fine, the tests are not correct.
svg/dynamic-updates tests have to use the scheme:

SVGFooElement-dom-XXX-attr.html
SVGFooElement-svgdom-XXX-prop.html

We have to ensure dynamic updates work via XML DOM (setAttribute) and SVG DOM.

Rob, you basically have to duplicate the entire tests.

[Nikolas Zimmermann]

Comment on attachment 73197 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73197&action=review

> WebCore/svg/SVGForeignObjectElement.cpp:88
> +    if (SVGTests::isKnownAttribute(attrName)) {
> +        processTestAttributeChange();
> +        return;
> +    }
> +

Okay, I have another nit-pick. I'd rather prefer to use:
if (SVGTests::handleAttributeChange(this, attrName))
     return;

SVGTests should have a "static void handleAttributeChange(SVGElement* contextElement, const QualifiedName)" method.
That would look slightly better, and the code would be more encapsulated.

[Rob Buis]

Moin Niko,

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 73197 [details] [details])
> > r=me
> 
> The code is fine, the tests are not correct.
> svg/dynamic-updates tests have to use the scheme:
> 
> SVGFooElement-dom-XXX-attr.html
> SVGFooElement-svgdom-XXX-prop.html
> 
> We have to ensure dynamic updates work via XML DOM (setAttribute) and SVG DOM.
> 
> Rob, you basically have to duplicate the entire tests.

Ok, that is a bit more work, but doable. Seems like it is better to r- up until I have the new patch ready then? Also I found one crash in the svg-dom-types svg file, so a new patch was needed anyway.
Cheers,

Rob.

[Rob Buis]

Created attachment 73816 [details]
Patch

[Rob Buis]

Created attachment 73818 [details]
Patch

[Rob Buis]

Created attachment 73821 [details]
Patch

[Dirk Schulze]

Comment on attachment 73821 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73821&action=review

> LayoutTests/svg/dynamic-updates/script-tests/SVGUseElement-dom-requiredFeatures.js:29
> +    debug("Check that adding something valid to requiredFeatures keeps rendering the element");
> +    useElement.setAttribute("requiredFeatures", "http://www.w3.org/TR/SVG11/feature#Path");
> +    shouldBeEqualToString("document.defaultView.getComputedStyle(useElement, null).display", "");

This looks wrong to me. Talked with Rob and we found out that #Path is not a valid feature. He'll investigate.

[Rob Buis]

Created attachment 73826 [details]
Patch

[Dirk Schulze]

Comment on attachment 73826 [details]
Patch

LGTM. r=me

[WebKit Review Bot]

http://trac.webkit.org/changeset/71969 might have broken Qt Linux Release
The following tests are not passing:
svg/W3C-SVG-1.1-SE/types-dom-06-f.svg

[Nikolas Zimmermann]

Comment on attachment 73826 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73826&action=review

> WebCore/svg/SVGCircleElement.cpp:80
> +    if (SVGTests::handleAttributeChange(const_cast<SVGCircleElement*>(this), attrName))

I would have preferred to avoid this const_cast everywhere! Because....

> WebCore/svg/SVGTests.cpp:101
> +bool SVGTests::handleAttributeChange(SVGElement* element, const QualifiedName& attrName)
> +{
> +    if (knownAttribute(attrName)) {
> +        bool valid = element->isValid();

... you could have early exited here, if (!knownAttribute(attrName)) and save the const_cast.
Only do the const_cast here, if a SVGTests attribute has changed.
In general, early exit is the prefereed style...

Can you fix that in a follow-up commit?

[Dirk Schulze]

(In reply to comment #24)
> (From update of attachment 73826 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73826&action=review
> 
> > WebCore/svg/SVGCircleElement.cpp:80
> > +    if (SVGTests::handleAttributeChange(const_cast<SVGCircleElement*>(this), attrName))
> 
> I would have preferred to avoid this const_cast everywhere! Because....
> 
> > WebCore/svg/SVGTests.cpp:101
> > +bool SVGTests::handleAttributeChange(SVGElement* element, const QualifiedName& attrName)
> > +{
> > +    if (knownAttribute(attrName)) {
> > +        bool valid = element->isValid();
> 
> ... you could have early exited here, if (!knownAttribute(attrName)) and save the const_cast.
> Only do the const_cast here, if a SVGTests attribute has changed.
> In general, early exit is the prefereed style...
> 
> Can you fix that in a follow-up commit?

Indeed, looks better. I spoke with Rob about the const_cast during the review, but didn't see the obvious solution :-P

[Mihai Parparita]

Looks like these tests were checked in without pixel baselines. I don't mind generating Chromium baselines for them, but I thought per the "Pixel test experiment" thread from last month (https://lists.webkit.org/pipermail/webkit-dev/2010-October/thread.html#14669) that svg/ layout tests were supposed to have up to date pixel baselines.

[Mihai Parparita]

(In reply to comment #26)
> Looks like these tests were checked in without pixel baselines. I don't mind generating Chromium baselines for them, but I thought per the "Pixel test experiment" thread from last month (https://lists.webkit.org/pipermail/webkit-dev/2010-October/thread.html#14669) that svg/ layout tests were supposed to have up to date pixel baselines.

Alternatively, it looks like tests don't actually need pixel output, should window.enablePixelTesting be set to false for them?

[Rob Buis]

Moin Niko,

(In reply to comment #24)
> (From update of attachment 73826 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73826&action=review
> 
> > WebCore/svg/SVGCircleElement.cpp:80
> > +    if (SVGTests::handleAttributeChange(const_cast<SVGCircleElement*>(this), attrName))
> 
> I would have preferred to avoid this const_cast everywhere! Because....
> 
> > WebCore/svg/SVGTests.cpp:101
> > +bool SVGTests::handleAttributeChange(SVGElement* element, const QualifiedName& attrName)
> > +{
> > +    if (knownAttribute(attrName)) {
> > +        bool valid = element->isValid();
> 
> ... you could have early exited here, if (!knownAttribute(attrName)) and save the const_cast.
> Only do the const_cast here, if a SVGTests attribute has changed.
> In general, early exit is the prefereed style...
> 
> Can you fix that in a follow-up commit?

Yep, can look at that tonight.
Cheers,

Rob.

[Nikolas Zimmermann]

(In reply to comment #26)
> Looks like these tests were checked in without pixel baselines. I don't mind generating Chromium baselines for them, but I thought per the "Pixel test experiment" thread from last month (https://lists.webkit.org/pipermail/webkit-dev/2010-October/thread.html#14669) that svg/ layout tests were supposed to have up to date pixel baselines.

Yes, that's true. Rob probably forgot to add them, I'll take care today, if no one else does.

[Rob Buis]

Created attachment 73926 [details]
Patch to correct const_cast style problem

[Dirk Schulze]

Comment on attachment 73926 [details]
Patch to correct const_cast style problem

View in context: https://bugs.webkit.org/attachment.cgi?id=73926&action=review

> WebCore/ChangeLog:8
> +        Centralize the const_cast handling and only do it if needed.

A bit more detailed please. You removed the const_cast in all svgAttributeChanged functions and moved it to SVGTest::handleAttributeChange. You added an early return to handleAttributeChange to avoid unnecessary const_casts. This is a cleanup patch without change of functionality, so you don't need any new tests.

> WebCore/svg/SVGTests.cpp:98
> +bool SVGTests::handleAttributeChange(const SVGElement* constElement, const QualifiedName& attrName)

constElement sounds bad maybe use targetElement? Make a proposal.

> WebCore/svg/SVGTests.cpp:102
> +    SVGElement* element = const_cast<SVGElement*>(constElement);

can you use svgElement please? sounds better.

[Rob Buis]

This work has landed and a few problems with tests have seperate bugs that are also
almost fixed, so closing this one.
Cheers,

Rob.