Bug 90218 - IDL overloads should not treat wrapper types as nullable by default
Summary: IDL overloads should not treat wrapper types as nullable by default
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on:
Blocks: 84217
  Show dependency treegraph
 
Reported: 2012-06-28 16:04 PDT by Joshua Bell
Modified: 2012-07-02 18:08 PDT (History)
10 users (show)

See Also:


Attachments
Patch (15.33 KB, patch)
2012-06-28 16:04 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.32 MB, application/zip)
2012-06-28 19:54 PDT, WebKit Review Bot
no flags Details
Patch (18.50 KB, patch)
2012-06-29 13:09 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (37.07 KB, patch)
2012-06-29 14:01 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (850.16 KB, application/zip)
2012-06-29 19:17 PDT, WebKit Review Bot
no flags Details
Patch (37.94 KB, patch)
2012-07-02 11:50 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2012-06-28 16:04:12 PDT
IDL overloads should not treat wrapper types as nullable by default
Comment 1 Joshua Bell 2012-06-28 16:04:59 PDT
Created attachment 150036 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-28 16:11:20 PDT
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.
Comment 3 Joshua Bell 2012-06-28 16:13:21 PDT
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.
Comment 4 Joshua Bell 2012-06-28 16:14:23 PDT
haraken@ - any guidance?
Comment 5 WebKit Review Bot 2012-06-28 19:54:30 PDT
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
Comment 6 WebKit Review Bot 2012-06-28 19:54:35 PDT
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
Comment 7 Joshua Bell 2012-06-29 13:09:05 PDT
Created attachment 150240 [details]
Patch
Comment 8 Joshua Bell 2012-06-29 13:14:25 PDT
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.
Comment 9 Joshua Bell 2012-06-29 14:01:25 PDT
Created attachment 150254 [details]
Patch
Comment 10 Joshua Bell 2012-06-29 14:06:01 PDT
(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.
Comment 11 WebKit Review Bot 2012-06-29 19:17:54 PDT
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
Comment 12 WebKit Review Bot 2012-06-29 19:17:59 PDT
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
Comment 13 Kentaro Hara 2012-07-01 09:52:46 PDT
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
Comment 14 Kentaro Hara 2012-07-01 09:57:57 PDT
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'.
Comment 15 Erik Arvidsson 2012-07-02 09:46:54 PDT
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.
Comment 16 Joshua Bell 2012-07-02 10:06:06 PDT
> 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.
Comment 17 Joshua Bell 2012-07-02 10:21:21 PDT
(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
Comment 18 Joshua Bell 2012-07-02 11:50:30 PDT
Created attachment 150458 [details]
Patch
Comment 19 Joshua Bell 2012-07-02 11:54:35 PDT
(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.)
Comment 20 Erik Arvidsson 2012-07-02 12:21:41 PDT
Comment on attachment 150458 [details]
Patch

Looks good to me. I'm not a reviewer yet so I'll let haraken do the r+
Comment 21 Kentaro Hara 2012-07-02 16:34:18 PDT
Comment on attachment 150458 [details]
Patch

LGTM too. Thanks arv for reviewing!
Comment 22 WebKit Review Bot 2012-07-02 18:08:42 PDT
Comment on attachment 150458 [details]
Patch

Clearing flags on attachment: 150458

Committed r121714: <http://trac.webkit.org/changeset/121714>
Comment 23 WebKit Review Bot 2012-07-02 18:08:50 PDT
All reviewed patches have been landed.  Closing bug.