Bug 9717 – Replace IDL ConvertUndefinedToTrue parameter attribute with Optional attribute

• Component: DOM
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Patch v2 for Bug 9179 (hence the dependency) implements the ability to call overloaded implementation methods when JavaScript methods have optional arguments.  This bug is to remove the ConvertUndefinedToTrue parameter attribute from the IDL language and replace it with the Optional attribute, letting overloaded implementation methods handle the default value.

Note that the ConvertUndefinedToTrue param attribute was first added in Bug 8227 (r13726).  See also Bug 9179 Comment #9.

[David Kilzer (:ddkilzer)]

Created attachment 9175 [details]
Patch v1

Proposed patch.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

(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?

[Geoffrey Garen]

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.

[Darin Adler]

Comment on attachment 9175 [details]
Patch v1

Looks good. r=me

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

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

[Darin Adler]

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.

[David Kilzer (:ddkilzer)]

Committed revision 15619.