| Summary: | IDL overloads should not treat wrapper types as nullable by default | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Joshua Bell <jsbell> | ||||||||||||||
| Component: | New Bugs | Assignee: | Joshua Bell <jsbell> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | abarth, arv, dglazkov, eric.carlson, feature-media-reviews, haraken, japhet, jochen, ojan, webkit.review.bot | ||||||||||||||
| Priority: | P2 | ||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Bug Depends on: | |||||||||||||||||
| Bug Blocks: | 84217 | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Joshua Bell
2012-06-28 16:04:12 PDT
Created attachment 150036 [details]
Patch
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.
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. haraken@ - any guidance? 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 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
Created attachment 150240 [details]
Patch
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. Created attachment 150254 [details]
Patch
(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 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 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 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 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 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. > 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. (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 Created attachment 150458 [details]
Patch
(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 on attachment 150458 [details]
Patch
Looks good to me. I'm not a reviewer yet so I'll let haraken do the r+
Comment on attachment 150458 [details]
Patch
LGTM too. Thanks arv for reviewing!
Comment on attachment 150458 [details] Patch Clearing flags on attachment: 150458 Committed r121714: <http://trac.webkit.org/changeset/121714> All reviewed patches have been landed. Closing bug. |