Bug 38188 – Remove custom NodeIterator bindings

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Remove custom NodeIterator bindings

[Adam Barth]

Created attachment 54402 [details]
Patch

[WebKit Review Bot]

Attachment 54402 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:210:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:212:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:225:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:227:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:230:  g_res is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.cpp:150:  Missing spaces around |  [whitespace/operators] [3]
WebCore/bindings/scripts/test/JS/JSTestObj.cpp:151:  Missing spaces around |  [whitespace/operators] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.h:95:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.h:98:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 9 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 54402 [details]
Patch

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.

[Adam Barth]

I think the behavior is correct.  I had the same question, but I looked at setDOMException and it made me think it would work right.  I'll look at the rest of your comments in the morning.

[Eric Seidel (no email)]

I was wrong:
http://trac.webkit.org/browser/trunk/WebCore/bindings/js/JSDOMBinding.cpp#L566

Could still use some better ChangeLogs.

Seems like it's also about time to fix setDOMException to take a globalObject. :)  (obviously in a separate change)

[Adam Barth]

(In reply to comment #3)
> (From update of attachment 54402 [details])
> WebCore/bindings/scripts/CodeGeneratorJS.pm:1894
>  +          push(@implContent, $indent . "setDOMException(exec, ec);\n") if
> @{$function->raisesExceptions};
> Comments in the ChangeLog would be helpful...

Done.

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

Done.

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

I think so.

> 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.

As discussed before, that's not how setDOMException works.

> 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.

We're going to fix lots of random bugs in this proces.  This didn't cause any tests to fail AFAIK.

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

This is left-over junk from an earlier change where I didn't re-generate the reference files correctly.  I'll handle that in a separate commit.

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

This is a different bug in the CodeGenerator.  We should fix it in a different patch.

[Adam Barth]

Created attachment 54431 [details]
Patch

[WebKit Review Bot]

Attachment 54431 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:210:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:212:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:225:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:227:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:230:  g_res is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/JS/JSTestObj.cpp:150:  Missing spaces around |  [whitespace/operators] [3]
WebCore/bindings/scripts/test/JS/JSTestObj.cpp:151:  Missing spaces around |  [whitespace/operators] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.h:95:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.h:98:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 9 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 54431 [details]
Patch

Looks right.

[Darin Adler]

(In reply to comment #6)
> > 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.
> 
> As discussed before, that's not how setDOMException works.

Typically in a case like this we construct a test case demonstrating that's OK.

> > 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.
> 
> We're going to fix lots of random bugs in this proces.  This didn't cause any
> tests to fail AFAIK.

Our project policy is normally that we create test cases when we notice bugs and fix them. Even if the reason for the code change in the first place was not that particular bug. I don’t think “we’re going to fix lots of random bugs” is a convincing argument.

Whether an existing test covers this or not is not the point.

This won’t prevent me from approving this patch, but I think we should continue to create test cases for bugs we fix. I spend quite a bit of time creating test cases for bugs I notice while fixing another bug, and this seems like the same sort of thing.

[Darin Adler]

Comment on attachment 54431 [details]
Patch

> +        $result .= $indent . "if (state.hadException())\n";

I think we want UNLIKELY here just as with the UNLIKELY(ec) case. Can we do that?

[Darin Adler]

Looks like Eric got in with review+ and commit-queue+ before I did, although I had intended both.

[Adam Barth]

> Typically in a case like this we construct a test case demonstrating that's OK.

Ok.  Will do shortly.

[Adam Barth]

Committed r58333: <http://trac.webkit.org/changeset/58333>

[Adam Barth]

(In reply to comment #13)
> > Typically in a case like this we construct a test case demonstrating that's OK.
> 
> Ok.  Will do shortly.

Turns out this is impossible because the only time ec is set is as the first
action of the method:

http://trac.webkit.org/browser/trunk/WebCore/dom/NodeIterator.cpp#L87

which means there's no test case we can write with our current code that shows
that the ExecState exception has priority.  (At least, I don't see how to write
code that generates both kinds of exceptions.)

[Adam Barth]

> Our project policy is normally that we create test cases when we notice bugs
> and fix them. Even if the reason for the code change in the first place was not
> that particular bug. I don’t think “we’re going to fix lots of random bugs” is
> a convincing argument.

A test that documents the prototype chain fix is now available in https://bugs.webkit.org/show_bug.cgi?id=38219

[Darin Adler]

(In reply to comment #16)
> > Our project policy is normally that we create test cases when we notice bugs
> > and fix them. Even if the reason for the code change in the first place was not
> > that particular bug. I don’t think “we’re going to fix lots of random bugs” is
> > a convincing argument.
> 
> A test that documents the prototype chain fix is now available in
> https://bugs.webkit.org/show_bug.cgi?id=38219

Thanks, I really appreciate it!