As discussed on the last video chat, resources need to be reworked - there's no reason for gradients to have renderers, and participate in layout(). As first step in that direction I've killed RenderSVGGradientStop - with minimal effort.
Created attachment 142867 [details] Patch
Comment on attachment 142867 [details] Patch Attachment 142867 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12728541
Comment on attachment 142867 [details] Patch Attachment 142867 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12730178 New failing tests: transitions/svg-transitions.html
Created attachment 142871 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 142867 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142867&action=review I like this change a lot and can r+ once the Mac problem is fixed. > Source/WebCore/ChangeLog:8 > + Eliminate RenderSVGGradientStop. Its sole purpose was to retreive style changes to <stop> elements. Typo retrieve > Source/WebCore/svg/SVGStopElement.cpp:92 > + RenderObject* renderer = parentNode() ? parentNode()->renderer() :0; : 0;
(In reply to comment #5) > (From update of attachment 142867 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142867&action=review > > I like this change a lot and can r+ once the Mac problem is fixed. Thanks for the review! I'll upload a new version to await EWS results. I couldn't reproduce the transitions/ failure chromium saw. Let's retry.
Created attachment 142875 [details] Patch v2
Comment on attachment 142875 [details] Patch v2 Attachment 142875 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12731226
Comment on attachment 142875 [details] Patch v2 Attachment 142875 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12723993 New failing tests: transitions/svg-transitions.html
Created attachment 142878 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Trying to fix the mac build with v3 - the chromium problem remains.
Created attachment 142879 [details] Patch v3
Comment on attachment 142879 [details] Patch v3 Attachment 142879 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12724852 New failing tests: transitions/svg-transitions.html
Created attachment 142880 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 142879 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=142879&action=review Again, looks good, but still a chromium problem. > Source/WebCore/ChangeLog:8 > + Eliminate RenderSVGGradientStop. Its sole purpose was to retrive style changes to <stop> elements. Small nit, "retrieve". Although you may mean resolve here?
Niko, I looked into this test failure and found that it's reproducible on both mac/safari and linux/chromium (probably everywhere else too). The issue is that LayoutTestController requires a renderer to be able to pauseTransitionAtTime. I'm not sure why the mac EWS bot didn't protest but it should have as I have it failing locally. Via manual examination the failing test does pass. Because this code is heavily tested elsewhere I would recommend we put this in, mark the test as failing, and file a bug against DumpRenderTree. One nit: It looks like we already have one checked in failed expectation (FAIL - "fill" property for "rect7"). If you do go ahead with checking this in, can you properly mark this test as failing and remove the failure from svg-transitions-expected.html?
(In reply to comment #16) > Niko, > > I looked into this test failure and found that it's reproducible on both mac/safari and linux/chromium (probably everywhere else too). The issue is that LayoutTestController requires a renderer to be able to pauseTransitionAtTime. I'm not sure why the mac EWS bot didn't protest but it should have as I have it failing locally. Well this transition/ test passes locally on my Mac! I have no clue why it does, and will try a rebuild from scratch now. I agree it should fail, as <stop> elements no longer have renderers. Are we sure my patch doesn't break CSS Transitions on <stop> elements in general? After all RenderOBject::setAnimatableStyle needs to be called, to let them take effect. That would mean my change breaks CSS Transitions on <stop> elements in general. I'll verify this once my build is ready. > Via manual examination the failing test does pass. Because this code is heavily tested elsewhere I would recommend we put this in, mark the test as failing, and file a bug against DumpRenderTree. Are you sure the stop parts also pass? Transitions are also not heavily tested anywhere, I think it's the only test for that, and we need to keep it working :-) > One nit: It looks like we already have one checked in failed expectation (FAIL - "fill" property for "rect7"). If you do go ahead with checking this in, can you properly mark this test as failing and remove the failure from svg-transitions-expected.html? I've checked that failing rect7 transition: we don't support url#foo) black -> url(#foo) blue transitions yet, so this fails for another reason (expected failure).
(In reply to comment #17) I'm back on the right track, new-run-webkit-tests transitions shows the SVG Transition test failure for me as well now, phew. I'll investigate now on how to continue.
(In reply to comment #18) > (In reply to comment #17) > I'm back on the right track, new-run-webkit-tests transitions shows the SVG Transition test failure for me as well now, phew. I'll investigate now on how to continue. It's obvious, CSS Transitions/Animations on <stop> elements are broken. No-one calls AnimationController::updateAnimations() on the render style that SVGStopElement receives & stores. The other user of the nonRenderRenderStyle() code path is HTMLOption/OptGroupElement which don't care about CSS Transitions/Animations in their styles. I'll try to invoke AnimationController::updateAnimations() in SVGStopElement::setRenderStyle()... let's see.
(In reply to comment #19) > I'll try to invoke AnimationController::updateAnimations() in SVGStopElement::setRenderStyle()... let's see. Really tricky area. I've implemented a prototype concept of CSS Transitions/Animations not depending on a RenderObject - it now supports render-less Nodes as well. The problem is gone, and everything is fixed locally. If bug 87373 is accepted, I'm going to update the patch for this bug (only needs minimal changes).
Comment on attachment 142879 [details] Patch v3 It sound this bug is not ready for review anymore. removing review flag.
Time passed by and I am not sure if we still want to follow this approach. Closing the bug now.