Bug 69459

Created attachment 125260 [details] Patch

Comment on attachment 125260 [details] Patch Attachment 125260 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11416198

Comment on attachment 125260 [details] Patch Attachment 125260 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11422123

Comment on attachment 125260 [details] Patch Attachment 125260 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11423113

Comment on attachment 125260 [details] Patch Attachment 125260 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11423121

Created attachment 125325 [details] Patch

Comment on attachment 125325 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125325&action=review > Source/WebCore/rendering/svg/RenderSVGContainer.cpp:68 > + bool selfNeedsLayout = this->selfNeedsLayout(); No need to cache this, as you're calling an inline function. The whole logic for this should be in RenderSVGViewportContainer only, not in RenderSVGContainer - you're wasting memory by storing/tracking m_isLayoutSizeChanged for eg. any <g> now. > Source/WebCore/rendering/svg/RenderSVGContainer.cpp:74 > + SVGSVGElement* svg = static_cast<SVGSVGElement*>(node()); > + ASSERT(svg); > + m_isLayoutSizeChanged = svg->hasRelativeLengths() && m_needsBoundariesUpdate; No need to ASsERT(svg) here anymore. Just use m_isLayoutSizeChanged = static_cast<SVGSVGElement*>(node())->hasRelativeLengths ... but only do this once you moved this into RenderSVGViewportContainer. > LayoutTests/svg/custom/dynamic-viewBox-2.svg:14 > + if (window.layoutTestController) > + layoutTestController.waitUntilDone(); > + > + window.setTimeout(function() { > + document.getElementById('s').setAttribute("viewBox", "0 0 200 200"); > + > + if (window.layoutTestController) > + layoutTestController.notifyDone(); > + },0); That's considered harmful nowadays, what you want here is: <script> if (window.layoutTestController) layoutTestController.waitUntilDone(); function runTest() { document.rootElement.offsetLeft; if (window.layoutTestController) layoutTestController.display(); document.getElementById('s').setAttribute(...) if (window.layoutTestController) layoutTestController.notifyDone(); } setTimeout(runTest, 0); </script> There's no guarantee otherwise that you're painting before the change -- currently with your test its painted the first time after your setAttribute("viewBox") not before, so you can't test dynamic changes with this approach. I learned this the hard way.

Created attachment 125348 [details] Patch

Comment on attachment 125348 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125348&action=review > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:44 > + SVGRenderSupport::layoutChildren(this, selfNeedsLayout() || SVGRenderSupport::filtersForceContainerLayout(this)); Call the base class here instead of duplicating this. > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:45 > + m_isLayoutSizeChanged = false; Why is it necessary to reset this? Does anyone query it after SVGRenderSupport::layoutChildren() ran, I don't think so? > Source/WebCore/svg/SVGSVGElement.cpp:297 > + || attrName == SVGNames::viewBoxAttr > || SVGFitToViewBox::isKnownAttribute(attrName)) { > updateRelativeLengths = true; > updateRelativeLengthsInformation(); Just noticed that this is wrong. You should add a new case below instead of here: if (updateRelativeLengths || attrName == SVGNames::viewBoxAttr || SVGLangSpace::isKnownAttribute(attrName) || SVGExternalResourcesRequired::isKnownAttribute(attrName) ... as you shouldn't call updateRelativeLengthsInformation() if viewBox changes, only if x/y/width/height changed. > LayoutTests/svg/custom/dynamic-viewBox-2.svg:7 > + if (window.layoutTestController) > + layoutTestController.waitUntilDone(); Test still not updated.

Created attachment 125438 [details] Patch

