Bug 40887 - requiredFeatures does not adapt to SVGStringList changes
Summary: requiredFeatures does not adapt to SVGStringList changes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Rob Buis
URL: http://dev.w3.org/SVG/profiles/1.1F2/...
Keywords:
Depends on: 48898
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-20 10:46 PDT by Rob Buis
Modified: 2010-11-23 23:37 PST (History)
6 users (show)

See Also:


Attachments
Patch (23.52 KB, patch)
2010-06-20 11:07 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
All buildfiles updated (27.50 KB, patch)
2010-06-21 12:09 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (59.26 KB, patch)
2010-11-07 10:54 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (139.57 KB, patch)
2010-11-13 02:25 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (115.14 KB, patch)
2010-11-13 03:14 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (174.60 KB, patch)
2010-11-13 06:45 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (175.00 KB, patch)
2010-11-13 07:41 PST, Rob Buis
krit: review+
Details | Formatted Diff | Diff
Patch to correct const_cast style problem (10.09 KB, patch)
2010-11-15 13:19 PST, Rob Buis
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2010-06-20 10:46:42 PDT
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.
Comment 1 Rob Buis 2010-06-20 11:07:02 PDT
Created attachment 59200 [details]
Patch
Comment 2 WebKit Review Bot 2010-06-20 11:08:45 PDT
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.
Comment 3 Early Warning System Bot 2010-06-20 11:17:36 PDT
Attachment 59200 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3324482
Comment 4 WebKit Review Bot 2010-06-20 13:33:53 PDT
Attachment 59200 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3307457
Comment 5 Rob Buis 2010-06-21 01:39:29 PDT
(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.
Comment 6 Rob Buis 2010-06-21 01:54:58 PDT
(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.
Comment 7 Rob Buis 2010-06-21 12:09:38 PDT
Created attachment 59271 [details]
All buildfiles updated

A quick grep indicates that this should be all build files that need to be updated.
Comment 8 Nikolas Zimmermann 2010-06-23 01:00:12 PDT
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.
Comment 9 Nikolas Zimmermann 2010-11-03 05:47:44 PDT
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).
Comment 10 Rob Buis 2010-11-03 05:53:21 PDT
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).
Comment 11 Nikolas Zimmermann 2010-11-03 06:10:23 PDT
(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.
Comment 12 Rob Buis 2010-11-07 10:54:10 PST
Created attachment 73197 [details]
Patch
Comment 13 Dirk Schulze 2010-11-07 12:23:36 PST
Comment on attachment 73197 [details]
Patch

r=me
Comment 14 Nikolas Zimmermann 2010-11-07 23:32:32 PST
(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.
Comment 15 Nikolas Zimmermann 2010-11-07 23:37:31 PST
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.
Comment 16 Rob Buis 2010-11-07 23:39:51 PST
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.
Comment 17 Rob Buis 2010-11-13 02:25:39 PST
Created attachment 73816 [details]
Patch
Comment 18 Rob Buis 2010-11-13 03:14:55 PST
Created attachment 73818 [details]
Patch
Comment 19 Rob Buis 2010-11-13 06:45:06 PST
Created attachment 73821 [details]
Patch
Comment 20 Dirk Schulze 2010-11-13 07:15:06 PST
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.
Comment 21 Rob Buis 2010-11-13 07:41:56 PST
Created attachment 73826 [details]
Patch
Comment 22 Dirk Schulze 2010-11-13 09:03:01 PST
Comment on attachment 73826 [details]
Patch

LGTM. r=me
Comment 23 WebKit Review Bot 2010-11-13 11:02:14 PST
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
Comment 24 Nikolas Zimmermann 2010-11-14 11:10:15 PST
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?
Comment 25 Dirk Schulze 2010-11-14 15:22:53 PST
(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
Comment 26 Mihai Parparita 2010-11-14 15:57:51 PST
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.
Comment 27 Mihai Parparita 2010-11-14 16:27:53 PST
(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?
Comment 28 Rob Buis 2010-11-15 00:01:39 PST
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.
Comment 29 Nikolas Zimmermann 2010-11-15 00:42:37 PST
(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.
Comment 30 Rob Buis 2010-11-15 13:19:33 PST
Created attachment 73926 [details]
Patch to correct const_cast style problem
Comment 31 Dirk Schulze 2010-11-15 13:27:01 PST
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.
Comment 32 Rob Buis 2010-11-23 23:37:59 PST
This work has landed and a few problems with tests have seperate bugs that are also
almost fixed, so closing this one.
Cheers,

Rob.