Bug 43031 – WebKit shouldn't ignore resource cycles, but break them as Opera does

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Nikolas Zimmermann]

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.

[Nikolas Zimmermann]

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.

[Nikolas Zimmermann]

*** Bug 42617 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

I meant to say bug 42617, not 42618.

[Nikolas Zimmermann]

*** Bug 15124 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

*** Bug 26975 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

*** Bug 41902 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

Created attachment 62673 [details]
Patch

[Nikolas Zimmermann]

Created attachment 62674 [details]
LayoutTest changes

[Nikolas Zimmermann]

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.

[Nikolas Zimmermann]

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.

[Nikolas Zimmermann]

Created attachment 62803 [details]
Patch chunk 1 - v2

Uploading v2, of "Patch chunk 1" after Dirks first round of review.

[Dirk Schulze]

Comment on attachment 62803 [details]
Patch chunk 1 - v2

Went through the CycleSolver line by line together with Niko. LGTM r=me

[Nikolas Zimmermann]

Comment on attachment 62803 [details]
Patch chunk 1 - v2

Landed "Patch chunk 1 - v2" in r64196.

[WebKit Review Bot]

http://trac.webkit.org/changeset/64196 might have broken GTK Linux 64-bit Debug

[Nikolas Zimmermann]

Thanks sherriff bot, forgot to land Gtk build system changes, fixed in r64200.

[Nikolas Zimmermann]

Created attachment 62837 [details]
Patch chunk 2 - WebCore

Patch that enables the new resources logic.

[Nikolas Zimmermann]

Created attachment 62838 [details]
Patch chunk 2 - LayoutTests

[Dirk Schulze]

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?

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

*** Bug 42616 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

Created attachment 62937 [details]
Patch chunk 2 - v2 - WebCore

[Nikolas Zimmermann]

Created attachment 62938 [details]
Patch chunk 2 - v2 - LayoutTests

Includes new baselines for the fixed marker tests.

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

LayoutTests/svg/custom/recursive-pattern.svg contained tabs, already fixed. (style bot doesn't complain as it's a test though)

[Dirk Schulze]

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

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

Committed r64275: <http://trac.webkit.org/changeset/64275>

[Nikolas Zimmermann]

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.

[WebKit Review Bot]

http://trac.webkit.org/changeset/64275 might have broken Qt Linux Release

[Nikolas Zimmermann]

Updated Qt results in r64279.