Bug 40887

Created attachment 59200 [details] Patch

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.

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

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

(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.

(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.

Created attachment 59271 [details] All buildfiles updated A quick grep indicates that this should be all build files that need to be updated.

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.

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).

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).

(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.

Created attachment 73197 [details] Patch

Comment on attachment 73197 [details] Patch r=me

(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 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.

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.

Created attachment 73816 [details] Patch

Created attachment 73818 [details] Patch

Created attachment 73821 [details] Patch

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.

Created attachment 73826 [details] Patch

Comment on attachment 73826 [details] Patch LGTM. r=me

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 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?

(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

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.

(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?

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.

(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.

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

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.

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