Skip to content

Commit

Permalink
Utilize event deduping with addEventListener
Browse files Browse the repository at this point in the history
The initial leak found here
#761

And then patched here
957656d

Was never the "correct" fix. Yes the patch in
957656d cause us to no longer leak
however we were still paying the cost of registering and unregistering
for the same event over and over, as well as allocating a new bound
scope each time (thus generating more garbage and thus eventually
causing more time to be spent int GC)

Instead I opted here to take advantage of addEventListener and its
ability to automatically discard duplicate listeners.

> If multiple identical EventListeners are registered on the same
> EventTarget with the same parameters, the duplicate instances are
> discarded. They do not cause the EventListener to be called twice, and
> they do not need to be removed manually with the removeEventListener
> method.
https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#Multiple_identical_event_listeners

Because there is no outside pointers into this event, there is no need
to remove the listener anymore, because once the node reference becomes
unreachable the events will be automatically GCd for us.

:)
  • Loading branch information
samccone committed Dec 9, 2015
1 parent 9920165 commit 402ae63
Showing 1 changed file with 13 additions and 9 deletions.
22 changes: 13 additions & 9 deletions src/menu/menu.js
Original file line number Diff line number Diff line change
Expand Up @@ -338,21 +338,25 @@
}
};

/**
* cleanup function to remove animation listeners.
*
* @param {Event} evt
* @private
*/

MaterialMenu.prototype.removeAnimationEndListener_ = function(evt) {
evt.target.classList.remove(MaterialMenu.prototype.CssClasses_.IS_ANIMATING);
};

/**
* Adds an event listener to clean up after the animation ends.
*
* @private
*/
MaterialMenu.prototype.addAnimationEndListener_ = function() {
var cleanup = function() {
this.element_.removeEventListener('transitionend', cleanup);
this.element_.removeEventListener('webkitTransitionEnd', cleanup);
this.element_.classList.remove(this.CssClasses_.IS_ANIMATING);
}.bind(this);

// Remove animation class once the transition is done.
this.element_.addEventListener('transitionend', cleanup);
this.element_.addEventListener('webkitTransitionEnd', cleanup);
this.element_.addEventListener('transitionend', this.removeAnimationEndListener_);
this.element_.addEventListener('webkitTransitionEnd', this.removeAnimationEndListener_);
};

/**
Expand Down

0 comments on commit 402ae63

Please sign in to comment.