[webkit-reviews] review granted: [Bug 234568] [WebIDL] document.body.onerror setter should setup a five-parameter listener : [Attachment 447733] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Shvayka <ashvayka at apple.com>'s
request for review:
Bug 234568: [WebIDL] document.body.onerror setter should setup a five-parameter
listener
https://bugs.webkit.org/show_bug.cgi?id=234568

Attachment 447733: Patch

https://bugs.webkit.org/attachment.cgi?id=447733&action=review




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 447733
  --> https://bugs.webkit.org/attachment.cgi?id=447733
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.