1586014 - Factor out RecompileScriptEventListeners

Status: RESOLVED FIXED

(Core :: DOM: Events, enhancement)

Description

[Frédéric Wang (:fredw)]

RecompileScriptEventListeners is implemented very similarly in SVG, HTML, XUL and (after bug 1571487) MathML. We should try to avoid duplicate code.

Comment 1

[Frédéric Wang (:fredw)]

Checking now... First, nsXULElement::RecompileScriptEventListeners can be modified to just call "IsEventAttributeName(attr)" (without the XUL namespace parameter).

Then the only difference between

SVGElement::RecompileScriptEventListeners
MathMLElement::RecompileScriptEventListeners
nsGenericHTMLElement::RecompileScriptEventListeners
nsXULElement::RecompileScriptEventListeners

is that SVG uses GetEventNameForAttr(attr).

One can easily declare a default Element::GetEventNameForAttr function that just returns its parameter and override it in the SVGElement class. However per bug 1571487 comment 5, maybe we just want to drop this renaming...

Comment 2

[Somdatta]

Hello,
I am Somdatta and I am an outreachy applicant.I would like to work on this bug.

Comment 3

[Somdatta]

Could you please clarify the following statement-
One can easily declare a default Element::GetEventNameForAttr function that just returns its parameter and override it in the SVGElement class. However per bug 1571487 comment 5, maybe we just want to drop this renaming...

Comment 4

[Frédéric Wang (:fredw)]

(In reply to Somdatta from comment #3)

Could you please clarify the following statement-
One can easily declare a default Element::GetEventNameForAttr function that just returns its parameter and override it in the SVGElement class.

However per bug 1571487 comment 5, maybe we just want to drop this renaming...

Hi, yes so SVG has SVGElement::GetEventNameForAttr has some special renaming, for example <svg onload="..."> will register an onSVGLoad event.

https://searchfox.org/mozilla-central/rev/daa5c195e5efe45a63bf79f97ae77419b16f62f1/dom/svg/SVGElement.cpp#1487

So IIRC, I was thinking create a method

Element::GetEventNameForAttr(nsAtom* aAttr) { return aAttr; }

then

either (a) override it in SVGElement::GetEventNameForAttr to keep the special renaming
or (b) wait clarification on the spec side, hopefully we don't need this kind of renaming https://github.com/w3c/svgwg/issues/467

That said, it seems since then Boris introduced Element::GetEventNameForAttr in bug 1607918, so you can probably just directly merge the RecompileScriptEventListeners implementations now.

(would still be nice to clarify the event thing in the SVG spec though)

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

First off, I don't think this bug should change any behavior.

It makes sense to me to make Element::GetEventNameForAttr virtual rather than static, and make Element::RecompileScriptEventListeners non-virtual. That would lead to slightly more virtual calls (one more per event handler attribute), but it's only called from nsINode::CloneAndAdopt, so that's probably fine.

Comment 6

[Somdatta]

Attachment: Bug 1586014- Factor out RecompileScriptEventListeners r?emilio

Comment 7

[Somdatta]

Hello,I am an outreachy applicant.Actually you(Emilio) reviewed my revision for this bug and requested some changes.I have made the required changes.Tomorrow(5th nov) is the deadline for my final application.So,I was wondering if you could look into the revision so that if it is okay,I can add this.Thanks in advance@emilio

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1586014 - Minor follow-up: don't do unnecessary attribute lookups. r=smaug

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/043b85dea615
Factor out RecompileScriptEventListeners r=emilio

Comment 10

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/043b85dea615

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/19d60f319a66
Minor follow-up: don't do unnecessary attribute lookups. r=smaug

Comment 12

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/19d60f319a66