Summary: | Implement getIntersectionList(), getEnclosureList(), checkIntersection() and checkEnclosure() in SVGSVGElement | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Rob Buis <rwlbuis> | ||||||||||||||||||||||||
Component: | SVG | Assignee: | Rob Buis <rwlbuis> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | dglazkov, helder.magalhaes, jeffschiller, krit, webkit.review.bot, webmaster, zimmermann | ||||||||||||||||||||||||
Priority: | P3 | ||||||||||||||||||||||||||
Version: | 420+ | ||||||||||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||||||||||
OS: | OS X 10.4 | ||||||||||||||||||||||||||
URL: | http://www.w3.org/TR/SVG/struct.html#InterfaceSVGSVGElement | ||||||||||||||||||||||||||
Bug Depends on: | 71184, 77179 | ||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||
Attachments: |
|
Description
Rob Buis
2006-10-13 02:26:30 PDT
Eric says, fun weekend hack, but low prio. 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.
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. 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 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? :) 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... Created attachment 97732 [details]
Patch
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 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
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 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 :-) (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? The mozilla bug has some interesting comments as well: https://bugzilla.mozilla.org/show_bug.cgi?id=501421 Created attachment 97907 [details]
Patch
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 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 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 (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 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 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. Created attachment 98655 [details]
Patch
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 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 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 Created attachment 101492 [details]
Patch
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 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 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 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? Created attachment 101502 [details]
Patch
(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. 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. (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 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 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. Created attachment 101896 [details]
Patch
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 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? This got landed in r91850. (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. (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 :( |