[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