702161 - videocontrols.xml has anonymous function event listeners that are added but never removed

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

videocontrols.xml has anonymous function event listeners that are added but never removed.

this.muteButton.addEventListener("command", function() { self.toggleMute(); }, false);
this.playButton.addEventListener("command", function() { self.togglePause(); }, false);
this.controlsSpacer.addEventListener("click", function(e) { if (e.button == 0) { self.togglePause(); } }, false);
this.fullscreenButton.addEventListener("command", function() { this.muteButton.addEventListener("mouseover",  function(e) { self.onVolumeMouseInOut(e); }, false);
this.muteButton.addEventListener("mouseout",   function(e) { self.onVolumeMouseInOut(e); }, false);
this.volumeStack.addEventListener("mouseover", function(e) { self.onVolumeMouseInOut(e); }, false);
this.volumeStack.addEventListener("mouseout",  function(e) { self.onVolumeMouseInOut(e); }, false);
this.videocontrols.addEventListener("transitionend", function(e) { self.onTransitionEnd(e); }, false);

These should be removed in |terminateEventListeners|.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

These changes should be made in /toolkit/content/widgets/videocontrols.xml

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

For most of these, we can simply store the bound function (created using |fnc.bind(this)|), but we will probably want something more elegant than a bunch of separate properties attached to |this|.

Comment 3

[David Seifried (:dseif)]

I'm willing to take on this bug if someone can assign it to me

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Thanks David! Let me know if you have any questions on this bug or any other bugs. I'd be glad to help :)

Comment 5

[David Seifried (:dseif)]

Thanks Jared, appreciate it :)

Comment 6

[David Seifried (:dseif)]

Attachment: Early work, no tests

Just putting this up to get some feedback and to make sure I'm on the right track. No tests yet, been sort of stumped on how to approach them.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 589765 [details]
Early work, no tests

David: I haven't tested this but it looks like a good approach. Can you see about a way to reduce some of the duplication here?

Maybe we could use an object with the names of the events as the keys and the bound events as the values? This would allow us to do a for loop over the key/value pairs in the object when doing the removeEventListener/delete pattern.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

I don't think we should be worried with adding tests for this bug, but I do appreciate your attempts to add tests :)

Comment 9

[David Seifried (:dseif)]

Attachment: patch v2

Thanks for the feedback Jared.  I think I hit all of the points you outlined in this patch.  Thanks again for all your help :)

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 590083 [details] [diff] [review]
patch v2

Review of attachment 590083 [details] [diff] [review]:
-----------------------------------------------------------------

David: Nice work on the patch. Can you make these changes and upload a new version?

::: toolkit/content/widgets/videocontrols.xml
@@ +609,5 @@
>                      }
>                  },
>  
>                  terminateEventListeners : function () {
> +

nit: we can remove this empty line and some of the other empty lines that were added, just try to follow the style of the existing code.

@@ +623,5 @@
> +                      var element = this.controlListeners[ i ];
> +                      element.item.removeEventListener( element.event, element.fnc, false );
> +                    }
> +                    
> +                    delete this.controlListeners;

we can use |for each (var element in this.controlListeners)| so that we don't need to have the extra line that does |var element = this.controlListeners[i];|. See https://developer.mozilla.org/en/JavaScript/Reference/Statements/for_each...in for more information.

@@ +1289,5 @@
>  
> +                    this.controlListeners = [];
> +
> +                    // Helper function to add an event listener to the given element
> +                    function addListener( elem, eventName, func ) {

Nice, I like your helper function here :)

@@ +1295,5 @@
> +                      self.controlListeners.push({ item: elem, event: eventName, fnc: func });
> +                      elem.addEventListener( eventName, func, false );
> +                    }
> +
> +                    addListener( this.muteButton, "command", this.toggleMute.bind(this) ); 

nit: no space between the opening and closing parenthesis in the function calls and no blank line after an opening curly brace.

Comment 11

[David Seifried (:dseif)]

Attachment: Third patch, fixes styling issues mostly

Sorry about the styling, I think I fixed everything that I touched.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 590088 [details] [diff] [review]
Third patch, fixes styling issues mostly

Looks good! One last request from me, can you update your patch to have the summary and user properly set?

You can follow the first two bullet points of this blog post for help: http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

After you make those changes, you can request review from [e-mail].

Comment 13

[David Seifried (:dseif)]

Attachment: Added user line and description

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

dolske: review ping?

Comment 15

[Justin Dolske [:Dolske]]

Comment on attachment 590098 [details] [diff] [review]
Added user line and description

Review of attachment 590098 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay.

::: toolkit/content/widgets/videocontrols.xml
@@ +1288,5 @@
> +                    // Helper function to add an event listener to the given element
> +                    function addListener(elem, eventName, func) {
> +                      self.controlListeners.push({ item: elem, event: eventName, fnc: func });
> +                      elem.addEventListener(eventName, func, false);
> +                    }

Suggestion...

