[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