Opened 7 months ago

Closed 7 months ago

Last modified 5 months ago

#7202 closed change (fixed)

Add hasListeners method to EventEmitter

Reported by: mjethani Assignee: mjethani
Priority: P3 Milestone:
Module: Core Keywords:
Cc: Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29977555/

Description (last modified by mjethani)

Background

The extension contains a hasListener(tabId) method in lib/hitLogger.js. This should be done by Core instead, in lib/events.js, by keeping track of whether an event has any listeners. Most of the time the HitLogger object in the extension does not have any listeners for any events, therefore this could be optimized in Core to avoid a lookup in the internal _listeners map, simply by checking the value of its size property first.

What to change

In lib/events.js modify EventEmitter.prototype.off to remove any entry that has no listeners left.

Add a new method hasListeners(name) that returns true if there are any filters for the given event or, if no name is given, for any event.

Integration notes

The extension should modify its lib/hitLogger.js to redefine the hasListener method as follows:

function hasListener(tabId)
{
  return eventEmitter.hasListeners(tabId);
}

Hints

The new hasListener method is not used by the extension as of now. Please see the dependency update ticket for this change for hints on how to test this.

Change History (15)

comment:1 Changed 7 months ago by mjethani

  • Component changed from Unknown to Core

comment:2 Changed 7 months ago by mjethani

  • Review URL(s) modified (diff)

comment:3 Changed 7 months ago by mjethani

  • Description modified (diff)
  • Summary changed from Remove EventEmitter entries if there are no listeners left for an event to Add hasListeners method to EventEmitter

comment:4 Changed 7 months ago by mjethani

  • Description modified (diff)

comment:5 Changed 7 months ago by mjethani

  • Description modified (diff)

comment:6 Changed 7 months ago by mjethani

  • Blocking 7000 added

comment:7 Changed 7 months ago by sebastian

Thanks for filing this issue, and sorry for the confusion, but I don't think we need that after all.

comment:8 Changed 7 months ago by mjethani

Thanks for the clarification.

As I mentioned in the description, we could still use this because right now the extension is accessing the internal property _listeners. But we don't need to make any more changes, just call eventEmitter.hasListeners() in lib/hitLogger.js.

Last edited 7 months ago by mjethani (previous) (diff)

comment:9 Changed 7 months ago by abpbot

A commit referencing this issue has landed:
Issue 7202 - Add hasListeners method to EventEmitter

comment:10 Changed 7 months ago by mjethani

  • Description modified (diff)
  • Priority changed from Unknown to P3
  • Ready set

comment:11 Changed 7 months ago by mjethani

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from new to closed

comment:12 Changed 7 months ago by mjethani

  • Owner set to mjethani

comment:13 Changed 7 months ago by mjethani

  • Blocking 7000 removed

comment:14 Changed 6 months ago by abpbot

A commit referencing this issue has landed:
Issue 7202 - Add hasListeners method to EventEmitter

comment:15 Changed 5 months ago by rscott

  • Verified working set
Note: See TracTickets for help on using tickets.