[webkit-reviews] review denied: [Bug 11274] Implement getIntersectionList(), getEnclosureList(), checkIntersection() and checkEnclosure() in SVGSVGElement : [Attachment 97907] 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 97907: Patch
https://bugs.webkit.org/attachment.cgi?id=97907&action=review

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

Next rounds of comments:

> Source/WebCore/svg/SVGSVGElement.cpp:440
> +    Vector<RefPtr<Node> > nodes;
> +    Node* node = traverseNextNode(referenceElement ? referenceElement :
this);
> +    while (node) {
> +	   if (node->isSVGElement() &&
checkIntersection(static_cast<SVGElement*>(node), rect))
> +	       nodes.append(node);
> +	   node = node->traverseNextNode(referenceElement ? referenceElement :
this);
> +    }
> +    return StaticNodeList::adopt(nodes);

This code duplication is bad. How about:

enum CollectIntersectionOrEnclosure {
    CollectIntersectionList,
    CollectEnclosureList
};

PassRefPtr<NodeList> SVGSVGElement::collectIntersectionOrEnclosureList(const
FloatRect& rect, SVGElement* referenceElement, CollectIntersectionOrEnclosure
collect) const
{
    Vector<RefPtr<Node> > nodes;
    Node* node = traverseNextNode(referenceElement ? referenceElement : this);
    while (node) {
	if (node->isSVGElement()) { 
	    if (collect == CollectIntersectionList)
		if (checkIntersection(static_cast<SVGElement*>(node), rect))
		    nodes.append(node);
	    } else {
		if (checkEnclosure(static_cast<SVGElement*>(node), rect))
		    nodes.append(node);
	    }
	}
	
	node = node->traverseNextNode(referenceElement ? referenceElement :
this);
    }
    return StaticNodeList::adopt(nodes);
}

PassRefPtr<NodeList> SVGSVGElement::getEnclosureList(const FloatRect& rect,
SVGElement* referenceElement) const
{
    return collectIntersectionOrEnclosureList(rect, referenceElement,
CollectEnclosureList);
}

The enum would go in the header in the private: section, just like
collectIntersectionOrEnclosureList.

> Source/WebCore/svg/SVGSVGElement.cpp:455
> +static AffineTransform getElementCTM(SVGElement* element)

Returning AffineTransform is bad, it has to copy all the values, rather pass in
a AffineTransform& reference here.

>> Source/WebCore/svg/SVGSVGElement.cpp:471
>> +		r2.inflateY(.1);
> 
> what's that? Why do you increase the size of the rect? If it has zero sizes,
why don't we just return false? 0.1 is very unclear. Take a viewBox of "0 0 0.1
0.1" your inflation would cover the complete area. MaybeI misunderstand what
you are doing here. A comment would help anyway.

I have the same concerncs.
Isn't there any method available that does what you want here?

> Source/WebCore/svg/SVGSVGElement.cpp:495
> +	   AffineTransform transformation = getElementCTM(element);
> +	   FloatRect clippedBoundingBox =
element->renderer()->objectBoundingBox();
> +	  
SVGRenderSupport::intersectRepaintRectWithResources(element->renderer(),
clippedBoundingBox);
> +	   return intersectsAllowingEmpty(rect,
transformation.mapRect(clippedBoundingBox));

These operations indicate this belongs in the render tree. The render tree also
caches CTMs at least for shapes.
Can you look into moving these into RenderSVGRoot as public API? And use that
from here?

> Source/WebCore/svg/SVGSVGElement.cpp:502
> +    if (RenderObject* renderer = element->renderer()) {

Ditto.