Clean up `EventListenerManager::AddEventListenerInternal` with `switch`
Categories
(Core :: DOM: Events, enhancement, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox96 | --- | fixed |
People
(Reporter: masayuki, Assigned: masayuki)
References
Details
Attachments
(14 files)
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review |
It now has a lot of if
- else if
s and compare pointers of nsAtom
. However, all of them are mapped to EventMessage
, thus, we can make the code simpler.
Luckily, my experimental patches do not cause any performance regression on tryserver:
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=26d3174ce6aebfd881c4bfcafec017cdb98957e8&newProject=try&newRevision=b3689f19030348fc4d60ed3784f2d6a81191a36b&framework=10&page=1&showOnlyImportant=1
Assignee | ||
Comment 1•3 years ago
|
||
In theory, switch
statement is faster than if
- else if
s especially when
there are a lot of else if
s. Although it may be optimized by the compilers
in these days, but they may have same performance even in the worst case.
So if we can rewrite the big if
- else if
block in
EventListenerManager::AddEventListenerInternal()
with a switch
statement,
it may become faster and anyway looks much simpler.
A lot of them compares an nsAtom
pointer and nsGkAtoms
, and the others
compares EventMessage
. Fortunately, it treats only known events which
are registered both in EventMessage
and nsGkAtoms
. Therefore, we can
use switch
statement with EventMessage
.
For detecting unexpected regressions, this and the following patches put
NS_ASSERTION
s into the default
block. Unfortunately, we cannot output
dynamic values like the value of variables with MOZ_ASSERT
, we need to
use NS_ASSERTION
here. See bug 779173.
Depends on D131750
Assignee | ||
Comment 2•3 years ago
|
||
Current code firstly check whether the given event message is a pointer event
message or not, but it does not necessary for switch
statement. Thus, we
can make it simpler with the switch
statement.
Depends on D131756
Assignee | ||
Comment 3•3 years ago
|
||
Depends on D131757
Assignee | ||
Comment 4•3 years ago
|
||
Depends on D131758
Assignee | ||
Comment 5•3 years ago
|
||
Oddly, nsContentUtils
does not register touch events in desktop environments
which do have a touch screen into the map of EventMessage
and nsGkAtoms
.
However, AddEventListenerInternal()
needs to cache whether there are some
touch event listeners since touch screen may be connected later. Therefore,
this patch makes AddEventListenerInternal()
adjusts aEventMessage
only
when given event message is eUnidentifiedEvent
and given nsAtom
is one
of the touch events before the switch
statement. I guess that the dynamical
registration is for some handlers of touch events, but from the point of view
of EventListenerManager
, there should be another way to check whether touch
events may be fired or not, and the mapping table should be same in any
environments. However, it's out of scope of this cleaning up bug and
fortunately, we can skip the additional check with the eUnidentifiedEvent
check and nsAtom::IsStatic()
check.
Depends on D131759
Assignee | ||
Comment 6•3 years ago
|
||
Depends on D131760
Assignee | ||
Comment 7•3 years ago
|
||
Depends on D131761
Assignee | ||
Comment 8•3 years ago
|
||
Depends on D131762
Assignee | ||
Comment 9•3 years ago
|
||
Depends on D131763
Assignee | ||
Comment 10•3 years ago
|
||
Depends on D131764
Assignee | ||
Comment 11•3 years ago
|
||
Depends on D131765
Assignee | ||
Comment 12•3 years ago
|
||
Depends on D131766
Assignee | ||
Comment 13•3 years ago
|
||
Depends on D131767
Assignee | ||
Comment 14•3 years ago
|
||
Depends on D131768
Comment 15•3 years ago
|
||
Comment 16•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6e7596255bd0
https://hg.mozilla.org/mozilla-central/rev/82ac9416cd39
https://hg.mozilla.org/mozilla-central/rev/4fb45f5cd3c3
https://hg.mozilla.org/mozilla-central/rev/49d2cf7a138b
https://hg.mozilla.org/mozilla-central/rev/36c02a5f1577
https://hg.mozilla.org/mozilla-central/rev/a72f1ea12741
https://hg.mozilla.org/mozilla-central/rev/7ea03729fff3
https://hg.mozilla.org/mozilla-central/rev/6819df524734
https://hg.mozilla.org/mozilla-central/rev/4810f4a305cf
https://hg.mozilla.org/mozilla-central/rev/9774d93127d5
https://hg.mozilla.org/mozilla-central/rev/b25d0d0cce20
https://hg.mozilla.org/mozilla-central/rev/f3fafdfe78d0
https://hg.mozilla.org/mozilla-central/rev/0b5f194a5e3c
https://hg.mozilla.org/mozilla-central/rev/b2075a9d8174
Description
•