According to Erik Dahlstrom, Opera breaks cycles when they are encountered. We're currently ignoring cyclic references. It has been agreed to fix it, they way Opera does.
In order to fix this bug we need to introduce a cache of SVGResources <-> RenderObject. (SVGResources is a set of resources for a RenderObject: clipper/masker/filter etc..). As side effect this fixes bug 42618.
*** Bug 42617 has been marked as a duplicate of this bug. ***
I meant to say bug 42617, not 42618.
*** Bug 15124 has been marked as a duplicate of this bug. ***
*** Bug 26975 has been marked as a duplicate of this bug. ***
*** Bug 41902 has been marked as a duplicate of this bug. ***
Created attachment 62673 [details] Patch
Created attachment 62674 [details] LayoutTest changes
No worries, this beast isn't supposed to be landed/reviewed as is: - I want EWS to run, hence the patch is marked for r? - It will land in smaller pieces, the full patch should just serve as reference, how it's all glued together The SVGMaskElement/SVGForeignObjectElement changes (that remove SVGURIReference) should be landed before. Same for the addition of the new SVGResources/SVGResourcesCache/SVGResourcesCycleSolver classes + build system changes. A later patch can glue them together.
Created attachment 62716 [details] Patch chunk 1 Obsoleting the big patch, leaving it here as reference. "Patch chunk 1" adds the new files and does some prepare work. The SVGResourcesCache concept itself is not glued into WebCore yet - but builds.
Created attachment 62803 [details] Patch chunk 1 - v2 Uploading v2, of "Patch chunk 1" after Dirks first round of review.
Comment on attachment 62803 [details] Patch chunk 1 - v2 Went through the CycleSolver line by line together with Niko. LGTM r=me
Comment on attachment 62803 [details] Patch chunk 1 - v2 Landed "Patch chunk 1 - v2" in r64196.
http://trac.webkit.org/changeset/64196 might have broken GTK Linux 64-bit Debug
Thanks sherriff bot, forgot to land Gtk build system changes, fixed in r64200.
Created attachment 62837 [details] Patch chunk 2 - WebCore Patch that enables the new resources logic.
Created attachment 62838 [details] Patch chunk 2 - LayoutTests
Comment on attachment 62837 [details] Patch chunk 2 - WebCore Great patch, just some notes and questions: WebCore/rendering/RenderPath.cpp:76 + bool RenderPath::fillContains(const FloatPoint& point, bool requiresFill, WindRule fillRule) Why did you remove the const here? WebCore/rendering/RenderPath.cpp:87 + bool RenderPath::strokeContains(const FloatPoint& point, bool requiresStroke) dito WebCore/rendering/RenderPath.cpp:266 + if (diff == StyleDifferenceLayout || diff == StyleDifferenceRepaint) I know that this is a further opimization, but why do we need to recalculate boundaries on a repaint? WebCore/rendering/RenderSVGResourceContainer.h:55 + void markSingleClientForInvalidation(RenderObject*, InvalidationMode); Can you just name it markClientForInvalidation? The difference to markAllClientsForInvalidation should be clear. WebCore/rendering/SVGInlineTextBox.cpp:298 + bool SVGInlineTextBox::acquirePaintingResource(GraphicsContext*& context, RenderObject* renderer, RenderStyle* style) Can you add an Assert for renderer? you give this method parent()->renderer() some lines later without checking the existance. Great design with SVGResources. Are you sure that you removed all inlcudes of resources, where we just need SVGResources now?
(In reply to comment #18) > (From update of attachment 62837 [details]) > Great patch, just some notes and questions: Sure! > > WebCore/rendering/RenderPath.cpp:76 > + bool RenderPath::fillContains(const FloatPoint& point, bool requiresFill, WindRule fillRule) > Why did you remove the const here? Quote from the ChangeLog: + (WebCore::RenderPath::fillContains): Remove constness, to avoid the need to pass around const RenderObjects* to the SVGResourcesCache. > > WebCore/rendering/RenderPath.cpp:87 > + bool RenderPath::strokeContains(const FloatPoint& point, bool requiresStroke) > dito It was the least intrusive solution, to remove the const for fill/strokeContains, and add two const_casts in SVGRenderTreeAsText, otherwhise const_cast would be needed in far more places. > > WebCore/rendering/RenderPath.cpp:266 > + if (diff == StyleDifferenceLayout || diff == StyleDifferenceRepaint) > I know that this is a further opimization, but why do we need to recalculate boundaries on a repaint? Good catch, testing. > WebCore/rendering/RenderSVGResourceContainer.h:55 > + void markSingleClientForInvalidation(RenderObject*, InvalidationMode); > Can you just name it markClientForInvalidation? The difference to markAllClientsForInvalidation should be clear. Yes, will rename. > WebCore/rendering/SVGInlineTextBox.cpp:298 > + bool SVGInlineTextBox::acquirePaintingResource(GraphicsContext*& context, RenderObject* renderer, RenderStyle* style) > Can you add an Assert for renderer? you give this method parent()->renderer() some lines later without checking the existance. Good idea, will do! > Great design with SVGResources. Are you sure that you removed all inlcudes of resources, where we just need SVGResources now? I think so, will recheck.
*** Bug 42616 has been marked as a duplicate of this bug. ***
Created attachment 62937 [details] Patch chunk 2 - v2 - WebCore
Created attachment 62938 [details] Patch chunk 2 - v2 - LayoutTests Includes new baselines for the fixed marker tests.
(In reply to comment #19) > > WebCore/rendering/RenderPath.cpp:266 > > + if (diff == StyleDifferenceLayout || diff == StyleDifferenceRepaint) > > I know that this is a further opimization, but why do we need to recalculate boundaries on a repaint? > Good catch, testing. Fixed, as dicussed on IRC. > > WebCore/rendering/SVGInlineTextBox.cpp:298 > > + bool SVGInlineTextBox::acquirePaintingResource(GraphicsContext*& context, RenderObject* renderer, RenderStyle* style) > > Can you add an Assert for renderer? you give this method parent()->renderer() some lines later without checking the existance. > Good idea, will do! Forgot to include in the patch, added locally. Contains asserts for both renderer & style now. > > Great design with SVGResources. Are you sure that you removed all inlcudes of resources, where we just need SVGResources now? > I think so, will recheck. Done.
LayoutTests/svg/custom/recursive-pattern.svg contained tabs, already fixed. (style bot doesn't complain as it's a test though)
Comment on attachment 62937 [details] Patch chunk 2 - v2 - WebCore LGTM. r=me Please take a look why sl is not building. Bad that the build bot didn't attach build output :-(
(In reply to comment #25) > (From update of attachment 62937 [details]) > LGTM. r=me Please take a look why sl is not building. Bad that the build bot didn't attach build output :-( Thanks, I have no idea why the sl bot keeps failing without posting results. I'll try my luck with landing...
Committed r64275: <http://trac.webkit.org/changeset/64275>
Landed win build fix in r64276. Warned about unreachable code in RenderSVGRoot/Container::selfWillPaint(), because I had: #if ENABLE(FILTERS) .. return ...; #endif return false; Fixed, by wrapping the return false in an #else branch. Fix release builds in r64277. (Maybe that's the reason the mac build complained?) Remove three left-over ASSERTs that resulted in unused variables in release builds.
http://trac.webkit.org/changeset/64275 might have broken Qt Linux Release
Updated Qt results in r64279.