Bug 38188

Summary: Remove custom NodeIterator bindings
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, eric, webkit.review.bot, yaar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch eric: review+, eric: commit-queue+

Description Adam Barth 2010-04-27 02:35:03 PDT
Remove custom NodeIterator bindings
Comment 1 Adam Barth 2010-04-27 02:36:11 PDT
Created attachment 54402 [details]
Patch
Comment 2 WebKit Review Bot 2010-04-27 02:40:42 PDT
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.
Comment 3 Eric Seidel (no email) 2010-04-27 02:43:26 PDT
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.
Comment 4 Adam Barth 2010-04-27 02:46:07 PDT
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.
Comment 5 Eric Seidel (no email) 2010-04-27 02:50:25 PDT
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)
Comment 6 Adam Barth 2010-04-27 10:50:18 PDT
(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.
Comment 7 Adam Barth 2010-04-27 10:57:52 PDT
Created attachment 54431 [details]
Patch
Comment 8 WebKit Review Bot 2010-04-27 11:05:07 PDT
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.
Comment 9 Eric Seidel (no email) 2010-04-27 12:01:17 PDT
Comment on attachment 54431 [details]
Patch

Looks right.
Comment 10 Darin Adler 2010-04-27 12:01:43 PDT
(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.
Comment 11 Darin Adler 2010-04-27 12:02:00 PDT
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?
Comment 12 Darin Adler 2010-04-27 12:02:19 PDT
Looks like Eric got in with review+ and commit-queue+ before I did, although I had intended both.
Comment 13 Adam Barth 2010-04-27 12:07:42 PDT
> Typically in a case like this we construct a test case demonstrating that's OK.

Ok.  Will do shortly.
Comment 14 Adam Barth 2010-04-27 13:55:25 PDT
Committed r58333: <http://trac.webkit.org/changeset/58333>
Comment 15 Adam Barth 2010-04-27 13:59:37 PDT
(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.)
Comment 16 Adam Barth 2010-04-27 14:16:07 PDT
> 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
Comment 17 Darin Adler 2010-04-27 14:20:15 PDT
(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!