Bug 43031

Summary: WebKit shouldn't ignore resource cycles, but break them as Opera does
Product: WebKit Reporter: Nikolas Zimmermann <zimmermann>
Component: SVGAssignee: Nikolas Zimmermann <zimmermann>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, jay, mabbo, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
LayoutTest changes
none
Patch chunk 1
none
Patch chunk 1 - v2
krit: review+
Patch chunk 2 - WebCore
none
Patch chunk 2 - LayoutTests
none
Patch chunk 2 - v2 - WebCore
krit: review+, krit: commit-queue-
Patch chunk 2 - v2 - LayoutTests krit: review+

Description Nikolas Zimmermann 2010-07-27 02:13:48 PDT
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.
Comment 1 Nikolas Zimmermann 2010-07-27 02:15:09 PDT
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.
Comment 2 Nikolas Zimmermann 2010-07-27 02:15:25 PDT
*** Bug 42617 has been marked as a duplicate of this bug. ***
Comment 3 Nikolas Zimmermann 2010-07-27 02:15:48 PDT
I meant to say bug 42617, not 42618.
Comment 4 Nikolas Zimmermann 2010-07-27 03:22:07 PDT
*** Bug 15124 has been marked as a duplicate of this bug. ***
Comment 5 Nikolas Zimmermann 2010-07-27 03:23:13 PDT
*** Bug 26975 has been marked as a duplicate of this bug. ***
Comment 6 Nikolas Zimmermann 2010-07-27 03:27:48 PDT
*** Bug 41902 has been marked as a duplicate of this bug. ***
Comment 7 Nikolas Zimmermann 2010-07-27 03:31:12 PDT
Created attachment 62673 [details]
Patch
Comment 8 Nikolas Zimmermann 2010-07-27 03:31:52 PDT
Created attachment 62674 [details]
LayoutTest changes
Comment 9 Nikolas Zimmermann 2010-07-27 03:44:35 PDT
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.
Comment 10 Nikolas Zimmermann 2010-07-27 11:28:44 PDT
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.
Comment 11 Nikolas Zimmermann 2010-07-28 01:52:10 PDT
Created attachment 62803 [details]
Patch chunk 1 - v2

Uploading v2, of "Patch chunk 1" after Dirks first round of review.
Comment 12 Dirk Schulze 2010-07-28 03:46:10 PDT
Comment on attachment 62803 [details]
Patch chunk 1 - v2

Went through the CycleSolver line by line together with Niko. LGTM r=me
Comment 13 Nikolas Zimmermann 2010-07-28 03:57:42 PDT
Comment on attachment 62803 [details]
Patch chunk 1 - v2

Landed "Patch chunk 1 - v2" in r64196.
Comment 14 WebKit Review Bot 2010-07-28 04:16:38 PDT
http://trac.webkit.org/changeset/64196 might have broken GTK Linux 64-bit Debug
Comment 15 Nikolas Zimmermann 2010-07-28 04:21:07 PDT
Thanks sherriff bot, forgot to land Gtk build system changes, fixed in r64200.
Comment 16 Nikolas Zimmermann 2010-07-28 09:47:14 PDT
Created attachment 62837 [details]
Patch chunk 2 - WebCore

Patch that enables the new resources logic.
Comment 17 Nikolas Zimmermann 2010-07-28 09:48:03 PDT
Created attachment 62838 [details]
Patch chunk 2 - LayoutTests
Comment 18 Dirk Schulze 2010-07-28 10:34:54 PDT
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?
Comment 19 Nikolas Zimmermann 2010-07-29 00:12:26 PDT
(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.
Comment 20 Nikolas Zimmermann 2010-07-29 04:22:36 PDT
*** Bug 42616 has been marked as a duplicate of this bug. ***
Comment 21 Nikolas Zimmermann 2010-07-29 04:44:36 PDT
Created attachment 62937 [details]
Patch chunk 2 - v2 - WebCore
Comment 22 Nikolas Zimmermann 2010-07-29 04:45:34 PDT
Created attachment 62938 [details]
Patch chunk 2 - v2 - LayoutTests

Includes new baselines for the fixed marker tests.
Comment 23 Nikolas Zimmermann 2010-07-29 04:51:29 PDT
(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.
Comment 24 Nikolas Zimmermann 2010-07-29 04:52:30 PDT
LayoutTests/svg/custom/recursive-pattern.svg contained tabs, already fixed. (style bot doesn't complain as it's a test though)
Comment 25 Dirk Schulze 2010-07-29 06:11:24 PDT
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 :-(
Comment 26 Nikolas Zimmermann 2010-07-29 06:29:27 PDT
(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...
Comment 27 Nikolas Zimmermann 2010-07-29 06:51:39 PDT
Committed r64275: <http://trac.webkit.org/changeset/64275>
Comment 28 Nikolas Zimmermann 2010-07-29 07:13:50 PDT
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.
Comment 29 WebKit Review Bot 2010-07-29 07:15:40 PDT
http://trac.webkit.org/changeset/64275 might have broken Qt Linux Release
Comment 30 Nikolas Zimmermann 2010-07-29 08:08:48 PDT
Updated Qt results in r64279.