Bug 234254 – Automatically generate event handler content attribute maps so they are easier to maintain

• Component: DOM
• Status: RESOLVED FIXED

[Darin Adler]

Automatically generate event handler content attribute maps so they are easier to maintain

[Darin Adler]

Created attachment 447061 [details]
Patch

[Darin Adler]

Created attachment 447067 [details]
Patch

[Alexey Shvayka]

Comment on attachment 447061 [details]
Patch

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

Thank you for addressing this issue, Darin!

> Source/WebCore/html/HTMLElement.cpp:64
> +#include "JSHTMLElement.h"

Currently, we have event handler name map on HTMLElement, which is not quite right. GlobalEventHandlers are shared beyond HTML, like for example `SVGElement includes GlobalEventHandlers`. Moving the handlers down to JSHTMLElement.h doesn't seem like a direction I would prefer. It doesn't feel right to store them on any wrapper.

Do you think generating them into a separate file in preprocess-idls.pl would be more preferred approach? It would also let us avoid adding extended attributes like GenerateForEachEventHandlerContentAttribute.

[Darin Adler]

(In reply to Alexey Shvayka from comment #3)
> Currently, we have event handler name map on HTMLElement, which is not quite
> right. GlobalEventHandlers are shared beyond HTML, like for example
> `SVGElement includes GlobalEventHandlers`. Moving the handlers down to
> JSHTMLElement.h doesn't seem like a direction I would prefer. It doesn't
> feel right to store them on any wrapper.
> 
> Do you think generating them into a separate file in preprocess-idls.pl
> would be more preferred approach?

Sure, I suppose it would be OK, but adding more generated files will be more complex. We might even need to change the IDL parser around.

I think you, or someone else, should feel free to move this later, but I chose what I thought would be expedient, and I feel good about my choice. My patch is much smaller because I took advantage of what we already have.

I have a hard time finding any practical problem with this solution, or advantage to a more correct one. But if there is one I would welcome the improvement.

> It would also let us avoid adding extended
> attributes like GenerateForEachEventHandlerContentAttribute.

But it would do that by requiring that we hard code the name GlobalEventHandlers somewhere?

One of the good things in practice about what I did is how easy it was to reuse for HTMLBodyElement and the window event handlers.

[Alexey Shvayka]

Comment on attachment 447067 [details]
Patch

(In reply to Darin Adler from comment #4)
> (In reply to Alexey Shvayka from comment #3)
> > It would also let us avoid adding extended
> > attributes like GenerateForEachEventHandlerContentAttribute.
> 
> But it would do that by requiring that we hard code the name
> GlobalEventHandlers somewhere?

Yes, and I would argue that is a good thing. Currently, GenerateForEachEventHandlerContentAttribute considers all EventHandlers to be of GlobalEventHandlers & DocumentAndElementEventHandlers. In practice that's fine because SVGElement / MathMLElement are consistent.

> One of the good things in practice about what I did is how easy it was to
> reuse for HTMLBodyElement and the window event handlers.

preprocess-idls.pl solution would just gather all [WindowEventHandler] handlers when looping through interfaces, avoiding a bit weird call stacks like HTMLFrameSetElement::parseAttribute() => HTMLBodyElement::eventNameForWindowEventHandlerAttribute() => JSHTMLBodyElement::forEachEventHandlerContentAttribute().

> (In reply to Alexey Shvayka from comment #3)
> > Currently, we have event handler name map on HTMLElement, which is not quite
> > right. GlobalEventHandlers are shared beyond HTML, like for example
> > `SVGElement includes GlobalEventHandlers`. Moving the handlers down to
> > JSHTMLElement.h doesn't seem like a direction I would prefer. It doesn't
> > feel right to store them on any wrapper.
> > 
> > Do you think generating them into a separate file in preprocess-idls.pl
> > would be more preferred approach?
> 
> Sure, I suppose it would be OK, but adding more generated files will be more
> complex. We might even need to change the IDL parser around.
> 
> I think you, or someone else, should feel free to move this later, but I
> chose what I thought would be expedient, and I feel good about my choice. My
> patch is much smaller because I took advantage of what we already have.
> 
> I have a hard time finding any practical problem with this solution, or
> advantage to a more correct one. But if there is one I would welcome the
> improvement.

All true points, preprocess-idls.pl solution is a different time contribution w/o added practical benefit, only slightly improving visual similarity of our code with the spec.

[Darin Adler]

Oops, not passing tests. I need to check!

[Darin Adler]

(In reply to Alexey Shvayka from comment #5)
> > But it would do that by requiring that we hard code the name
> > GlobalEventHandlers somewhere?
> 
> Yes, and I would argue that is a good thing. Currently,
> GenerateForEachEventHandlerContentAttribute considers all EventHandlers to
> be of GlobalEventHandlers & DocumentAndElementEventHandlers. In practice
> that's fine because SVGElement / MathMLElement are consistent.

Sure, that sounds fine. I suppose GlobalEventHandlers can be in a list just like all the element classes are in a list, in the build system, but sad that we already have to have so many special lists like that.

> preprocess-idls.pl solution would just gather all [WindowEventHandler]
> handlers when looping through interfaces, avoiding a bit weird call stacks
> like HTMLFrameSetElement::parseAttribute() =>
> HTMLBodyElement::eventNameForWindowEventHandlerAttribute() =>
> JSHTMLBodyElement::forEachEventHandlerContentAttribute().

Sure, avoiding the strange call stacks is good. But collecting all [WindowEventHandler] from all IDL files sounds very strange. If this doesn’t come from a particular named interface, like GlobalEventHandlers, then it seems like we’d want some logic about which interfaces we’d look at, and why. Doesn’t make sense that if we labeled something with [WindowEventHandler] in another interface that HTMLBodyElement and HTMLFrameSetElement would just both sprout content event handlers for those automatically.

> All true points, preprocess-idls.pl solution is a different time
> contribution w/o added practical benefit, only slightly improving visual
> similarity of our code with the spec.

I think it’s fine to improve later. Glad you are OK with me landing this now (once I debug it and figure out why it’s doing the wrong thing). I could even write a FIXME about how this is slightly illogical but expedient.

[Darin Adler]

Created attachment 447092 [details]
Patch

[EWS]

Committed r287019 (245224@main): <https://commits.webkit.org/245224@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447092 [details].

[Radar WebKit Bug Importer]

<rdar://problem/86458873>

[Sam Weinig]

Very nice.