[webkit-reviews] review denied: [Bug 38188] Remove custom NodeIterator bindings : [Attachment 54402] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Adam Barth <abarth at webkit.org>'s
request for review:
Bug 38188: Remove custom NodeIterator bindings
https://bugs.webkit.org/show_bug.cgi?id=38188

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
WebCore/bindings/scripts/CodeGeneratorJS.pm:1894
 +	    push(@implContent, $indent . "setDOMException(exec, ec);\n") if
@{$function->raisesExceptions};
Comments in the ChangeLog would be helpful...

WebCore/bindings/scripts/CodeGeneratorV8.pm:2422
 +	if ($hasScriptState) {
Again here....

WebCore/bindings/scripts/test/JS/JSTestObj.cpp:625
 +	setDOMException(exec, ec);
Is this correct to unconditionally set the dom exception?

WebCore/bindings/scripts/test/JS/JSTestObj.cpp:640
 +	setDOMException(exec, ec);
This will smash over an an existing exception on the exec it seems.  Seems
wrong.

WebCore/bindings/scripts/test/JS/JSTestObj.cpp:639
 +	JSC::JSValue result = toJS(exec, castedThisObj->globalObject(),
WTF::getPtr(imp->withScriptStateObjException(exec, ec)));
This will probably cause this to actually fix isolated world bugs.  You may
need to update test results.

WebCore/bindings/scripts/test/V8/V8TestObj.cpp: 
 +	if (state->hadException())
Why?  ChangeLog comment would help here.

WebCore/bindings/scripts/test/V8/V8TestObj.cpp:361
 +	EmptyScriptState state;
Indent?

I think the JSC handling of the case where the callback throws an exception
(which is left on the exec) is not handled correctly.