Event handlers in video control XBL binding should self-terminate.

VERIFIED FIXED in mozilla1.9.2a1

Status

()

defect
P3
normal
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

({verified1.9.1})

unspecified
mozilla1.9.2a1
Points:
---
Bug Flags:
blocking1.9.1 -
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

From bug 448909 comment 79 (and some discussion leading up to that)...

The event handlers added by the controls binding can outlive the binding (eg, when the <video> node is removed from the document). We should look at having them self-terminate, since they're no longer needed at that point.
Posted patch Patch v.1 (WIP) (obsolete) — Splinter Review
Something like this... Stole the <field name="alive"> trick from bug 206636, and moved the repeated addEventListener calls into loop with an array of events.

This is sufficient to make the listeners self-terminate, but the video controls don't work quite right when the binding is reattached. The construction needs to do a little work to extract the current state of the video, since the events it's listening for might have already been dispatched.

I'd like to spin off the code bug 462113 is adding into helper functions (so it can be reused here). I'll either do that in the next round of reviews for that bug, or do it here after that lands.
Posted patch Patch v.2 (obsolete) — Splinter Review
This makes the video controls self-terminate, even if another instance of the binding has since been re-added (only the older instances will self-terminate). The controls also pick up the correct initial state, so that if they are attached to a video that's already started playback the controls are as expected.
Attachment #357751 - Attachment is obsolete: true
Attachment #360168 - Flags: review?(gavin.sharp)
Flags: blocking1.9.1?
Target Milestone: mozilla1.9.1 → mozilla1.9.2a1
So this manifests as controls not working when the video is removed and re-added to the document, right? I don't think we need to block on this although we do definitely want a fix.
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Priority: -- → P3
Comment on attachment 360168 [details] [diff] [review]
Patch v.2

Oops. I figured gavin might get around to reviewing this while Enn was away, but we both seem to have forgotten about this patch. :)
Attachment #360168 - Flags: review?(gavin.sharp) → review?(enndeakin)
Comment on attachment 360168 [details] [diff] [review]
Patch v.2

Odd, but ok I guess. Why the 'alive' variable? Cannot randomID be used and initialized to some non-zero value?
Attachment #360168 - Flags: review?(enndeakin) → review+
Yeah, I'll just drop |alive|. I started out with only that, added the randomID to catch the case of a different binding being attached, and didn't merge them.
Posted patch Patch v.3Splinter Review
Updated with nits.
Attachment #360168 - Attachment is obsolete: true
Pushed http://hg.mozilla.org/mozilla-central/rev/6311be095706
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment on attachment 364194 [details] [diff] [review]
Patch v.3

a191=beltzner
Attachment #364194 - Flags: approval1.9.1+
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090413 Minefield/3.6a1pre
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090413 Shiretoko/3.5b4pre
Status: RESOLVED → VERIFIED
Component: Video/Audio → Video/Audio Controls
Product: Core → Toolkit
QA Contact: video.audio → video.audio
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.