Bug 86941 – Eliminate RenderSVGGradientStop

• Status: RESOLVED
• Resolution: WONTFIX
• Component: SVG

[Nikolas Zimmermann]

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.

[Nikolas Zimmermann]

Created attachment 142867 [details]
Patch

[Build Bot]

Comment on attachment 142867 [details]
Patch

Attachment 142867 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12728541

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Rob Buis]

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;

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

Created attachment 142875 [details]
Patch v2

[Build Bot]

Comment on attachment 142875 [details]
Patch v2

Attachment 142875 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12731226

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Nikolas Zimmermann]

Trying to fix the mac build with v3 - the chromium problem remains.

[Nikolas Zimmermann]

Created attachment 142879 [details]
Patch v3

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Rob Buis]

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?

[Philip Rogers]

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?

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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

[Dirk Schulze]

Comment on attachment 142879 [details]
Patch v3

It sound this bug is not ready for review anymore. removing review flag.

[Dirk Schulze]

Time passed by and I am not sure if we still want to follow this approach. Closing the bug now.