[webkit-reviews] review granted: [Bug 39965] SVG repaintRect should be empty if content got clipped away : [Attachment 57531] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has granted Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 39965: SVG repaintRect should be empty if content got clipped away
https://bugs.webkit.org/show_bug.cgi?id=39965

Attachment 57531: Patch
https://bugs.webkit.org/attachment.cgi?id=57531&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
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.