Automatically generate event handler content attribute maps so they are easier to maintain
Created attachment 447061 [details] Patch
Created attachment 447067 [details] Patch
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.
(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.
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.
Oops, not passing tests. I need to check!
(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.
Created attachment 447092 [details] Patch
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].
<rdar://problem/86458873>
Very nice.