Closed Bug 1273511 Opened 5 years ago Closed 5 years ago

Optimize EventListenerManager::AddEventListenerByType


(Core :: DOM: Events, defect)

36 Branch
Not set



Tracking Status
firefox49 --- fixed


(Reporter: smaug, Assigned: smaug)



(Whiteboard: btpp-active)


(2 files)

It does too many hashtable lookups and temporary string creations etc.
Also atomizations, free() calls... Lots of stuff in there.
yup, it all reduces to one hashtable lookup.
Attached patch patchSplinter Review

This reduces the temporary string creation and hashtable lookups.
Attachment #8753390 - Flags: review?(masayuki)
Comment on attachment 8753390 [details] [diff] [review]

I don't like adding bool argument because it's not unclear from caller side. But I have no better idea.
Attachment #8753390 - Flags: review?(masayuki) → review+
yeah, the method call is a bit ugly, but luckily this is used rarely. And since the method takes weird params anyhow, one needs to look at the documentation each time one uses it.
Whiteboard: btpp-active
Hmm, I need to look at the SVG/SMIL events. I may have made a mistake there.
We pass the test current, and with this patch, but the previous patch didn't.
And yes, SVG/SMIL events are crazy.

(looks like try is closed atm)
Attachment #8754545 - Flags: review?(masayuki)
Comment on attachment 8754545 [details] [diff] [review]
v2 + tests for SMIL/SVG

>+  if (sStringEventTable->Get(aName, &mapping)) {
>+    if (mapping.mMaybeSpecialSVGorSMILEvent) {
>+      // Try the atom version so that we should get the right message for
>+      // SVG/SMIL.
>+      atom = NS_Atomize(NS_LITERAL_STRING("on") + aName);
>+      msg = GetEventMessage(atom);

The events are not so many, so, it might be faster to use switch statement instead of hashtable. However, I guess that the hashtable's cost is not so better than switch statement and this is stronger for maintain.

I think that we should make the SVG/SMIL events constructed as usual in another bug, though.
Attachment #8754545 - Flags: review?(masayuki) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.