(In reply to comment #9) > (From update of attachment 125348 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125348&action=review > > > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:44 > > + SVGRenderSupport::layoutChildren(this, selfNeedsLayout() || SVGRenderSupport::filtersForceContainerLayout(this)); > > Call the base class here instead of duplicating this. I changed the way how we set the flag. This way we don't have code duplication at all. > > > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:45 > > + m_isLayoutSizeChanged = false; > > Why is it necessary to reset this? Does anyone query it after SVGRenderSupport::layoutChildren() ran, I don't think so? I checked it. Just the children call ask for the flag during the layout phase. At this point we already set the flag. Removed the resetting in RenderSVGRoot as well, for compatibility reasons. > > > Source/WebCore/svg/SVGSVGElement.cpp:297 > > + || attrName == SVGNames::viewBoxAttr > > || SVGFitToViewBox::isKnownAttribute(attrName)) { > > updateRelativeLengths = true; > > updateRelativeLengthsInformation(); > > Just noticed that this is wrong. You should add a new case below instead of here: > if (updateRelativeLengths > || attrName == SVGNames::viewBoxAttr > || SVGLangSpace::isKnownAttribute(attrName) > || SVGExternalResourcesRequired::isKnownAttribute(attrName) > > ... as you shouldn't call updateRelativeLengthsInformation() if viewBox changes, only if x/y/width/height changed. Did it already, uploading it with the next patch in a minute. > > > LayoutTests/svg/custom/dynamic-viewBox-2.svg:7 > > + if (window.layoutTestController) > > + layoutTestController.waitUntilDone(); > > Test still not updated. I changed the test completely. We now run the code after onload event and follow repaint logic for HTML. Just like James suggested. Next step will be using Ref Test instead of pixel test. But i need to check this first. rniwa just landed the changes that make it possible for SVG files.

Created attachment 125441 [details] Patch

Comment on attachment 125441 [details] Patch Attachment 125441 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11420516

Comment on attachment 125441 [details] Patch Attachment 125441 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11420521

Created attachment 125453 [details] Patch

Comment on attachment 125453 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125453&action=review The code is perfect, the test case is not - but that's not your faut, the current repaint test framework is not usable as is for SVG. We need to call runRepaintTest from a onload="setTimeout(runRepaintTest, 0)" statement, otherwise we're not seeing the gray rect from layoutTestController.display(), nor the repaints. This is because the HTML load even and SVG load event work differently - I hope to have my patch done soon, which fixes this. Can this wait another day? > Source/WebCore/rendering/svg/RenderSVGContainer.cpp:67 > + setLayoutSizeChanged(); The naming of the function is wrong, and should be made so obvious that you don't need a comment. Call it eg. determineIfLayoutSizeChanged(). > LayoutTests/svg/repaint/resources/repaint.js:1 > +function runRepaintTest() You shouldn't duplicate this, but instead use the one in fast/repaint/resources/. Note: there's a problem with your current approach - did you notice that your png file doesn't actually show any gray rect, and doesn't show repaint changes? There's an annoying difference between the HTML/SVG load event timing, that makes runRepaintTest not work, when calling right from onload. I invented a runSVGRepaintTest() for this, which calls runRepaintTest with a setTimeout(, 0), and layoutTestController.waitUntilDone(). I hope to have a patch ready soon, which covers this. Currently the way you're testing this is not reliable. Try embedded your <svg> in a <htm> document, and listen to <body onload="runRepaintTest()" and note that the gray rect is now painted. > LayoutTests/svg/repaint/resources/repaint.js:4 > + document.rootElement.offsetTop; I see why you copied this, I ran into the same issue, but used: if (document.body) document.body.offsetTop; else if (document.rootElement) ... to avoid having to duplicate the code.

(In reply to comment #16) > (From update of attachment 125453 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125453&action=review > Note: there's a problem with your current approach - did you notice that your png file doesn't actually show any gray rect, and doesn't show repaint changes? I haven't use display and don#t plan to use it. I don't see how I would get a benefit of it? I already see the difference. Either the size is correct, than repainting works. Or it is not correct and something is wrong. The repaint testing framework makes it harder to create ref tests. While the gray might make sense for certain situations, we should't use it when not needed. And I expressly want to please you not to introduce it in tests that don't necessarily need them! Ref tests will be used by the W3C, and are already used at Mozilla. For test interoperability and for decreasing test expectation sizes, we should use Ref tests as much as possible!

(In reply to comment #16) > (From update of attachment 125453 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125453&action=review > > The code is perfect, the test case is not - but that's not your faut, the current repaint test framework is not usable as is for SVG. We need to call runRepaintTest from a onload="setTimeout(runRepaintTest, 0)" statement, otherwise we're not seeing the gray rect from layoutTestController.display(), nor the repaints. This is because the HTML load even and SVG load event work differently - I hope to have my patch done soon, which fixes this. Can this wait another day? > > > Source/WebCore/rendering/svg/RenderSVGContainer.cpp:67 > > + setLayoutSizeChanged(); > > The naming of the function is wrong, and should be made so obvious that you don't need a comment. > Call it eg. determineIfLayoutSizeChanged(). > > > LayoutTests/svg/repaint/resources/repaint.js:1 > > +function runRepaintTest() > > You shouldn't duplicate this, but instead use the one in fast/repaint/resources/. > Note: there's a problem with your current approach - did you notice that your png file doesn't actually show any gray rect, and doesn't show repaint changes? > There's an annoying difference between the HTML/SVG load event timing, that makes runRepaintTest not work, when calling right from onload. > I invented a runSVGRepaintTest() for this, which calls runRepaintTest with a setTimeout(, 0), and layoutTestController.waitUntilDone(). > I hope to have a patch ready soon, which covers this. > Currently the way you're testing this is not reliable. > > Try embedded your <svg> in a <htm> document, and listen to <body onload="runRepaintTest()" and note that the gray rect is now painted. Wouldn't it be a better approach to fix onload event instead? I think we should avoid using setTimeout and asynchronous behavior. Who knows if we end up in the same situation later? The SVG WG decided to align with the behavior of HTML <script> and events anyway. Also, I don't want to embed a SVG document into an HTML document. Some behaviors (like some events) might behave differently on embedding. For instance 'blur' event works in HTML5 documents for SVG, while it didn't work on my tests with plain SVGs in the past (not checked if it is still true). I'll wait with uploading a new test. I want to make sure that we agree on a union testing behavior on SVG. And also port existing tests to this behavior. Hopefully considering using more Ref Tests :)

Comment on attachment 125453 [details] Patch Removing review flag till we agree on a new testing framework.

(In reply to comment #17) > I haven't use display and don#t plan to use it. I don't see how I would get a benefit of it? I already see the difference. Either the size is correct, than repainting works. Or it is not correct and something is wrong. Dirk, using display() is the only way to guarantee a draw of our view. Consider: <svg> <rect/> <script> setTimeout(doIt, 0); fonction doIt() { // change rect layoutTestController.notifyDone(); } </script> </svg> We hoped that doIt, was called after the first _rendering_. Now we guarantee, that once the document is loaded, we call runRepaintTest(), which calls display() - and it forces a layout update before. We can be sure that the view actually painted, before we "change it". The usage of display() for repainting tests is absolutely necessary. > The repaint testing framework makes it harder to create ref tests. While the gray might make sense for certain situations, we should't use it when not needed. Ref tests can not be used to test repainting, properly. For that we need the repaint.js harness. I don't want too elaborate too much on it now. > And I expressly want to please you not to introduce it in tests that don't necessarily need them! That's your misunderstanding. Every test that doesn't do dumpAsText() needs this, otherwise they're flakey by design (relying on setTimeout, etc.).

(In reply to comment #18) > Wouldn't it be a better approach to fix onload event instead? I think we should avoid using setTimeout and asynchronous behavior. Who knows if we end up in the same situation later? The SVG WG decided to align with the behavior of HTML <script> and events anyway. Heh, the SVG onload event timing is a really complex topic. I guess you don't consider externalResourcesRequired & friends - it makes it absolutely impossible to be 1:1 aligned to the HTML load event, but we can get closer. For now I just centralize the "setTimeout" hack, which is only needed for plain SVG documents that want to use repaint.js - in repaint.js runSVGRepaintTest(). Once we fix the "unload bug" we can do a s/runSVGRepaintTest/runRepaintTests/ and be done. I don't plan to fix onload support right in the patch at bug 77736, it's out of scope for that. > > Also, I don't want to embed a SVG document into an HTML document. Some behaviors (like some events) might behave differently on embedding. For instance 'blur' event works in HTML5 documents for SVG, while it didn't work on my tests with plain SVGs in the past (not checked if it is still true). I just wanted to emphasize that there's a key difference between using repaint.js from a HTML and a SVG document, due the timing differences. I don't want you to use HTML files for testing, just to convince yourself that your last patch was wrong, as you didn't use display() correctly, due the onload bug :-) Whenever you use display() and don't see gray rects - you've timed something wrong. > > I'll wait with uploading a new test. I want to make sure that we agree on a union testing behavior on SVG. And also port existing tests to this behavior. Hopefully considering using more Ref Tests :) Ref tests are not an option for testing repainting. I agree that some of the tests in bug 77736, could be converted to ref tests, but most of them really check repainting, or at least pretended they would up until now ;-)

(In reply to comment #20) > > The repaint testing framework makes it harder to create ref tests. While the gray might make sense for certain situations, we should't use it when not needed. > Ref tests can not be used to test repainting, properly. For that we need the repaint.js harness. I don't want too elaborate too much on it now. Ok, I decided to elaborate :-) - Use ref tests, when your document contains no scripting/animations. I made several tests in the pass that showed eg. 5 green rects on the left (generated by eg. filter/pattern), a vertical separator line, and the same again (as reference image). These tests don't use any scripting, nor animations, and thus should be converted to a reftest (left side as test case, right side as expected.svg) - When we use a script or animation, to change eg. a rect's fill color/width, etc.. then we want to use a repaint test, and thus need the repaint.js harness. For *.svg files, we currently need to use runSVGRepaintTest() from <svg onload="", for non-svg files, use <body onload="runRepaintTest()"> and supply a function repaintTest() in both cases. SEe bug 77736 for more details. That summarizes my point-of-view on that topic.

(In reply to comment #22) > (In reply to comment #20) > > > The repaint testing framework makes it harder to create ref tests. While the gray might make sense for certain situations, we should't use it when not needed. > > Ref tests can not be used to test repainting, properly. For that we need the repaint.js harness. I don't want too elaborate too much on it now. > Ok, I decided to elaborate :-) > - Use ref tests, when your document contains no scripting/animations. I made several tests in the pass that showed eg. 5 green rects on the left (generated by eg. filter/pattern), a vertical separator line, and the same again (as reference image). These tests don't use any scripting, nor animations, and thus should be converted to a reftest (left side as test case, right side as expected.svg) > - When we use a script or animation, to change eg. a rect's fill color/width, etc.. then we want to use a repaint test, and thus need the repaint.js harness. For *.svg files, we currently need to use runSVGRepaintTest() from <svg onload="", for non-svg files, use <body onload="runRepaintTest()"> and supply a function repaintTest() in both cases. SEe bug 77736 for more details. > > That summarizes my point-of-view on that topic. At least we can change all tests at once by editing repaint.js if we came up with another solution in the future. I wonder why we can't still use Ref Tests, even with the gray background?!? I mean, at the end you are still comparing image results. You don't do anything else with ref tests. Of course it makes more work. And maybe we can refactor some tests that we can reuse ref test results as much as possible. I will create a new pixel test with your logic. I just want to say that I don't see a reason not to do ref tests even with display().

Created attachment 125994 [details] Patch

Comment on attachment 125994 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125994&action=review > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:235 > +static inline bool layoutSizeOfNearestViewportChanged(const RenderObject* start) That sounds like "styleDidChange", something which already happened. Darin could find better words for this :-) Anyhow, still fine with me. > LayoutTests/svg/repaint/dynamic-viewBox-2.svg:1 > +<svg width="200" height="200" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" onload="runRepaintTest();"> As I told you on IRC, this is not going to work. You still don't have gray overlay rects :( Your testcase is borken. You still need to use runSVGRepaintTest(), not runRepaintTest() - then you'll see the overlays. Currently you can't know when painting happens. I hope to land a patch soon, which avoids the need for runSVGRepaintTest, then even your notifyDone() call could vanish. Stay tuned...

(In reply to comment #25) > I hope to land a patch soon, which avoids the need for runSVGRepaintTest, then even your notifyDone() call could vanish. Stay tuned... Patch is in, you can now provide a hack-free repaint test, without special SVG surgery.

Created attachment 126050 [details] Patch

Comment on attachment 126050 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126050&action=review Almost there... > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:43 > + m_isLayoutSizeChanged = static_cast<SVGSVGElement*>(node())->hasRelativeLengths() && selfNeedsLayout(); Ouch, just spotted this. It's a security bug, you have to check for <svg> tag first. <symbol> inside <defs> create RenderSVGVieewportContainers recently, see the related code in calcViewport(). > LayoutTests/svg/repaint/dynamic-viewBox-2.svg:1 > +<svg width="200" height="200" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" onload="runRepaintTest();"> The ; is not necessary.

Created attachment 126052 [details] Patch

Comment on attachment 126052 [details] Patch r=me.

(In reply to comment #30) > (From update of attachment 126052 [details]) > r=me. Ah and please rename the test before landing from dynamic-viewBox-2.svg (as there is no -1.svg) to inner-svg-change-viewBox.svg or sth. similar.

Comment on attachment 126052 [details] Patch Patch landed in http://trac.webkit.org/changeset/107108 Removing review flag and closing bug now.