What if we add |func = func.bind(self)| to the helper? That should all the callers to be simplified even more. EG |addListener(thing, "stuff", this.toggleFoo);|.

I'll take it either way, though.

Comment 16

[David Seifried (:dseif)]

Yea I like the idea of simplifying it even more. I'll make the changes and submit a new patch.

Comment 17

[David Seifried (:dseif)]

Attachment: Patch v5

Alright I moved the bind call into the helper function.

Comment 18

[Justin Dolske [:Dolske]]

Comment on attachment 592877 [details] [diff] [review]
Patch v5

Review of attachment 592877 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/videocontrols.xml
@@ +1291,1 @@
>                        elem.addEventListener(eventName, func, false);

Oops, that won't work. |foo.fnc| and |func| are different, you need to make sure the same reference is used in the cached object and the addEventListener.

Comment 19

[David Seifried (:dseif)]

Attachment: Patch v6

Ah sorry about that, I think I addressed the issue now.

Comment 20

[David Seifried (:dseif)]

review ping?

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 593275 [details] [diff] [review]
Patch v6

I think this didn't get attention because it was missing an assigned reviewer. I've added Dolske back as the reviewer for the patch.

Comment 22

[Justin Dolske [:Dolske]]

Comment on attachment 593275 [details] [diff] [review]
Patch v6

(Sorry, missed the initial request in general bugmail.)

>+                    // Helper function to add an event listener to the given element
>+                    function addListener(elem, eventName, func) {
>+                      var listeners = self.controlListeners;
>+                      listeners.push({ item: elem, event: eventName, fnc: func.bind(self) });
>+                      elem.addEventListener(eventName, listeners[ listeners.length - 1 ].fnc, false);
>+                    }

Nit: this would be simpler as:

    function addListener(elem, eventName, func) {
      let boundFunc = func.bind(self);
      self.controlListeners.push({ item: elem, event: eventName, fnc: boundFunc });
      elem.addEventListener(eventName, boundFunc, false);
    }

r+ with that.

[Now that I look again I make a little funny face at "fnc", but meh.]

Comment 23

[David Seifried (:dseif)]

Attachment: Patch v7

Thanks for the review Dolske, I changed fnc to func so hopes that sit's better :)

Comment 24

[Jon Buckley]

try server: https://tbpl.mozilla.org/?tree=Try&rev=4045ebd90cb0

Comment 25

[Phil Ringnalda (:philor)]

I'll go out on a limb and say that that try run says checkin-not-just-yet rather than checkin-needed :)

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 596241 [details] [diff] [review]
Patch v7

Review of attachment 596241 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/videocontrols.xml
@@ +1329,5 @@
> +
> +                    // Helper function to add an event listener to the given element
> +                    function addListener(elem, eventName, func) {
> +                      let boundFunc = func.bind(self);
> +                      listeners.push({ item: elem, event: eventName, func: boundFunc });

I think this should be: this.controlListeners.push( ... );

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 596241 [details] [diff] [review]
Patch v7

Review of attachment 596241 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/videocontrols.xml
@@ +1329,5 @@
> +
> +                    // Helper function to add an event listener to the given element
> +                    function addListener(elem, eventName, func) {
> +                      let boundFunc = func.bind(self);
> +                      listeners.push({ item: elem, event: eventName, func: boundFunc });

Whoops, I'm an idiot. Based on all the places that addListener is called, this.controlListeners should work, but it's probably better to use |self.controlsListeners.push( ... );| as Justin mentioned above.

Comment 28

[David Seifried (:dseif)]

Attachment: Patch v8

Alright sorry about that last patch haha.  Here is an updated patch with the correct changes.

Comment 29

[Jon Buckley]

try server: https://tbpl.mozilla.org/?tree=Try&rev=2576e303b294

Comment 30

[Justin Dolske [:Dolske]]

Comment on attachment 596887 [details] [diff] [review]
Patch v8

Review of attachment 596887 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/videocontrols.xml
@@ +1335,5 @@
> +                    }
> +
> +                    addListener(this.muteButton, "command", this.toggleMute.bind(this)); 
> +                    addListener(this.playButton, "command", this.togglePause.bind(this));
> +                    addListener(this.fullscreenButton, "command", this.toggleFullscreen.bind(this));

So... Very... Close... :)

There's no need for the |.bind(this)| in these 3 calls; addListener() is already doing that, so boundFunc ends up being double-bound.

r+ with this change, maybe jared can just do that as he lands it?

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Justin Dolske [:Dolske] from comment #30) 
> r+ with this change, maybe jared can just do that as he lands it?

Yeah, I'll make the change when I land this.

Comment 32

[Jared Wein [:jaws] (please needinfo? me)]

Pushed to fx-team repository. This should get merged to mozilla-central within a day or two.

https://hg.mozilla.org/integration/fx-team/rev/27aed921df7e

Thanks for sticking with this :)

Comment 33

[David Seifried (:dseif)]

Thanks Jared, looking forward to doing some more bugs :)

Comment 34

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/27aed921df7e