Closed Bug 702161 Opened 8 years ago Closed 8 years ago

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

Categories

(Toolkit :: Video/Audio Controls, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: jaws, Assigned: dseif)

References

Details

(Whiteboard: [good first bug][mentor=jwein][lang=js])

Attachments

(1 file, 7 obsolete files)

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|.
These changes should be made in /toolkit/content/widgets/videocontrols.xml
Whiteboard: [good first bug][mentor=jwein][lang=js]
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|.
I'm willing to take on this bug if someone can assign it to me
Assignee: nobody → david.c.seifried
Thanks David! Let me know if you have any questions on this bug or any other bugs. I'd be glad to help :)
Status: NEW → ASSIGNED
Thanks Jared, appreciate it :)
Attached file Early work, no tests (obsolete) —
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 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.
Attachment #589765 - Flags: feedback+
I don't think we should be worried with adding tests for this bug, but I do appreciate your attempts to add tests :)
Attached patch patch v2 (obsolete) — Splinter Review
Thanks for the feedback Jared.  I think I hit all of the points you outlined in this patch.  Thanks again for all your help :)
Attachment #589765 - Attachment is obsolete: true
Attachment #590083 - Flags: review?(jwein)
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.
Attachment #590083 - Flags: review?(jwein) → feedback+
Sorry about the styling, I think I fixed everything that I touched.
Attachment #590083 - Attachment is obsolete: true
Attachment #590088 - Flags: review?(jwein)
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 dolske@mozilla.com.
Attachment #590088 - Flags: review?(jwein) → feedback+
Attached patch Added user line and description (obsolete) — Splinter Review
Attachment #590088 - Attachment is obsolete: true
Attachment #590098 - Flags: review?(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.
Attachment #590098 - Flags: review?(dolske) → review+
Yea I like the idea of simplifying it even more. I'll make the changes and submit a new patch.
Attached patch Patch v5 (obsolete) — Splinter Review
Alright I moved the bind call into the helper function.
Attachment #590098 - Attachment is obsolete: true
Attachment #592877 - Flags: review?(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.
Attachment #592877 - Flags: review?(dolske) → review-
Attached patch Patch v6 (obsolete) — Splinter Review
Ah sorry about that, I think I addressed the issue now.
Attachment #592877 - Attachment is obsolete: true
Attachment #593275 - Flags: review?
review ping?
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.
Attachment #593275 - Flags: review? → review?(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.]
Attachment #593275 - Flags: review?(dolske) → review+
Attached patch Patch v7 (obsolete) — Splinter Review
Thanks for the review Dolske, I changed fnc to func so hopes that sit's better :)
Attachment #593275 - Attachment is obsolete: true
Attachment #596241 - Flags: review+
Keywords: checkin-needed
I'll go out on a limb and say that that try run says checkin-not-just-yet rather than checkin-needed :)
Keywords: checkin-needed
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( ... );
Attachment #596241 - Flags: review+
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.
Attachment #596241 - Flags: feedback+
Attached patch Patch v8Splinter Review
Alright sorry about that last patch haha.  Here is an updated patch with the correct changes.
Attachment #596241 - Attachment is obsolete: true
Attachment #596887 - Flags: review?(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?
Attachment #596887 - Flags: review?(dolske) → review+
(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.
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 :)
Whiteboard: [good first bug][mentor=jwein][lang=js] → [good first bug][mentor=jwein][lang=js][fixed-in-fx-team]
Thanks Jared, looking forward to doing some more bugs :)
https://hg.mozilla.org/mozilla-central/rev/27aed921df7e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=jwein][lang=js][fixed-in-fx-team] → [good first bug][mentor=jwein][lang=js]
Target Milestone: --- → mozilla13
Depends on: 790577
You need to log in before you can comment on or make changes to this bug.