Bug 187851 – Add CEReactions=NotNeeded for reactions only needed for customized builtins

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Frédéric Wang (:fredw)]

partial interface HTMLInputElement {
    [CEReactions]
    attribute DOMString capture;
};

This is tested by https://w3c-test.org/custom-elements/reactions/HTMLInputElement.html (imported in bug 187806) but we lack support for customized built-in elements (bug 187849) so the test fails.

[Ryosuke Niwa]

This should be easy to fix!

[Frédéric Wang (:fredw)]

(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?

[Ryosuke Niwa]

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.

[Frédéric Wang (:fredw)]

(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.

[Frédéric Wang (:fredw)]

Created attachment 345514 [details]
Patch

[Frédéric Wang (:fredw)]

(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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Frédéric Wang (:fredw)]

Created attachment 345538 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 345564 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Sam Weinig]

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.

[Frédéric Wang (:fredw)]

Created attachment 345839 [details]
Patch

[Ryosuke Niwa]

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.

[Radar WebKit Bug Importer]

<rdar://problem/42843003>

[Ryosuke Niwa]

Created attachment 346584 [details]
Adds CEReactions=NotNeeded

[Ryosuke Niwa]

Created attachment 346585 [details]
Adds the basic support

[Ryosuke Niwa]

Created attachment 346652 [details]
Fixed release builds

[Chris Dumez]

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

[Ryosuke Niwa]

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