Bug 11274 – Implement getIntersectionList(), getEnclosureList(), checkIntersection() and checkEnclosure() in SVGSVGElement

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Rob Buis]

Implement the functionlity required by the methods mentioned above.

[Rob Buis]

Eric says, fun weekend hack, but low prio.

[Eric Seidel (no email)]

Created attachment 16580 [details]
basic test case

This test case has not been tested as no other browser (on my machine) seems to implement these.

[Eric Seidel (no email)]

This still looks like fun. :)  Sadly, I've never seen an SVG which actually uses these.  Perhaps I'll get to it over the holidays.

[Jeff Schiller]

Created attachment 32078 [details]
Simpler test case for the get methods

I'm adding the same test case I attached to https://bugzilla.mozilla.org/show_bug.cgi?id=501421

Let's see which browser implements it first :P

[Jeff Schiller]

FYI - Opera implements these methods.

Maybe this would be a good intro to get me hacking on Webkit?  Is there an IRC channel where seidel lurks so I can pester him on a weekend? :)

[Jeff Schiller]

FYI - I want this so I can use it in SVG-edit:  http://svg-edit.googlecode.com/

Looks like I'll have to brute force it in JS for now...

[Rob Buis]

Created attachment 97732 [details]
Patch

[WebKit Review Bot]

Comment on attachment 97732 [details]
Patch

Attachment 97732 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8910375

New failing tests:
svg/W3C-SVG-1.1-SE/struct-dom-11-f.svg

[WebKit Review Bot]

Created attachment 97739 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dirk Schulze]

The reasons why I stopped working on that are the unanswered questions. Should intersection(encloser)List just look for childs of the current svg-element? What with nested SVG's? 

<svg id="root">
<rect x="0" y="0" width="100" height="100" id="r1"/>
<svg id="askedForIntersection">
<rect x="0" y="0" width="100" height="100" id="r2"/>
<svg id="nestedSVG">
<rect x="0" y="0" width="100" height="100" id="r3"/>
</svg>
</svg>
</svg>

If you ask "askedForIntersection" for the intersectionLst, which rects are included? This needs definitely a lot more test cases and checking in other browsers.

[Nikolas Zimmermann]

Comment on attachment 97732 [details]
Patch

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 :-)

[Dirk Schulze]

(In reply to comment #11)
> (From update of attachment 97732 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97732&action=review
> 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).

http://www.w3.org/TR/SVG11/struct.html#__svg__SVGSVGElement__getIntersectionList

"Returns the list of graphics elements whose rendered content intersects the supplied rectangle. Each candidate graphics element is to be considered a match only if the same graphics element can be a target of pointer events as defined in ‘pointer-events’ processing."

My understanding is: resources like clipper should be taken into account.

But the interesting point is 'pointer events'. If the element has no pointer event, it shouldn't be in the list, right?

[Dirk Schulze]

The mozilla bug has some interesting comments as well: https://bugzilla.mozilla.org/show_bug.cgi?id=501421

[Rob Buis]

Created attachment 97907 [details]
Patch

[WebKit Review Bot]

Comment on attachment 97907 [details]
Patch

Attachment 97907 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8910749

New failing tests:
svg/W3C-SVG-1.1-SE/struct-dom-11-f.svg

[WebKit Review Bot]

Created attachment 97910 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dirk Schulze]

Comment on attachment 97907 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97907&action=review

I'd like to leave the review to Niko, just some comments.

> Source/WebCore/svg/SVGSVGElement.cpp:464
> +static bool intersectsAllowingEmpty(const FloatRect& r, const FloatRect& other)

please use rect or intersectionRect but not r

> 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.

> Source/WebCore/svg/SVGSVGElement.cpp:497
> +    if (RenderObject* renderer = element->renderer()) {
> +        if (!renderer || renderer->style()->pointerEvents() == PE_NONE)
> +            return false;
> +        if (!(renderer->isSVGPath() || renderer->isSVGText() || renderer->isSVGImage() || renderer->isSVGShadowTreeRootContainer()))
> +            return false;
> +        AffineTransform transformation = getElementCTM(element);
> +        FloatRect clippedBoundingBox = element->renderer()->objectBoundingBox();
> +        SVGRenderSupport::intersectRepaintRectWithResources(element->renderer(), clippedBoundingBox);
> +        return intersectsAllowingEmpty(rect, transformation.mapRect(clippedBoundingBox));
> +    }
> +    return false;

