Bug 90218 – IDL overloads should not treat wrapper types as nullable by default

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Joshua Bell]

IDL overloads should not treat wrapper types as nullable by default

[Joshua Bell]

Created attachment 150036 [details]
Patch

[WebKit Review Bot]

Attachment 150036 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joshua Bell]

This is NOT ready for landing, just a proof of concept, but I want the bots to grovel over it and report errors. 

The issue the patch is intended to fix is mentioned in bug 84217:

transaction(in DOMStringList storeNames, in DOMString mode)
transaction(in DOMString storeName, in DOMString mode);

Called as transaction(null), DOMStringList is matched but the WebIDL rules say that null should coerce to a DOMString of "null".

This patch introduces a [Nullable] attribute which should correspond to the Type? signifier in IDL, but isn't supported broadly, just as an object parameter and just in overloaded methods. Adding [Nullable] restores the previous behavior, leaving it off removes the IsNull check.

Outside of IDB, there are about 34 places that I could find using overloads, all of which probably need to have [Nullable] added. I have NOT done that exercise yet - I want feedback first.

Alternately, I could flip this to [NonNullable] which would then require only touching the places where the WebIDL behavior is desired. That'd be a much safer/more scoped change, but would leave us farther from WebKit IDL / WebIDL unity.

[Joshua Bell]

haraken@ - any guidance?

[WebKit Review Bot]

Comment on attachment 150036 [details]
Patch

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

New failing tests:
canvas/philip/tests/2d.imageData.create1.zero.html
fast/canvas/webgl/copy-tex-image-and-sub-image-2d.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-data-rgb565.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-canvas-rgba5551.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-data.html
fast/canvas/webgl/gl-teximage.html
fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias.html
fast/canvas/webgl/buffer-data-array-buffer.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-canvas.html
http/tests/security/script-crossorigin-loads-correctly.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-rgba5551.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-canvas-rgba4444.html
fast/canvas/webgl/object-deletion-behaviour.html
http/tests/canvas/webgl/origin-clean-conformance.html
fast/files/url-null.html
canvas/philip/tests/2d.imageData.put.null.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-data-rgba5551.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image.html
fast/canvas/webgl/framebuffer-test.html
fast/canvas/canvas-createImageData.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-canvas-rgb565.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-rgb565.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-array-buffer-view.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-data-rgba4444.html
canvas/philip/tests/2d.drawImage.null.html
canvas/philip/tests/2d.pattern.image.null.html
fast/canvas/webgl/invalid-passed-params.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-rgba4444.html

[WebKit Review Bot]

Created attachment 150069 [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: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Joshua Bell]

Created attachment 150240 [details]
Patch

[Joshua Bell]

Per discussion over in webkit.org/b/84217 this new patch adds parser support for "?" as a type suffix, but only in method parameters and it ends up being an alias for "[Nullable]" rather than adding extensive support throughout the code generators to handle the suffix. If this approach is acceptable, I'll proceed.

Again, only the test and IndexedDB IDLs are updated. I'll do another pass shortly to annotate the other affected IDL files.

[Joshua Bell]

Created attachment 150254 [details]
Patch

[Joshua Bell]

