Bug 172688 – [WebIDL] Generate named property deleters

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Sam Weinig]

Its time to generate named property deleters.

[Sam Weinig]

Created attachment 311456 [details]
Test case

Turns out we have some subtle bugs in our current hand rolled implementation (not surprising). Attaching test case showing them.

[Sam Weinig]

Created attachment 311458 [details]
Patch

[Build Bot]

Comment on attachment 311458 [details]
Patch

Attachment 311458 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3835495

New failing tests:
storage/domstorage/localstorage/delete-defineproperty-removal.html

[Build Bot]

Created attachment 311459 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311458 [details]
Patch

Attachment 311458 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3835548

New failing tests:
storage/domstorage/localstorage/delete-defineproperty-removal.html

[Build Bot]

Created attachment 311460 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311458 [details]
Patch

Attachment 311458 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3835515

New failing tests:
storage/domstorage/localstorage/delete-defineproperty-removal.html

[Build Bot]

Created attachment 311461 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311458 [details]
Patch

Attachment 311458 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3835573

New failing tests:
storage/domstorage/localstorage/delete-defineproperty-removal.html
compositing/masks/compositing-clip-path-change-no-repaint.html

[Build Bot]

Created attachment 311462 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Sam Weinig]

Created attachment 311480 [details]
Patch

[Build Bot]

Comment on attachment 311480 [details]
Patch

Attachment 311480 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3838312

New failing tests:
http/tests/cache/cancel-during-revalidation-succeeded.html

[Build Bot]

Created attachment 311483 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Sam Weinig]

Created attachment 311485 [details]
Patch

[Chris Dumez]

Comment on attachment 311485 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMAbstractOperations.h:38
> +template<bool overrideBuiltins, class JSClass>

Could we use an enum class instead of a boolean? I understand this is only used in generated code but the call sites are really not very readable.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:822
> +        push(@$outputArray, "        return !impl.isSupportedPropertyIndex(index.value()) ? true : false;\n");

return !impl.isSupportedPropertyIndex(index.value()); ? Why the ternary?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:829
> +    # the remained of the algoritm ourselves.

typo: algorithm

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:856
> +        push(@$outputArray, "    return !impl.isSupportedPropertyIndex(index) ? true : false;\n");

Why the ternary?

> Source/WebCore/storage/Storage.h:50
> +    // Bindings support functions

Missing period at the end

[Sam Weinig]

(In reply to Chris Dumez from comment #15)
> Comment on attachment 311485 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=311485&action=review
> 
> > Source/WebCore/bindings/js/JSDOMAbstractOperations.h:38
> > +template<bool overrideBuiltins, class JSClass>
> 
> Could we use an enum class instead of a boolean? I understand this is only
> used in generated code but the call sites are really not very readable.

Ok.

> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:822
> > +        push(@$outputArray, "        return !impl.isSupportedPropertyIndex(index.value()) ? true : false;\n");
> 
> return !impl.isSupportedPropertyIndex(index.value()); ? Why the ternary?

Just cause it was how the spec was worded. I'll collapse it.

> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:829
> > +    # the remained of the algoritm ourselves.
> 
> typo: algorithm

Ok.

> > Source/WebCore/storage/Storage.h:50
> > +    // Bindings support functions
> 
> Missing period at the end

Ok.

[Sam Weinig]

Committed r217585: <http://trac.webkit.org/changeset/217585>

[Radar WebKit Bug Importer]

<rdar://problem/32479673>