| Summary: | [WebIDL] document.body.onerror setter should setup a five-parameter listener | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alexey Shvayka <ashvayka> | ||||||
| Component: | Bindings | Assignee: | Alexey Shvayka <ashvayka> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Trivial | CC: | ahmad.saleem792, cdumez, changseok, clopez, darin, esprehn+autocc, ews-watchlist, gyuyoung.kim, kangil.han, kondapallykalyan, webkit-bug-importer, youennf | ||||||
| Priority: | P2 | Keywords: | BrowserCompat, InRadar, WPTImpact | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| See Also: | https://github.com/web-platform-tests/wpt/pull/32164 | ||||||||
| Attachments: |
|
||||||||
|
Description
Alexey Shvayka
2021-12-21 10:37:38 PST
Created attachment 447733 [details]
Patch
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess Hey Darin, this change follows-up your FIXME added in https://bugs.webkit.org/show_bug.cgi?id=142282. I would appreciate a feedback! Comment on attachment 447733 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=447733&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5437 > + my $isOnErrorEventHandler = $attribute->name eq "onerror" && ($interface->type->name eq "DOMWindow" || $interface->type->name eq "WorkerGlobalScope" || $attribute->extendedAttributes->{WindowEventHandler}); I still would like to do this by using OnErrorEventHandler and OnBeforeUnloadEventHandler in the IDL files rather than hard-coding the attribute and class names here. This change looks great to me, but I think we should do it by adding an understanding of the names OnErrorEventHandler and OnBeforeUnloadEventHandler. There are only four places in the entire code generator where the name EventHandler appears, so that is the maximum number of call sites we’d have to change to use a shared function that understands these three different EventHandler names that exist, or special case all type names with that end with an EventHandler suffix instead of comparing the whole type name. Then, I would still suggest we keep the rest of the code you wrote the same; this one line of code would be different. > Source/WebCore/dom/GlobalEventHandlers.idl:55 > + attribute EventHandler onerror; // OnErrorEventHandler semantics is added in GenerateAttributeSetterBodyDefinition We should write OnErrorEventHandler here in the IDL file and fix the code generator to understand what it means. Is there a good reason not to do that? If it’s really helpful to not write OnErrorEventHandler as the type, we could consider an extended attribute named [OnErrorEventHandler] instead, but that seems unnecessary. One of the ways you can tell this is a good idea is that we end up without having to write a comment explaining ourselves! > Source/WebCore/page/WindowEventHandlers.idl:31 > + [WindowEventHandler] attribute EventHandler onbeforeunload; // OnBeforeUnloadEventHandler semantics is added in JSEventListener::handleEvent() Ditto. > Source/WebCore/workers/WorkerGlobalScope.idl:38 > + attribute EventHandler onerror; // OnErrorEventHandler semantics is added in GenerateAttributeSetterBodyDefinition Ditto. (In reply to Darin Adler from comment #4) > We should write OnErrorEventHandler here in the IDL file and fix the code > generator to understand what it means. Is there a good reason not to do > that? I would say it's not practical to do that considering the necessary changes to functions like ProcessInterfaceSupplementalDependencies. Let's consider the changes for suggested approach: 1. Add attribute `OnErrorEventHandler onerror` to the DOMWindow.idl. We can't just amend the type of `onerror` in GlobalEventHandlers (like the spec does) because other receivers other than Window / Worker / <body> / <frameset> should setup single-parameter listener. This is fine though. 2. Teach code generator to support type names ending with EventHandler suffix => also neat and fine. Now we get a compilation error because there are two `onerror` accessors on DOMWindow. 3. We now need to teach ProcessInterfaceSupplementalDependencies to ignore an `ErrorHandler onerror` attribute from supplemental interface if there is a `OnErrorEventHandler onerror`. That won't be neat and I would rather prefer a hard-coded type name check, especially since it's nicely matches the spec, then somewhat weird merging. Same effort would be needed for using an [OnErrorEventHandler] extended attribute rather than a return type. How do the WebIDL specification and the other specifications deal with this conflict between onerror in GlobalEventhandlers and in DOMWindow with two different types? (In reply to Darin Adler from comment #6) > How do the WebIDL specification and the other specifications deal with this > conflict between onerror in GlobalEventhandlers and in DOMWindow with two > different types? `onerror` in GlobalEventHandlers has a OnErrorEventHandler type, expecting a five-parameter function, which is pretty confusing given that a regular listener is invoked for most receivers. WebIDL has no mechanism of validating / rejecting a user land function by signature, so that OnErrorEventHandler accepts a regular listener. However, when any event handler is called, if it's an ErrorEvent with "error" type fired on Window / Worker (step 3 of https://html.spec.whatwg.org/#the-event-handler-processing-algorithm), it's called with 5 arguments instead of 1. Unlike the spec, WebKit attempts to do less work in runtime, hence the JSErrorHandler and the additional code generation efforts. Given this history, I suggest we specify OnErrorEventHandler in both places, do the 1-argument version for efficiency by default and use an extended attribute on the interfaces that want the full 5-argument form. This won’t be hard to do. If you prefer to have me make that change I will be happy to. I think it’s even better to do it that way than the way you’ve done it here. (In reply to Darin Adler from comment #8) > Given this history, I suggest we specify OnErrorEventHandler in both places, > do the 1-argument version for efficiency by default and use an extended > attribute on the interfaces that want the full 5-argument form. Hmm, I don't see how that saves us from implementing the weird attribute merging. Unless we put an extended attribute on the Window / Worker / <body> / <frameset> interfaces itself, which doesn't seem better than hardcoding their names. I'm happy do the change myself! === As for OnBeforeUnloadEventHandler, there is no conflict of signatures for it. It's handled in JSEventListener::handleEvent() only for convenience. We can put a OnBeforeUnloadEventHandler type to the IDL, and make a subclass of JSEventListener, but is there a practical benefit to such a change? (In reply to Alexey Shvayka from comment #9) > Unless we put an extended attribute on the Window / Worker / <body> > / <frameset> interfaces itself, which doesn't seem better than hardcoding > their names. Aha, we have found it, this the key point on which we differ. I do think that putting the extended attribute on these 4 interfaces is better than hard-coding their names. (In reply to Darin Adler from comment #10) > Aha, we have found it, this the key point on which we differ. I do think > that putting the extended attribute on these 4 interfaces is better than > hard-coding their names. Seems like I came up with a solution we both will be happy with. Exporting the WPT while it compiles... Created attachment 447762 [details]
Patch
Introduce IsEventHandlerType helper, remove the hard-coding of interface names, and align IDLs with the spec.
Comment on attachment 447762 [details] Patch Hey Darin, Does the updated patch look to you like something we can land w/o a follow-up? I appreciate all your feedback! View in context: https://bugs.webkit.org/attachment.cgi?id=447762&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5437 > + my $eventListenerType = $attribute->type->name eq "OnErrorEventHandler" && (IsDOMGlobalObject($interface) || $attribute->extendedAttributes->{WindowEventHandler}) ? "JSErrorHandler" : "JSEventListener"; Usage of IsDOMGlobalObject here is forward-compatible & safe because there will be no EventHandler's introduced in new specs, as those are considered legacy, especially the ones of OnErrorEventHandler type. I will make sure to note this in ChangeLog. (In reply to Alexey Shvayka from comment #14) > Does the updated patch look to you like something we can land w/o a > follow-up? Yes. > I appreciate all your feedback! Thanks; I find it fun to try to tame the code generation code a bit and make it simpler over time. Cutting down special cases. And also, relying a bit more on C++ techniques that were not available when it was originally written. Separately, maybe someone should port it to Python at some point. *** Bug 256484 has been marked as a duplicate of this bug. *** Pull request: https://github.com/WebKit/WebKit/pull/13983 Committed 264190@main (cf847edd8255): <https://commits.webkit.org/264190@main> Reviewed commits have been landed. Closing PR #13983 and removing active labels. |