[webkit-reviews] review denied: [Bug 41655] Implement SVGSVGElement.getElementById : [Attachment 60923] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jul 8 12:13:20 PDT 2010
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?
More information about the webkit-reviews
mailing list