Bug 11274 - Implement getIntersectionList(), getEnclosureList(), checkIntersection() and checkEnclosure() in SVGSVGElement
Summary: Implement getIntersectionList(), getEnclosureList(), checkIntersection() and ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P3 Normal
Assignee: Rob Buis
URL: http://www.w3.org/TR/SVG/struct.html#...
Keywords:
Depends on: 71184 77179
Blocks:
  Show dependency treegraph
 
Reported: 2006-10-13 02:26 PDT by Rob Buis
Modified: 2012-02-01 04:16 PST (History)
7 users (show)

See Also:


Attachments
basic test case (3.05 KB, image/svg+xml)
2007-10-07 16:52 PDT, Eric Seidel (no email)
no flags Details
Simpler test case for the get methods (1.12 KB, image/svg+xml)
2009-06-30 11:21 PDT, Jeff Schiller
no flags Details
Patch (39.09 KB, patch)
2011-06-19 19:35 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.33 MB, application/zip)
2011-06-19 20:53 PDT, WebKit Review Bot
no flags Details
Patch (43.83 KB, patch)
2011-06-20 18:47 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.23 MB, application/zip)
2011-06-20 19:12 PDT, WebKit Review Bot
no flags Details
Patch (46.57 KB, patch)
2011-06-26 19:40 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.41 MB, application/zip)
2011-06-26 20:10 PDT, WebKit Review Bot
no flags Details
Patch (41.41 KB, patch)
2011-07-20 12:03 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (41.40 KB, patch)
2011-07-20 13:34 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (41.54 KB, patch)
2011-07-25 12:50 PDT, Rob Buis
zimmermann: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2006-10-13 02:26:30 PDT
Implement the functionlity required by the methods mentioned above.
Comment 1 Rob Buis 2007-06-12 11:14:13 PDT
Eric says, fun weekend hack, but low prio.
Comment 2 Eric Seidel (no email) 2007-10-07 16:52:02 PDT
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.
Comment 3 Eric Seidel (no email) 2007-12-19 03:20:06 PST
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.
Comment 4 Jeff Schiller 2009-06-30 11:21:01 PDT
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
Comment 5 Jeff Schiller 2009-06-30 11:22:01 PDT
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? :)
Comment 6 Jeff Schiller 2009-06-30 11:24:37 PDT
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...
Comment 7 Rob Buis 2011-06-19 19:35:58 PDT
Created attachment 97732 [details]
Patch
Comment 8 WebKit Review Bot 2011-06-19 20:53:17 PDT
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
Comment 9 WebKit Review Bot 2011-06-19 20:53:22 PDT
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
Comment 10 Dirk Schulze 2011-06-19 23:47:56 PDT
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.
Comment 11 Nikolas Zimmermann 2011-06-20 00:45:25 PDT
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 :-)
Comment 12 Dirk Schulze 2011-06-20 00:54:59 PDT
(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?
Comment 13 Dirk Schulze 2011-06-20 01:01:55 PDT
The mozilla bug has some interesting comments as well: https://bugzilla.mozilla.org/show_bug.cgi?id=501421
Comment 14 Rob Buis 2011-06-20 18:47:02 PDT
Created attachment 97907 [details]
Patch
Comment 15 WebKit Review Bot 2011-06-20 19:11:55 PDT
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
Comment 16 WebKit Review Bot 2011-06-20 19:12:00 PDT
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
Comment 17 Dirk Schulze 2011-06-25 12:41:34 PDT
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
Comment 18 Dirk Schulze 2011-06-25 12:43:02 PDT
(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
Comment 19 Dirk Schulze 2011-06-25 12:44:29 PDT
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
Comment 20 Nikolas Zimmermann 2011-06-25 15:33:04 PDT
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.
Comment 21 Rob Buis 2011-06-26 19:40:10 PDT
Created attachment 98655 [details]
Patch
Comment 22 WebKit Review Bot 2011-06-26 20:10:08 PDT
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
Comment 23 WebKit Review Bot 2011-06-26 20:10:14 PDT
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
Comment 24 Dirk Schulze 2011-07-20 03:22:44 PDT
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
Comment 25 Rob Buis 2011-07-20 12:03:50 PDT
Created attachment 101492 [details]
Patch
Comment 26 Rob Buis 2011-07-20 12:10:25 PDT
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.
Comment 27 WebKit Review Bot 2011-07-20 12:36:57 PDT
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
Comment 28 Nikolas Zimmermann 2011-07-20 12:50:34 PDT
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.
Comment 29 Dirk Schulze 2011-07-20 12:56:15 PDT
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?
Comment 30 Rob Buis 2011-07-20 13:34:17 PDT
Created attachment 101502 [details]
Patch
Comment 31 Rob Buis 2011-07-20 13:35:47 PDT
(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.
Comment 32 Rob Buis 2011-07-20 13:37:41 PDT
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.
Comment 33 Nikolas Zimmermann 2011-07-20 13:43:48 PDT
(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.
Comment 34 WebKit Review Bot 2011-07-20 14:04:18 PDT
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
Comment 35 Dirk Schulze 2011-07-22 00:14:33 PDT
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.
Comment 36 Rob Buis 2011-07-25 12:50:31 PDT
Created attachment 101896 [details]
Patch
Comment 37 WebKit Review Bot 2011-07-25 17:53:52 PDT
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
Comment 38 Nikolas Zimmermann 2011-07-27 00:43:59 PDT
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?
Comment 39 Rob Buis 2011-07-27 10:40:28 PDT
This got landed in r91850.
Comment 40 Dirk Schulze 2011-10-30 03:02:48 PDT
(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.
Comment 41 Nikolas Zimmermann 2012-02-01 04:16:45 PST
(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 :(