Bug 39965 – SVG repaintRect should be empty if content got clipped away

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

SVG repaintRect should be empty if content got clipped away

[Dirk Schulze]

Created attachment 57487 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 57487 [details]
Patch

Hi Dirk, some comments:

WebCore/ChangeLog:10
 +          The reference of the RenderObjects get already applied to the resource
I don't understand this comment.
I guess it's related to the fact that you're creating ClipperData/MaskerData earlier now, but I don't understand why this is needed.

WebCore/rendering/RenderSVGImage.cpp:154
 +          
Please remove the extra spaces.

WebCore/rendering/RenderSVGResourceClipper.cpp:274
 +      SVGClipPathElement* clipElement = static_cast<SVGClipPathElement*>(node());
That's not needed, you can leave it as it was before. You're not using 'clipElement' anywhere but in the if some lines below.

WebCore/rendering/SVGRenderSupport.cpp:306
 +              repaintRect.intersect(masker->resourceBoundingBox(const_cast<RenderObject*>(object)));
No need for the const_cast again, you already have a local variable 'renderer' above.

Okay, not much to fixup, but r-, as long as I don't understand why you have to early-create ClipperData/MaskerData etc I don't see the immediate need to do so, especially not if (selfNeedsLayout()) is true :-)
Please enlighten me.

Cheers,
Niko

[Dirk Schulze]

(In reply to comment #2)
> Please enlighten me.
Sure :-) Imagine a RenderPath object. The object has a clipper as resource. This clipper clips away the whole content of the object. Therefore we don't run through the rendering process in RenderPath::paint, because the repaintRect is zero. No problem up to here.

Now we change the clipPath of the clipper. The resource (the clipper) tells its references to relayout. But we add the object to the resource during the paint phase in applyResource(). But we didn't enter this phase for the object, because the repaintRect was empty. Therefore the resource don't know of the referencing object, our RenderPath. This means that the RenderPath doesn't get relayouted and still don't get painted.

A complicated scenario, but we already have tests in DRT for this.

[Nikolas Zimmermann]

(In reply to comment #3)
> (In reply to comment #2)
> > Please enlighten me.
> Sure :-) Imagine a RenderPath object. The object has a clipper as resource. This clipper clips away the whole content of the object. Therefore we don't run through the rendering process in RenderPath::paint, because the repaintRect is zero. No problem up to here.
> 
> Now we change the clipPath of the clipper. The resource (the clipper) tells its references to relayout. But we add the object to the resource during the paint phase in applyResource(). But we didn't enter this phase for the object, because the repaintRect was empty. Therefore the resource don't know of the referencing object, our RenderPath. This means that the RenderPath doesn't get relayouted and still don't get painted.
> 
> A complicated scenario, but we already have tests in DRT for this.

Ah, good explaination. Can you somehow move that into a comment, explaining why you're adding this Masker/ClipperData/.. at this point? That would be perfect.

[Dirk Schulze]

Created attachment 57531 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 57531 [details]
Patch

r=me, though the ChangeLog looks ugly and the comments I asked for are missing, please fix it before landing.

Can you avoid the linebreaks in the ChangeLog, there is no 80 char limit there.

WebCore/ChangeLog:9
 +          got clipped away.
Here.

WebCore/ChangeLog:11
 +          during the layout phase now.
Here.

The whole sentence " The reference of the RenderObjects need to get applied to MaskerData/ClipperData during the layout phase now." is still confusing.
How about "The MaskerData/ClipperData <-> RenderObject mapping is now set up during the layout phase, to be able to track...."

Same for "With an empty repaintRect, the painting phase is never called" -> "With an empty repaintRect, the paint() call is never entered."

"and therefore the object don't get" -> "and therefore the object doesn't get"

"This can cause problems, if the resource get changed by animations or scripts" -> "This is problematic, if the resource is dynamically changed by animations/scripts".

"and therefor won't get drawn." -> "and therefore won't get drawn".

etc..
Please rework the ChangeLog before landing :-)

WebCore/rendering/RenderSVGResourceClipper.cpp:271
 +      if (!m_clipper.contains(object))
Here the comment is missing, why the m_clipper.set() calls is done here.

WebCore/rendering/RenderSVGResourceMasker.cpp:215
 +      if (!m_masker.contains(object))
Ditto.

[Dirk Schulze]

Landed in http://trac.webkit.org/changeset/60541 . Closing bug.