Bug 187851

This should be easy to fix!

(In reply to Ryosuke Niwa from comment #1) > This should be easy to fix! Yes, just a change in the IDL file but I was not sure how to test it. Maybe https://w3c-test.org/custom-elements/reactions/HTMLInputElement.html can be modified to not rely on customized built-in elements?

Oh, actually, this is one of those [CEReactions] that are only needed for customized builtins. We should simply either new IDL attribute e.g. [CEReactionsNotNeeded] or make CEReactions take a value as in [CEReactions=NotNeeded] and simply assert that there is no new custom element reaction being enqueued.

(In reply to Ryosuke Niwa from comment #3) > Oh, actually, this is one of those [CEReactions] that are only needed for > customized builtins. We should simply either new IDL attribute e.g. > [CEReactionsNotNeeded] or make CEReactions take a value as in > [CEReactions=NotNeeded] and simply assert that there is no new custom > element reaction being enqueued. I see. Yes, probably we should do something like that for all the similar cases, so that one is not surprised when comparing the IDL against the spec.

Created attachment 345514 [details] Patch

(In reply to Ryosuke Niwa from comment #3) > Oh, actually, this is one of those [CEReactions] that are only needed for > customized builtins. We should simply either new IDL attribute e.g. > [CEReactionsNotNeeded] or make CEReactions take a value as in > [CEReactions=NotNeeded] and simply assert that there is no new custom > element reaction being enqueued. @Ryosuke: I just uploaded a patch that tries to do that. Is it what we you had in mind? Note: I still need to add tests and do all the CEReaction=NotNeeded.

Attachment 345514 [details] did not pass style-queue: ERROR: 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 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 345514 [details] Patch Attachment 345514 [details] did not pass bindings-ews (mac): Output: https://webkit-queues.webkit.org/results/8610226 New failing tests: (JS) JSTestCEReactions.cpp (JS) JSTestCEReactionsStringifier.cpp

Comment on attachment 345514 [details] Patch Attachment 345514 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8612417 New failing tests: http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html

Created attachment 345531 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Created attachment 345538 [details] Patch

Created attachment 345564 [details] Patch

Comment on attachment 345564 [details] Patch Attachment 345564 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8636398 New failing tests: http/tests/security/local-video-source-from-remote.html http/tests/security/canvas-remote-read-remote-video-localhost.html

Created attachment 345656 [details] Archive of layout-test-results from ews201 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews201 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Comment on attachment 345564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=345564&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7220 > + foreach my $interface (@_) { > + if ($interface && $interface->extendedAttributes->{CEReactions}) { > + InsertCustomElementReactionStack($outputArray, $conditional, $indent, $interface); > + return; > + } > + } The thing that worries me here is the case where this function is passed multiple interfaces and on has normal CEReactions and one has CEReactions=NotNeeded. In that case, depending on the order you passed the interfaces either CustomElementReactionStack or CustomElementReactionStackWithReactionsForbidden will be used. It might be worth asserting that all the passed in interfaces with CEReactions have the same value for those CEReactions.

Created attachment 345839 [details] Patch

Comment on attachment 345839 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=345839&action=review > Source/WebCore/dom/CustomElementReactionQueue.h:109 > +class CustomElementReactionStackWithReactionsForbidden : public CustomElementReactionStack { I don't think we can afford to instantiate CustomElementReactionStack everywhere like this. We need to create a completely new class (i.e. doesn't inherit from CustomElementReactionStack) which clang can optimize way. r- because of this. Also, this class name is rather verbose. How about UnneededCustomElementReactionStack? > Source/WebCore/html/HTMLAnchorElement.idl:22 > + [CEReactions=NotNeeded, Reflect] attribute DOMString charset; Let's split the patch to make the binding code & actual IDL changes.

<rdar://problem/42843003>

Created attachment 346584 [details] Adds CEReactions=NotNeeded

Created attachment 346585 [details] Adds the basic support

Created attachment 346652 [details] Fixed release builds

Comment on attachment 346652 [details] Fixed release builds View in context: https://bugs.webkit.org/attachment.cgi?id=346652&action=review r=me > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7416 > +sub GenerateCustomElementReactionsStackIfNeeed Typo: GenerateCustomElementReactionsStackIfNeeed -> GenerateCustomElementReactionsStackIfNeeded

Committed r234636: <https://trac.webkit.org/changeset/234636>