(In reply to comment #9)
> Created an attachment (id=150254) [details]
> Patch

Latest patch adds Nullable type suffix (i.e. "?") to all overloaded instances with wrapped types. Verified with some grepping and before/after run of the code generator w/ Chromium/V8 - a JSC run probably wouldn't hurt, but let's see what the bots say.

The implication will be that "owners" of the affected modules (IndexedDB, WebAudio, Canvas2D, WebGL, DataTransferItemList, BlobBuilder, DOMURL) who don't want the Nullable behavior that was previously implied can remove ? and update their code/tests at their leisure.

[WebKit Review Bot]

Comment on attachment 150254 [details]
Patch

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

New failing tests:
svg/zoom/page/zoom-img-preserveAspectRatio-support-1.html

[WebKit Review Bot]

Created attachment 150291 [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: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Kentaro Hara]

Comment on attachment 150254 [details]
Patch

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

Supporting '?' definitely looks like a progression! The approach looks great to me. Marking r- due to missing test cases.

> Verified with some grepping and before/after run of the code generator w/ Chromium/V8 - a JSC run probably wouldn't hurt, but let's see what the bots say.

Please doubly confirm that all generated code has 0 diff both in JSC and in V8.

> Source/WebCore/ChangeLog:7
> +

Please explain what this patch is doing.

> Source/WebCore/ChangeLog:8
> +        No new tests - no behavioral changes.

Please add test cases for '?' to TestObj.idl

[Kentaro Hara]

Comment on attachment 150254 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1266
> +            if (defined $parameter->extendedAttributes->{"Nullable"}) {

Remove 'defined'. (See the following comment.)

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1360
> +            if (defined $parameter->extendedAttributes->{"Nullable"}) {

Ditto.

> Source/WebCore/bindings/scripts/IDLParser.pm:255
> +        $paramDataNode->extendedAttributes->{'Nullable'} = "" if $paramTypeSuffix eq "?";

CodeGenerators are likely to use 'if ($paramDataNode->extendedAttributes->{'Nullable'})'. To make it work, let's use '$paramDataNode->extendedAttributes->{'Nullable'} = 1'.

[Erik Arvidsson]

Comment on attachment 150254 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1266
>> +            if (defined $parameter->extendedAttributes->{"Nullable"}) {
> 
> Remove 'defined'. (See the following comment.)

I would prefer if we exposed this as $parameter->isNullable just like we do for isStatic on methods.

>> Source/WebCore/bindings/scripts/IDLParser.pm:255
>> +        $paramDataNode->extendedAttributes->{'Nullable'} = "" if $paramTypeSuffix eq "?";
> 
> CodeGenerators are likely to use 'if ($paramDataNode->extendedAttributes->{'Nullable'})'. To make it work, let's use '$paramDataNode->extendedAttributes->{'Nullable'} = 1'.

Lets just do isNullable and be done with it.

[Joshua Bell]

> Please doubly confirm that all generated code has 0 diff both in JSC and in V8.

Will do.

> Please add test cases for '?' to TestObj.idl

Of course. Had one, then went too far in my "0 diffs" check.

> I would prefer if we exposed this as $parameter->isNullable just like we do for isStatic on methods.

Agreed, will do.

[Joshua Bell]

(In reply to comment #11)
> (From update of attachment 150254 [details])
> Attachment 150254 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/13114557
> 
> New failing tests:
> svg/zoom/page/zoom-img-preserveAspectRatio-support-1.html

Unrelated - this test was broken on Linux while the patch was uploaded: 

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=zoom-img-preserveAspectRatio-support-1

[Joshua Bell]

Created attachment 150458 [details]
Patch

[Joshua Bell]

(In reply to comment #18)
> Created an attachment (id=150458) [details]
> Patch

Updated:

* Uses $value->isNullable 

* I did a before/after test of all of the IDL files (619) under Source/WebCore and Tools/WebKitTestRunner with all of the ENABLE_XXX flags I could find with both the JS and V8 generators - no diffs.

* Additional overload case without "?" added to TestObj.idl and new baselines generated. (This *was* in the previous patch, I just hadn't touched the yet ChangeLog.)

[Erik Arvidsson]

Comment on attachment 150458 [details]
Patch

Looks good to me. I'm not a reviewer yet so I'll let haraken do the r+

[Kentaro Hara]

Comment on attachment 150458 [details]
Patch

LGTM too. Thanks arv for reviewing!

[WebKit Review Bot]

Comment on attachment 150458 [details]
Patch

Clearing flags on attachment: 150458

Committed r121714: <http://trac.webkit.org/changeset/121714>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.