[webkit-reviews] review denied: [Bug 41655] Implement SVGSVGElement.getElementById : [Attachment 60923] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Rob Buis <rwlbuis at gmail.com>'s request
for review:
Bug 41655: Implement SVGSVGElement.getElementById
https://bugs.webkit.org/show_bug.cgi?id=41655

Attachment 60923: Patch
https://bugs.webkit.org/attachment.cgi?id=60923&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
WebCore/svg/SVGSVGElement.cpp:600
 +	if (element && element->isSVGElement() &&
element->isDescendantOf(this))
Why isSVGElement()?  Please test that case by adding some foreignObject content
(or simply non-SVG content) under the SVG tree.

WebCore/svg/SVGSVGElement.cpp:606
 +	    if (node->isElementNode()) {
This should use the "early return" style, with an early continue here.

LayoutTests/svg/custom/svg-getelementid.xhtml:3
 +	<div id="foo">
This does not test "foo" both before and after, like suggested in my comment.

LayoutTests/svg/custom/svg-getelementid-expected.txt:1
 +  PASS
If you're going to go to all the trouble to re-write the test, you might as
well use the script-test js files (even if you don't make this a script test). 
Teh output is much much much nicer to read.

LayoutTests/svg/custom/svg-getelementid.xhtml:4
 +	  <div id="bar">
Why put all the content inside these divs instead of having them before or
after the SVG content?