[webkit-reviews] review denied: [Bug 11274] Implement getIntersectionList(), getEnclosureList(), checkIntersection() and checkEnclosure() in SVGSVGElement : [Attachment 97732] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied 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 97732: Patch
https://bugs.webkit.org/attachment.cgi?id=97732&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=97732&action=review

r- for now:

> Source/WebCore/svg/SVGSVGElement.cpp:439
> +    for (Node* node = traverseNextNode(referenceElement ? referenceElement :
this); node; node = node->traverseNextNode(referenceElement ? referenceElement
: this)) {
> +	   if (!node->isSVGElement())
> +	       continue;
> +
> +	   if (checkIntersection(static_cast<SVGElement*>(node), rect))
> +	       nodes.append(node);
> +    }

I think, a while loop would be more readable here:
Node* node = traverseNextNode(referenceElement ? referenceElement : this);
while (node) {
    if (node->isSVGElement() && checkIntersection(...))
	nodes.append(node);
    node = node->traverseNextNode(...);
}

> Source/WebCore/svg/SVGSVGElement.cpp:446
> +    Vector<RefPtr<Node> > nodes;
> +    for (Node* node = traverseNextNode(referenceElement ? referenceElement :
this); node; node = node->traverseNextNode(referenceElement ? referenceElement
: this)) {

Ditto.

Regarding Dirks comment: I think getEnclosure/IntersectionList is fine as-is,
it's a matter of implementingin checkIntersection/checkEnclose properly.

> Source/WebCore/svg/SVGSVGElement.cpp:460
> +    if ((element->isStyledLocatable() ||
element->hasTagName(SVGNames::textTag))
> +	  && !element->hasTagName(SVGNames::gTag) && element->renderer()) {

Early returns will help here.
Why is g excluded?

> Source/WebCore/svg/SVGSVGElement.cpp:462
> +	   AffineTransform trans;

s/trans/transformation/ - don't use abbrevations

> Source/WebCore/svg/SVGSVGElement.cpp:467
> +	   FloatRect bbox = element->renderer()->strokeBoundingBox();

s/bbox/boundingBoxIncludingStroke/.

Are you sure this one is correct? What about a <clip-path> applied to a rect,
which clips some parts away (eg. using a circle).
strokeBoundingBox() ignores resources, but repaintRectInLocalCoordinates() does
not.
Dirk, what do you think?

    // Cache smallest possible repaint rectangle
    m_repaintBoundingBox = m_strokeAndMarkerBoundingBox;
    SVGRenderSupport::intersectRepaintRectWithResources(this,
m_repaintBoundingBox);
..
This "intersect..WithResources" is only done for the repaint bounding box (this
example is from RenderSVGPath).

> Source/WebCore/svg/SVGSVGElement.cpp:477
> +    if ((element->isStyledLocatable() ||
element->hasTagName(SVGNames::textTag))

Same comments as abobe.
I think it should be possible to share coe between those both, at least the if
conditions, and the bbox/trans fetching can be shared in static inline helper
functions.

> LayoutTests/ChangeLog:5
> +	   Add test for:

As Dirk said, this needs lots more tests:
* nested <svg> tags (as Dirk asked for)
* resources applied to elements (eg. clip-path with <circle> on <rect> and
getIntersectionList/getEncloseList on a part that's normaly visible on the
<rect>, but not on screen as it got clipped away, do these need to take
resources into account? If yes or no, write a test and compare to Opera :-)