Bug 39965 - SVG repaintRect should be empty if content got clipped away
Summary: SVG repaintRect should be empty if content got clipped away
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 40043
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-31 11:19 PDT by Dirk Schulze
Modified: 2010-06-02 01:13 PDT (History)
2 users (show)

See Also:


Attachments
Patch (28.91 KB, patch)
2010-05-31 11:47 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (29.12 KB, patch)
2010-06-01 05:32 PDT, Dirk Schulze
zimmermann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2010-05-31 11:19:02 PDT
SVG repaintRect should be empty if content got clipped away
Comment 1 Dirk Schulze 2010-05-31 11:47:07 PDT
Created attachment 57487 [details]
Patch
Comment 2 Nikolas Zimmermann 2010-06-01 03:25:05 PDT
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
Comment 3 Dirk Schulze 2010-06-01 03:59:33 PDT
(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.
Comment 4 Nikolas Zimmermann 2010-06-01 04:29:40 PDT
(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.
Comment 5 Dirk Schulze 2010-06-01 05:32:27 PDT
Created attachment 57531 [details]
Patch
Comment 6 Nikolas Zimmermann 2010-06-01 07:10:24 PDT
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.
Comment 7 Dirk Schulze 2010-06-02 01:13:20 PDT
Landed in http://trac.webkit.org/changeset/60541 . Closing bug.