Bug 9717

Created attachment 9175 [details] Patch v1 Proposed patch.

Created attachment 9176 [details] Generated code changes (DO NOT APPLY) This patch demonstrates how the generated code changes after Patch v2 from Bug 9179 is applied, then this bug's Patch v1 is applied.

(In reply to comment #2) > Created an attachment (id=9176) [edit] > Generated code changes (DO NOT APPLY) > > This patch demonstrates how the generated code changes after Patch v2 from Bug > 9179 is applied, then this bug's Patch v1 is applied. I wonder if the argsCount comparison should be '<=' instead of  '==' in case a method is called within JavaScript with too few arguments?

Yes, I think you need <=, although I would vote for < as being a little clearer. That's something to change in your patch for bug 9179, right? With one exception, I think this patch is ready to land once the patch for bug 9179 is reviewed. The exception is that you do need a testcase, even if there is no behavior change. We have had many bugs with IDL changes accidentally producing behavior changes. You need to confirm that the optional parameter calling convention actually works. If an existing test already covers that, it's fine to name that test in the ChangeLog instead of duplicating the effort, but I don't think it's OK to land this change without any test.

Comment on attachment 9175 [details] Patch v1 Looks good. r=me

(In reply to comment #4) > Yes, I think you need <=, although I would vote for < as being a little > clearer. That's something to change in your patch for bug 9179, right? Yes.  I'll go make that change in Bug 9179. > With one exception, I think this patch is ready to land once the patch for bug > 9179 is reviewed. The exception is that you do need a testcase, even if there > is no behavior change. We have had many bugs with IDL changes accidentally > producing behavior changes. You need to confirm that the optional parameter > calling convention actually works. If an existing test already covers that, > it's fine to name that test in the ChangeLog instead of duplicating the effort, > but I don't think it's OK to land this change without any test. Test cases for DOMWindow.getMatchedCSSRules() are (the last two rely on the overloaded behavior): LayoutTests/fast/dom/defaultView.html LayoutTests/fast/inspector/matchedrules.html LayoutTests/fast/inspector/style.html Test cases for Element.scrollIntoView() are: WebCore/manual-tests/scrollIntoView-horizontal.html WebCore/manual-tests/scrollIntoView-vertical.html Note that there are no test cases for Element.scrollIntoViewIfNeeded(), although the change to the code is identical for both scrollIntoView() and scrollIntoViewIfNeeded(). The manual tests above passed, e.g., they did the same thing in Safari 2.0.4 (419.3) on Mac OS X 10.4.7 (8J135/PowerPC) as they did with a locally-built WebKit r15147 with patches for Bug 9179 and this patch applied; and they passed per instructions on the manual tests. All of the layout tests passed as well.

(In reply to comment #6) > Test cases for Element.scrollIntoView() are: > > WebCore/manual-tests/scrollIntoView-horizontal.html > WebCore/manual-tests/scrollIntoView-vertical.html It's worth noting that both of these test cases exercise the overloaded behavior as well.

Sorry, I didn't read Geoff's comments. I think it's OK to land -- there do seem to be tests here already. It would be better to cover all the functions, though. Not having a test for scrollIntoViewIfNeeded is a minor problem, because the way our IDL works, a small syntax error could completely break that function -- it doesn't matter how similar scrollIntoView is, because an omitted semicolon, for example, could break one and not affect the other.

Committed revision 15619.