[webkit-reviews] review denied: [Bug 12571] <clipPath> doesn't correctly handle <text> elements : [Attachment 51191] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Mar 21 11:55:51 PDT 2010


Nikolas Zimmermann <zimmermann at kde.org> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 12571: <clipPath> doesn't correctly handle <text> elements
https://bugs.webkit.org/show_bug.cgi?id=12571

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
Hi Dirk,

some random notes:

1) can you comment why	RenderSVGResourceClipper::invalidate now only takes()
the value out of the HashMap instead of actually deleting it?
Who's responsible to delete it? I see it's reused in calculateClipper() (which
needs a rename to sth. more meaningful btw.. :-) - though what happens if you
are invalidating a resource, that is never reused again? Won't your new code
just leave the ClipperData object undeleted?

2) In RenderSVGResourceClipper::fastClipper (which should be renamed to sth.
like pathOnlyClipping, instead of fast..) can you introduce local variable for
RenderSVGSTyle & co instead of callling style()->svgStyle() several times...

3) I am slightly worried about the approach you've chosen to implement the
non-fast-clipping mode, especially the RenderSttyle modifications... how about
creating a copy of the RenderSTyle that can be safely mutated and stored inside
the RenderSVGResourceClipper?

4) ClipperData doesn't need an empty default ctor

The general approach is _great_ though, glad you finally tried to fix that
issue!
r- until all issues are resolved.


More information about the webkit-reviews mailing list