Summary: | Remove custom NodeIterator bindings | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||||
Component: | New Bugs | Assignee: | 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
Adam Barth
2010-04-27 02:35:03 PDT
Created attachment 54402 [details]
Patch
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 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.
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. 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) (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. Created attachment 54431 [details]
Patch
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 on attachment 54431 [details]
Patch
Looks right.
(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 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? Looks like Eric got in with review+ and commit-queue+ before I did, although I had intended both. > Typically in a case like this we construct a test case demonstrating that's OK.
Ok. Will do shortly.
Committed r58333: <http://trac.webkit.org/changeset/58333> (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.) > 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 (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! |