[webkit-reviews] review granted: [Bug 11274] Implement getIntersectionList(), getEnclosureList(), checkIntersection() and checkEnclosure() in SVGSVGElement : [Attachment 101896] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jul 27 00:43:58 PDT 2011
Nikolas Zimmermann <zimmermann at kde.org> has granted Rob Buis
<rwlbuis at gmail.com>'s request for review:
Bug 11274: Implement getIntersectionList(), getEnclosureList(),
checkIntersection() and checkEnclosure() in SVGSVGElement
https://bugs.webkit.org/show_bug.cgi?id=11274
Attachment 101896: Patch
https://bugs.webkit.org/attachment.cgi?id=101896&action=review
------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=101896&action=review
Looks great, r=me. Please fix following comments before landing:
> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:478
> + element->document()->updateLayoutIgnorePendingStylesheets();
I'd like an ASSERT(element) above here.
> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:480
> + SVGElement* stopAtElement =
SVGLocatable::nearestViewportElement(element);
Is this guaranteed to be non-null? If so add assertions please.
> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:482
> + Node* current = const_cast<SVGElement*>(element);
const_cast is superflous, you don't need to cast at all here?
> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:487
> + AffineTransform temp =
currentElement->renderer()->localToParentTransform();
> + transform = temp.multiply(transform);
Avoid this temporary, it's heavy.
> Source/WebCore/rendering/svg/RenderSVGRoot.h:51
> + static bool checkIntersection(RenderObject*, const FloatRect&);
> + static bool checkEnclosure(RenderObject*, const FloatRect&);
I'd move all of this in RenderSVGModelObject, not sure why it should live in
RenderSVGRoot?
More information about the webkit-reviews
mailing list