Noissue - Use Map to store EventEmitter listeners

Noissue - Use Map to store EventEmitter listeners

[kzar, 2018-03-21 13:52:45]

Patch Set 1

I need this for #6402, but it seems to me a sensible change in itself.

[Manish Jethani, 2018-04-01 08:44:16]

https://codereview.adblockplus.org/29729594/diff/29729595/lib/events.js
File lib/events.js (right):

https://codereview.adblockplus.org/29729594/diff/29729595/lib/events.js#newco...
lib/events.js:39: if (this._listeners.has(name))
It's usually not worth calling Map.has if you're going to call Map.get anyway.
It would be better to do this instead:

  let listeners = this._listeners.get(name);
  if (listeners)
    listeners.push(listener);
  else
    this._listeners.set(name, [listener]);

https://codereview.adblockplus.org/29729594/diff/29729595/lib/events.js#newco...
lib/events.js:53: let listeners = this._listeners.get(name);
While we're at it, perhaps it would be a good idea to make listeners a Set
object. Two benefits: (1) no duplicate listeners (similar to other event APIs),
and (2) faster deletion.

[kzar, 2018-04-03 17:27:55]

Patch Set 2 : Avoid calling .get directly after .has

https://codereview.adblockplus.org/29729594/diff/29729595/lib/events.js
File lib/events.js (right):

https://codereview.adblockplus.org/29729594/diff/29729595/lib/events.js#newco...
lib/events.js:39: if (this._listeners.has(name))
On 2018/04/01 08:44:16, Manish Jethani wrote:
> It's usually not worth calling Map.has if you're going to call Map.get anyway.
> It would be better to do this instead:
> 
>   let listeners = this._listeners.get(name);
>   if (listeners)
>     listeners.push(listener);
>   else
>     this._listeners.set(name, [listener]);

Done.

https://codereview.adblockplus.org/29729594/diff/29729595/lib/events.js#newco...
lib/events.js:53: let listeners = this._listeners.get(name);
On 2018/04/01 08:44:16, Manish Jethani wrote:
> While we're at it, perhaps it would be a good idea to make listeners a 
> Set object. Two benefits: (1) no duplicate listeners (similar to other 
> event APIs), and (2) faster deletion.

Good idea, but I'd rather not since this would change behaviour subtly and if
we're unlucky lead to a regression somewhere. I would support the change in the
future if someone™ takes the time to verify nothing (including libadblockplus)
breaks!

[Sebastian Noack, 2018-04-03 20:42:35]

https://codereview.adblockplus.org/29729594/diff/29729595/lib/events.js
File lib/events.js (right):

https://codereview.adblockplus.org/29729594/diff/29729595/lib/events.js#newco...
lib/events.js:53: let listeners = this._listeners.get(name);
On 2018/04/03 17:27:55, kzar wrote:
> On 2018/04/01 08:44:16, Manish Jethani wrote:
> > While we're at it, perhaps it would be a good idea to make listeners a 
> > Set object. Two benefits: (1) no duplicate listeners (similar to other 
> > event APIs), and (2) faster deletion.
> 
> Good idea, but I'd rather not since this would change behaviour subtly and if
> we're unlucky lead to a regression somewhere. I would support the change in
the
> future if someone™ takes the time to verify nothing (including libadblockplus)
> breaks!

What change of behavior are you concerned about? Mind that Set() objects
preserve the order in which values are added. So the only difference in behavior
would be that if the same listener is attempted to be added more than once, it
will effectively only be registered once. I cannot think of any situation where
it would be intended that the same listener is called more than once. On the
other hand it would notably speedup listener removal (and simplifies the code a
bit). So I think the risk here (if any) would be justified.

FWIW, replacing a plain object with a Map() already introduces (similarly small)
changes in behavior, e.g. keys are no longer coerced to strings.

[Manish Jethani, 2018-04-04 06:54:49]

https://codereview.adblockplus.org/29729594/diff/29729595/lib/events.js
File lib/events.js (right):

https://codereview.adblockplus.org/29729594/diff/29729595/lib/events.js#newco...
lib/events.js:53: let listeners = this._listeners.get(name);
On 2018/04/03 20:42:34, Sebastian Noack wrote:
> On 2018/04/03 17:27:55, kzar wrote:
> > On 2018/04/01 08:44:16, Manish Jethani wrote:
> > > While we're at it, perhaps it would be a good idea to make listeners a 
> > > Set object. Two benefits: (1) no duplicate listeners (similar to other 
> > > event APIs), and (2) faster deletion.
> > 
> > Good idea, but I'd rather not since this would change behaviour subtly and
if
> > we're unlucky lead to a regression somewhere. I would support the change in
> the
> > future if someone™ takes the time to verify nothing (including
libadblockplus)
> > breaks!
> 
> What change of behavior are you concerned about? Mind that Set() objects
> preserve the order in which values are added. So the only difference in
behavior
> would be that if the same listener is attempted to be added more than once, it
> will effectively only be registered once. I cannot think of any situation
where
> it would be intended that the same listener is called more than once. On the
> other hand it would notably speedup listener removal (and simplifies the code
a
> bit). So I think the risk here (if any) would be justified.

Dave has a point that this would be more likely to break something. For example
every time you open a window you might add the same "close" listener, which
operates on the event's target and removes itself. The number of times it's
added would be equal to the number of windows open. Now this code would break
because the listener would get removed after the first window is closed.

So perhaps it should be a separate patch.

> FWIW, replacing a plain object with a Map() already introduces (similarly
small)
> changes in behavior, e.g. keys are no longer coerced to strings.

This is less serious in my opinion.

[Manish Jethani, 2018-04-04 06:57:43]

LGTM