Closed
Bug 462114
Opened 16 years ago
Closed 15 years ago
Event handlers in video control XBL binding should self-terminate.
Categories
(Toolkit :: Video/Audio Controls, defect, P3)
Toolkit
Video/Audio Controls
Tracking
()
VERIFIED
FIXED
mozilla1.9.2a1
People
(Reporter: Dolske, Assigned: Dolske)
Details
(Keywords: verified1.9.1)
Attachments
(2 files, 2 obsolete files)
486 bytes,
text/html
|
Details | |
8.75 KB,
patch
|
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•15 years ago
|
||
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.
Assignee | ||
Comment 2•15 years ago
|
||
Assignee | ||
Comment 3•15 years ago
|
||
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)
Assignee | ||
Updated•15 years ago
|
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
Assignee | ||
Comment 5•15 years ago
|
||
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 6•15 years ago
|
||
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+
Assignee | ||
Comment 7•15 years ago
|
||
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.
Assignee | ||
Comment 8•15 years ago
|
||
Updated with nits.
Attachment #360168 -
Attachment is obsolete: true
Assignee | ||
Comment 9•15 years ago
|
||
Pushed http://hg.mozilla.org/mozilla-central/rev/6311be095706
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 10•15 years ago
|
||
Comment on attachment 364194 [details] [diff] [review] Patch v.3 a191=beltzner
Attachment #364194 -
Flags: approval1.9.1+
Assignee | ||
Comment 11•15 years ago
|
||
Pushed to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0596286f2b0b
Keywords: fixed1.9.1
Comment 12•15 years ago
|
||
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
Keywords: fixed1.9.1 → verified1.9.1
Assignee | ||
Updated•14 years ago
|
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.
Description
•