Bug 62750 – Change IDL code generator to require all arguments by default

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Mark Pilgrim (Google)]

As per discussion on public-webapps, WebIDL is changing the default behavior to require all function arguments by default and raise an exception when an argument is missing. (This behavior is currently opt-in in WebKit's IDL system, on a function-by-function basis, with the "RequiresAllArguments=Raise" flag.) To order to match WebIDL as closely as possible, this patch adds an interface-level "LegacyDefaultOptionalArguments" flag and sets it on all existing IDL files (500+), then changes the code generator Perl scripts to behave the old way in the presence of the flag.

Future bugs will focus on adjusting individual IDL files and corresponding tests for newer APIs (like IndexedDB) which are moving to adopt the new default raise-on-missing-argument behavior. This patch does nothing beyond defining the new flag and adjusting existing IDL files so they behave the same way they behaved yesterday.

[Mark Pilgrim (Google)]

Created attachment 97354 [details]
Patch

[Adam Barth]

Comment on attachment 97354 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97354&action=review

> Source/WebCore/svg/SVGVKernElement.idl:26
> -    interface [Conditional=SVG&SVG_FONTS] SVGVKernElement : SVGElement {
> +    interface [
> +        LegacyDefaultOptionalArguments,
> +        Conditional=SVG&SVG_FONTS
> +    ] SVGVKernElement : SVGElement {
>      };

This one seems slightly unnecessary.  :)

It's probably ok to add this everywhere and then remove it on a case-by-case basis.

[Adam Barth]

It sounds like the consensus in the working group and in the WebKit community is require all arguments by default (at least for new interfaces).  The approach of switching the default and marking all existing interfaces causes a lot of edits to the codebase, but I think it's a sound approach.  I presume you'll be going through the interfaces carefully and evaluating whether we can remove this attribute.

Let's leave the patch up for review for a little while to see if other folks have input on whether this is a good approach.

[WebKit Review Bot]

Comment on attachment 97354 [details]
Patch

Attachment 97354 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8841882

New failing tests:
fast/canvas/canvas-overloads-fillText.html
svg/dom/SVGPointList-basics.xhtml
svg/dom/SVGLengthList-basics.xhtml
svg/dom/SVGTransformList.html
svg/dom/SVGLength.html
fast/canvas/canvas-overloads-drawImageFromRect.html
svg/dom/svgpath-out-of-bounds-getPathSeg.html
svg/dom/SVGStringList-basics.xhtml
fast/dom/Window/window-scroll-arguments.html
svg/dom/SVGPoint.html
svg/dom/SVGAngle.html
svg/dom/SVGPaint.html
svg/dom/SVGNumberList-basics.xhtml
fast/canvas/webgl/data-view-test.html
svg/dom/SVGTransform.html
fast/canvas/canvas-overloads-strokeText.html
svg/dom/SVGTransformList-basics.xhtml
fast/dom/Window/window-resize-and-move-arguments.html
svg/dom/SVGMatrix.html
svg/dom/SVGColor.html

[WebKit Review Bot]

Created attachment 97359 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Mark Pilgrim (Google)]

Stupid Perl scoping rules.

[Mark Pilgrim (Google)]

Created attachment 97366 [details]
Patch

[Adam Barth]

Comment on attachment 97366 [details]
Patch

I was hoping some more folks would weigh in, but I think this is the right path for us to take.  We'll be removing LegacyDefaultOptionalArguments from IDL files for a while, but this at least gets us started on that path.  Thanks Mark.

[Darin Adler]

Seems great to change the default before we get in any deeper.

[WebKit Review Bot]

Comment on attachment 97366 [details]
Patch

Clearing flags on attachment: 97366

Committed r89148: <http://trac.webkit.org/changeset/89148>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.