[SVG] Refactor getIntersectionList() and getEnclosureList()

Source/core/svg/SVGSVGElement.cpp

Index: Source/core/svg/SVGSVGElement.cpp
diff --git a/Source/core/svg/SVGSVGElement.cpp b/Source/core/svg/SVGSVGElement.cpp
index af5a33658f00b7a24f4ceec4348df4e23b319193..264b87839fc221f12573ea42e41faa684652f807 100644
--- a/Source/core/svg/SVGSVGElement.cpp
+++ b/Source/core/svg/SVGSVGElement.cpp
@@ -329,49 +329,121 @@ void SVGSVGElement::svgAttributeChanged(const QualifiedName& attrName)
     SVGGraphicsElement::svgAttributeChanged(attrName);
 }
 
-PassRefPtr<NodeList> SVGSVGElement::collectIntersectionOrEnclosureList(const FloatRect& rect, SVGElement* referenceElement, CollectIntersectionOrEnclosure collect) const
+// FloatRect::intersects does not consider horizontal or vertical lines (because of isEmpty()).
+// So special-case handling of such lines.
+static bool intersectsAllowingEmpty(const FloatRect& r, const FloatRect& other)

[Stephen Chennney, 2014/03/03 13:00:22]

Either this is much simpler to implement, or I'm not understanding the purpose
of the method (which means it's badly named).

The two rects intersect if max(r.x(), other.x()) > min(r.x() + r.height, other.x
+ other.height) and similar for y. That is, if their intersection is non-empty
(also called a separating lines check because you are finding out if there is a
line that separates the two rectangles in either dimension). Much cheaper to
compute that all this "contains" checking.

[f(malita), 2014/03/03 14:12:18]

TBH I haven't looked beyond the comments when moving this from
RenderSVGModelObject. As fs pointed out the current impl has functional issues
also, so it sure is a good candidate for refactoring. Will do.

+{
+    if (r.isEmpty() && other.isEmpty())

[fs, 2014/03/03 10:44:32]

This function had me wondering...

Since isEmpty() is "width <= 0 || height <= 0", then intersecting, say:

<0, 5, 10, 0> with
<5, 0, 0, 10>

would return false, although it should return true - yes?

Maybe it would just be easier to open-code FloatRect::intersects w/ the
!isEmpty()-parts of the predicate removed (and potentially add checks for
width/height < 0 - if that's even possible to achieve... [for the RenderObject
side])

+        return false;
+    if (r.isEmpty() && !other.isEmpty()) {
+        return (other.contains(r.x(), r.y()) && !other.contains(r.maxX(), r.maxY()))
+            || (!other.contains(r.x(), r.y()) && other.contains(r.maxX(), r.maxY()));
+    }
+    if (other.isEmpty() && !r.isEmpty())
+        return intersectsAllowingEmpty(other, r);

[fs, 2014/03/03 10:44:32]

Might make sense to just make the code in the above if-block (yet) a(nother)
helper-function? (But it depends on how the above is resolved of course.)

+    return r.intersects(other);
+}
+
+// One of the element types that can cause graphics to be drawn onto the target canvas.
+// Specifically: circle, ellipse, image, line, path, polygon, polyline, rect, text and use.
+static bool isIntersectionOrEnclosureTarget(RenderObject* renderer)
+{
+    return renderer->isSVGShape()
+        || renderer->isSVGText()
+        || renderer->isSVGImage()
+        || renderer->node()->hasTagName(SVGNames::useTag);
+}
+
+bool SVGSVGElement::checkIntersectionOrEnclosure(const SVGElement& element, const FloatRect& rect,
+    CheckIntersectionOrEnclosure mode) const
+{
+    RenderObject* renderer = element.renderer();
+    ASSERT(!renderer || renderer->style());
+    if (!renderer || renderer->style()->pointerEvents() == PE_NONE)
+        return false;
+
+    if (!isIntersectionOrEnclosureTarget(renderer))
+        return false;
+
+    ASSERT(WebCore::isSVGGraphicsElement(element));
+    AffineTransform ctm = toSVGGraphicsElement(element).computeCTM(AncestorScope, DisallowStyleUpdate, this);
+    FloatRect mappedRepaintRect = ctm.mapRect(renderer->repaintRectInLocalCoordinates());
+
+    bool result = false;
+    switch (mode) {
+    case CheckIntersection:
+        result = intersectsAllowingEmpty(rect, mappedRepaintRect);
+        break;
+    case CheckEnclosure:
+        result = rect.contains(mappedRepaintRect);
+        break;
+    default:
+        ASSERT_NOT_REACHED();
+        break;
+    }
+
+    return result;
+}
+
+PassRefPtr<NodeList> SVGSVGElement::collectIntersectionOrEnclosureList(const FloatRect& rect,
+    SVGElement* referenceElement, CheckIntersectionOrEnclosure mode) const
 {
     Vector<RefPtr<Node> > nodes;
-    Element* element = ElementTraversal::next(*(referenceElement ? referenceElement : this));
-    while (element) {
-        if (element->isSVGElement()) {
-            SVGElement* svgElement = toSVGElement(element);
-            if (collect == CollectIntersectionList) {
-                if (RenderSVGModelObject::checkIntersection(svgElement->renderer(), rect))
-                    nodes.append(element);
-            } else {
-                if (RenderSVGModelObject::checkEnclosure(svgElement->renderer(), rect))
-                    nodes.append(element);
-            }
+
+    const SVGElement* root = this;
+    if (referenceElement) {

[fs, 2014/03/03 10:44:32]

Some test(s) for this case would be nice - but I'm not going to force you to
write them. =)

[f(malita), 2014/03/03 14:12:18]

I'll add some willingly :)

+        // Only the common subtree needs to be traversed.
+        if (contains(referenceElement)) {
+            root = referenceElement;
+        } else if (!isDescendantOf(referenceElement)) {
+            // No common subtree: the spec is not clear about this case - should we bend over backwards

[fs, 2014/03/03 10:44:32]

I think this is in line with the spec wording, so seems fine to me.

[f(malita), 2014/03/03 14:12:18]

Awesome, I'll remove the comment then.

+            // to find a common ancestor and map everything in its space?
+            return StaticNodeList::adopt(nodes);
         }
+    }
 
-        element = ElementTraversal::next(*element, referenceElement ? referenceElement : this);
+    for (Element* element = ElementTraversal::firstWithin(*root); element;
+        element = ElementTraversal::next(*element, root)) {
+
+        if (!WebCore::isSVGGraphicsElement(*element))
+            continue;
+
+        SVGElement* svgElement = toSVGElement(element);
+        if (checkIntersectionOrEnclosure(*svgElement, rect, mode))
+            nodes.append(element);
     }
+
     return StaticNodeList::adopt(nodes);
 }
 
-PassRefPtr<NodeList> SVGSVGElement::getIntersectionList(PassRefPtr<SVGRectTearOff> passRect, SVGElement* referenceElement) const
+PassRefPtr<NodeList> SVGSVGElement::getIntersectionList(PassRefPtr<SVGRectTearOff> rect, SVGElement* referenceElement) const
 {
-    RefPtr<SVGRectTearOff> rect = passRect;
-    return collectIntersectionOrEnclosureList(rect->target()->value(), referenceElement, CollectIntersectionList);
+    document().updateLayoutIgnorePendingStylesheets();
+
+    return collectIntersectionOrEnclosureList(rect->target()->value(), referenceElement, CheckIntersection);
 }
 
-PassRefPtr<NodeList> SVGSVGElement::getEnclosureList(PassRefPtr<SVGRectTearOff> passRect, SVGElement* referenceElement) const
+PassRefPtr<NodeList> SVGSVGElement::getEnclosureList(PassRefPtr<SVGRectTearOff> rect, SVGElement* referenceElement) const
 {
-    RefPtr<SVGRectTearOff> rect = passRect;
-    return collectIntersectionOrEnclosureList(rect->target()->value(), referenceElement, CollectEnclosureList);
+    document().updateLayoutIgnorePendingStylesheets();
+
+    return collectIntersectionOrEnclosureList(rect->target()->value(), referenceElement, CheckEnclosure);
 }
 
-bool SVGSVGElement::checkIntersection(SVGElement* element, PassRefPtr<SVGRectTearOff> passRect) const
+bool SVGSVGElement::checkIntersection(SVGElement* element, PassRefPtr<SVGRectTearOff> rect) const
 {
-    RefPtr<SVGRectTearOff> rect = passRect;
-    return RenderSVGModelObject::checkIntersection(element->renderer(), rect->target()->value());
+    ASSERT(element);
+    document().updateLayoutIgnorePendingStylesheets();
+
+    return checkIntersectionOrEnclosure(*element, rect->target()->value(), CheckIntersection);
 }
 
-bool SVGSVGElement::checkEnclosure(SVGElement* element, PassRefPtr<SVGRectTearOff> passRect) const
+bool SVGSVGElement::checkEnclosure(SVGElement* element, PassRefPtr<SVGRectTearOff> rect) const
 {
-    RefPtr<SVGRectTearOff> rect = passRect;
-    return RenderSVGModelObject::checkEnclosure(element->renderer(), rect->target()->value());
+    ASSERT(element);
+    document().updateLayoutIgnorePendingStylesheets();
+
+    return checkIntersectionOrEnclosure(*element, rect->target()->value(), CheckEnclosure);
 }
 
 void SVGSVGElement::deselectAll()