[webkit-reviews] review granted: [Bug 234254] Automatically generate event handler content attribute maps so they are easier to maintain : [Attachment 447067] Patch

[bugzilla-daemon at webkit.org]

Alexey Shvayka <ashvayka at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 234254: Automatically generate event handler content attribute maps so they
are easier to maintain
https://bugs.webkit.org/show_bug.cgi?id=234254

Attachment 447067: Patch

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




--- Comment #5 from Alexey Shvayka <ashvayka at apple.com> ---
Comment on attachment 447067
  --> https://bugs.webkit.org/attachment.cgi?id=447067
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.