Closed Bug 1273511 Opened 5 years ago Closed 5 years ago

Optimize EventListenerManager::AddEventListenerByType

Categories

(Core :: DOM: Events, defect)

36 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

Details

(Whiteboard: btpp-active)

Attachments

(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
https://treeherder.mozilla.org/#/jobs?repo=try&revision=05b3bdb6e24afbc24c8f208f9ceb21f9e42dc785

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

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.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d47a2084eda
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+
https://hg.mozilla.org/mozilla-central/rev/b2974fa22af9
Status: NEW → RESOLVED
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.