Hm. IMHO it would be better to write:
RenderObject* renderer = element->renderer();
if (!renderer)
    return false;

than indent everything

> Source/WebCore/svg/SVGSVGElement.cpp:512
> +    if (RenderObject* renderer = element->renderer()) {
> +        if (!renderer || renderer->style()->pointerEvents() == PE_NONE)
> +            return false;
> +        if (!(renderer->isSVGPath() || renderer->isSVGText() || renderer->isSVGImage() || renderer->isSVGShadowTreeRootContainer()))
> +            return false;
> +        AffineTransform transformation = getElementCTM(element);
> +        FloatRect clippedBoundingBox = element->renderer()->objectBoundingBox();
> +        SVGRenderSupport::intersectRepaintRectWithResources(element->renderer(), clippedBoundingBox);
> +        return rect.contains(transformation.mapRect(clippedBoundingBox));
> +    }
> +    return false;

Ditto

[Dirk Schulze]

(In reply to comment #17)
> (From update of attachment 97907 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97907&action=review
> 
> I'd like to leave the review to Niko, just some comments.
> 
> > Source/WebCore/svg/SVGSVGElement.cpp:464
> > +static bool intersectsAllowingEmpty(const FloatRect& r, const FloatRect& other)
> 
> please use rect or intersectionRect but not r
> 
> > 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.
> 
> > Source/WebCore/svg/SVGSVGElement.cpp:497
> > +    if (RenderObject* renderer = element->renderer()) {
> > +        if (!renderer || renderer->style()->pointerEvents() == PE_NONE)
> > +            return false;
> > +        if (!(renderer->isSVGPath() || renderer->isSVGText() || renderer->isSVGImage() || renderer->isSVGShadowTreeRootContainer()))
> > +            return false;
> > +        AffineTransform transformation = getElementCTM(element);
> > +        FloatRect clippedBoundingBox = element->renderer()->objectBoundingBox();
> > +        SVGRenderSupport::intersectRepaintRectWithResources(element->renderer(), clippedBoundingBox);
> > +        return intersectsAllowingEmpty(rect, transformation.mapRect(clippedBoundingBox));
> > +    }
> > +    return false;
> 
> Hm. IMHO it would be better to write:
> RenderObject* renderer = element->renderer();
> if (!renderer)
>     return false;
> 
> than indent everything
> 
> > Source/WebCore/svg/SVGSVGElement.cpp:512
> > +    if (RenderObject* renderer = element->renderer()) {
> > +        if (!renderer || renderer->style()->pointerEvents() == PE_NONE)
> > +            return false;
> > +        if (!(renderer->isSVGPath() || renderer->isSVGText() || renderer->isSVGImage() || renderer->isSVGShadowTreeRootContainer()))
> > +            return false;
> > +        AffineTransform transformation = getElementCTM(element);
> > +        FloatRect clippedBoundingBox = element->renderer()->objectBoundingBox();
> > +        SVGRenderSupport::intersectRepaintRectWithResources(element->renderer(), clippedBoundingBox);
> > +        return rect.contains(transformation.mapRect(clippedBoundingBox));
> > +    }
> > +    return false;
> 
> Ditto

[Dirk Schulze]

Comment on attachment 97907 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97907&action=review

> Source/WebCore/svg/SVGSVGElement.cpp:490
> +        if (!(renderer->isSVGPath() || renderer->isSVGText() || renderer->isSVGImage() || renderer->isSVGShadowTreeRootContainer()))

Can you use de morgan here?

!renderer->isSVGPath() && !renderer->isSVGText() && !renderer->isSVGImage() && !renderer->isSVGShadowTreeRootContainer()

Maybe easier to understand the exclusion

[Nikolas Zimmermann]

Comment on attachment 97907 [details]
Patch

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.

[Rob Buis]

Created attachment 98655 [details]
Patch

[WebKit Review Bot]

Comment on attachment 98655 [details]
Patch

Attachment 98655 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8946176

New failing tests:
svg/W3C-SVG-1.1-SE/struct-dom-11-f.svg

[WebKit Review Bot]

Created attachment 98656 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dirk Schulze]

Comment on attachment 98655 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98655&action=review

Looks good. Still a question and a comment.

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:490
> +    if (r.isEmpty() && other.isEmpty())
> +        return false;

Just checked isEmpty()

bool isEmpty() const { return m_width <= 0 || m_height <= 0; }

What do other browsers if intersection rect has negative width or height? Is it maybe better to omit this return?

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:501
> +// 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â.

Encoding problem

[Rob Buis]

Created attachment 101492 [details]
Patch

[Rob Buis]

Hi Dirk,

(In reply to comment #24)
> (From update of attachment 98655 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98655&action=review
> 
> Looks good. Still a question and a comment.
> 
> > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:490
> > +    if (r.isEmpty() && other.isEmpty())
> > +        return false;
> 
> Just checked isEmpty()
> 
> bool isEmpty() const { return m_width <= 0 || m_height <= 0; }
> 
> What do other browsers if intersection rect has negative width or height? Is it maybe better to omit this return?

I think it is better. I am mostly interested in zero width/height though, not negative. I want to check lines against rects basically, since to me it seems they can intersect just fine. When both rects are really lines (one of width/height is zero, but not the other) things get a bit undefined. I propose to keep returning false until some svg out there actually tests/wants this. Could even leave a TODO/FIXME.

> > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:501
> > +// 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â.
> 
> Encoding problem

Fixed.
Cheers,

Rob.

[WebKit Review Bot]

Comment on attachment 101492 [details]
Patch

Attachment 101492 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9191353

New failing tests:
svg/W3C-SVG-1.1-SE/struct-dom-11-f.svg

[Nikolas Zimmermann]

Comment on attachment 101492 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101492&action=review

Some comments left:

> LayoutTests/ChangeLog:3
> +        Add tests for:

This belongs below the Reviewed by line.

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:480
> +         transform = static_cast<SVGTextElement*>(element)->getCTM();

These getCTM() calls can be avoided. This information is already stored within the render tree.
You'll need to find a way to access them.

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:484
> +    transform = static_cast<SVGStyledLocatableElement*>(element)->getCTM();

This needs an assertion, that casting to that object is safe.

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:516
> +    AffineTransform ctm;
> +    getElementCTM(static_cast<SVGElement*>(renderer->node()), ctm);
> +    FloatRect clippedBoundingBox = renderer->objectBoundingBox();
> +    SVGRenderSupport::intersectRepaintRectWithResources(renderer, clippedBoundingBox);

See the comment above regarding getElementCTM.
Even the repaintRectInLocalCoords is stored somewhere.

[Dirk Schulze]

Comment on attachment 101492 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101492&action=review

Some more comments.

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:489
> +static bool intersectsAllowingEmpty(const FloatRect& r, const FloatRect& other)

Can you add a check for zero sizes to FloatSize and FloatRect? IIRC we had this problem more than once. Please use rect, not r. Can you make this function inline?

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:502
> +// 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 isGraphicsElement(RenderObject* renderer)

I think the comments should be in the functions, not before. Don't think that we have a specific style rules.

Can you make it inline?

[Rob Buis]

Created attachment 101502 [details]
Patch

[Rob Buis]

(In reply to comment #29)
Hi Dirk,

> (From update of attachment 101492 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101492&action=review
> 
> Some more comments.
> 
> > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:489
> > +static bool intersectsAllowingEmpty(const FloatRect& r, const FloatRect& other)
> 
> Can you add a check for zero sizes to FloatSize and FloatRect? IIRC we had this problem more than once. Please use rect, not r. Can you make this function inline?

That may be something for another patch. Also I don't get how isZero() should work for FloatSize and FloatRect? Other stuff is fixed.

> > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:502
> > +// 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 isGraphicsElement(RenderObject* renderer)
> 
> I think the comments should be in the functions, not before. Don't think that we have a specific style rules.
> 
> Can you make it inline?

Fixed.
Cheers,

Rob.

[Rob Buis]

Hi Niko,

(In reply to comment #28)
> (From update of attachment 101492 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101492&action=review
> 
> Some comments left:
> 
> > LayoutTests/ChangeLog:3
> > +        Add tests for:
> 
> This belongs below the Reviewed by line.

Fixed.

> > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:480
> > +         transform = static_cast<SVGTextElement*>(element)->getCTM();
> 
> These getCTM() calls can be avoided. This information is already stored within the render tree.
> You'll need to find a way to access them.

I doubt this, it comes down to SVGLocatable::computeCTM which needs to do a loop to compute the whole CTM, AFAIK this is not cached in the render tree.

> > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:484
> > +    transform = static_cast<SVGStyledLocatableElement*>(element)->getCTM();
> 
> This needs an assertion, that casting to that object is safe.

Fixed.

> > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:516
> > +    AffineTransform ctm;
> > +    getElementCTM(static_cast<SVGElement*>(renderer->node()), ctm);
> > +    FloatRect clippedBoundingBox = renderer->objectBoundingBox();
> > +    SVGRenderSupport::intersectRepaintRectWithResources(renderer, clippedBoundingBox);
> 
> See the comment above regarding getElementCTM.
> Even the repaintRectInLocalCoords is stored somewhere.

Calling that now, fixed.
Cheers,

Rob.

[Nikolas Zimmermann]

(In reply to comment #32) 
> I doubt this, it comes down to SVGLocatable::computeCTM which needs to do a loop to compute the whole CTM, AFAIK this is not cached in the render tree.
Sure it is, it's needed every time we render something :-)
See RenderSVGPath.h:     virtual const AffineTransform& localToParentTransform() const { return m_localTransform; }

RenderSVGPath::layout does
    if (m_needsTransformUpdate) {
        m_localTransform = element->animatedLocalTransform();
...

It only regrabs the transform, if necessary!
You can always call localToParentTransform(), that gives you the same result as getCTM() on the corresponding element.

Exactly the same holds true for RenderSVGContainer.
We've optimized this a year or 2 ago - to avoid repeatedly calling getCTM() on the renderer, but instead cache those values in the renderers and only update them if the SVGNames::transformAttr changes.

[WebKit Review Bot]

Comment on attachment 101502 [details]
Patch

Attachment 101502 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9199434

New failing tests:
svg/W3C-SVG-1.1-SE/struct-dom-11-f.svg

[Dirk Schulze]

Comment on attachment 101502 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101502&action=review

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:516
> +    if (!renderer || renderer->style()->pointerEvents() == PE_NONE)
> +        return false;
> +    if (!isGraphicsElement(renderer))
> +        return false;
> +    AffineTransform ctm;
> +    getElementCTM(static_cast<SVGElement*>(renderer->node()), ctm);
> +    return intersectsAllowingEmpty(rect, ctm.mapRect(renderer->repaintRectInLocalCoordinates()));

Niko is right. And because you return earlier for !isGraphicsElement You can savely cast to a SVG specific renderer.

[Rob Buis]

Created attachment 101896 [details]
Patch

[WebKit Review Bot]

Comment on attachment 101896 [details]
Patch

Attachment 101896 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9251128

New failing tests:
svg/W3C-SVG-1.1-SE/struct-dom-11-f.svg

[Nikolas Zimmermann]

Comment on attachment 101896 [details]
Patch

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?

[Rob Buis]

This got landed in r91850.

[Dirk Schulze]

(In reply to comment #38)
> (From update of attachment 101896 [details])

> I'd move all of this in RenderSVGModelObject, not sure why it should live in RenderSVGRoot?

I looked at RenderSVGModelObject for general clean up. I'm pretty sure that these functions are misplaced there. RenderSVGModelObject is neither the parent class of RenderSVGImage nor of RenderSVGText. So why do we check incoming renderer for these classes? I mean I know why we do that,  but that is an indication that the code should move to RenderSVGSupport - the general SVG related renderer. I'd like to create a followup that moves the code to this position.

[Nikolas Zimmermann]

(In reply to comment #38)
> > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:487
> > +            AffineTransform temp = currentElement->renderer()->localToParentTransform();
> > +            transform = temp.multiply(transform);
> 
> Avoid this temporary, it's heavy.
*grml* that caused bug 77179 :(