Closed
Bug 882674
Opened 12 years ago
Closed 8 years ago
[webvtt] Add Pending Text Track Change notification flag to the Media Element
Categories
(Core :: Audio/Video: Playback, defect)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: reyre, Assigned: bechen)
References
()
Details
(Keywords: dev-doc-complete)
Attachments
(1 file)
Basically tells us if we've fired an event called 'change' when a text track in the media elements list of text track changes and if so, not to fire it again.
I don't know if we really need this at the moment, but at the minimum we need to ensure that when a text track does change in the media elements list the 'change' event is being fired.
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Updated•9 years ago
|
Component: Audio/Video → Audio/Video: Playback
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bechen
Updated•8 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 1•8 years ago
|
||
I'm going to implement the "change" event soon.
Comment hidden (mozreview-request) |
Comment 3•8 years ago
|
||
mozreview-review |
Comment on attachment 8787109 [details]
Bug 882674 - Implement "pending text track change notification flag".
https://reviewboard.mozilla.org/r/75986/#review74514
Thanks for continuing to work on this. Please address comments and let me see it again.
::: dom/media/TextTrackList.cpp:162
(Diff revision 1)
> + : TrackEventRunner(aList, aEvent)
> + {}
> +
> + NS_IMETHOD Run() override
> + {
> + mList->mPendingTextTrackChange = false;
You unset the pending flag here, when we're dispatched to the main thread, but set it below in `TextTrackList::CreateAndDispatchChangeEvent()` which seems like it could race. If they're both supposed to run only on the main thread, perhaps add asserts for that?
::: dom/media/test/test_texttracklist.html:31
(Diff revision 1)
> track = video.textTracks[0];
> video.textTracks.addEventListener("change", changed);
>
> is(track.mode, "hidden", "New TextTrack's mode should be hidden.");
> track.mode = "showing";
> +track.mode = "hidden";
Is the idea there to queue two mode changes faster than the event loop can spin, and verify we only get one? Is it clear the event loop won't spin between these two?
We end the test as soon as we finish checking the first changed() event, so it doesn't seem like adding this is enough. Seems like you we need to do some third thing and verify we only got a single changed() event in-between.
Attachment #8787109 -
Flags: review?(giles) → review-
Assignee | ||
Comment 4•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8787109 [details]
Bug 882674 - Implement "pending text track change notification flag".
https://reviewboard.mozilla.org/r/75986/#review74514
> You unset the pending flag here, when we're dispatched to the main thread, but set it below in `TextTrackList::CreateAndDispatchChangeEvent()` which seems like it could race. If they're both supposed to run only on the main thread, perhaps add asserts for that?
The mPendingTextTrackChange accessed by main thread only, I'll add asserion.
> Is the idea there to queue two mode changes faster than the event loop can spin, and verify we only get one? Is it clear the event loop won't spin between these two?
>
> We end the test as soon as we finish checking the first changed() event, so it doesn't seem like adding this is enough. Seems like you we need to do some third thing and verify we only got a single changed() event in-between.
If we receive multiple change events in the testcase, the testcase will failed and dump message "call SimpleTest.finish() multiple times". So I think the modification is ok.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 7•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8787109 [details]
Bug 882674 - Implement "pending text track change notification flag".
https://reviewboard.mozilla.org/r/75986/#review74514
> If we receive multiple change events in the testcase, the testcase will failed and dump message "call SimpleTest.finish() multiple times". So I think the modification is ok.
Hmm. I'm sure it works sometimes, but I'm having trouble convincing myself this is always true. Can you explain why the second event handler must run before the test is cleaned up? It looks like SimpleTest.finish() starts calling cleanup tasks and generates a report synchronously.
Comment 8•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8787109 [details]
Bug 882674 - Implement "pending text track change notification flag".
https://reviewboard.mozilla.org/r/75986/#review74514
> Hmm. I'm sure it works sometimes, but I'm having trouble convincing myself this is always true. Can you explain why the second event handler must run before the test is cleaned up? It looks like SimpleTest.finish() starts calling cleanup tasks and generates a report synchronously.
After further investigation, I think this isn't a good solution.
Relying on `SimpleTest.finish` to catch that the event handler
has been called twice is safe if the `change` event is fired
synchronously, because everything runs in order. But if the
event is fired asynchronously, there is no guarantee both
handlers will be invoked before the test is finished.
It seems the spec doesn't actually say if this event should be
synchronous or asynchronous ("queue a task to fire a simple event...")
but our implementation is asynchronous, and it is specified as
asynchronous for AudioTrackList and VideoTrackList. In any case,
I'd like the test to robust to either variant in case it changes
in the future.
I filed https://github.com/whatwg/html/issues/1788 about the spec issue.
We can protect ourselves from calling finish too soon by appending
the call to the event loop with `setTimeout(func, 0)`.
Something like:
```
SimpleTest.waitForExplicitFinish();
// ...other setup...
video.textTracks.addEventListener("change", changed);
is(track.mode, "hidden", "New TextTrack's mode should be hidden.");
track.mode = "showing";
track.mode = "hidden";
var event_count = 0;
function changed(event) {
event_count += 1;
is(event_count, 1, "change event dispatched multple times.");
// ...other checks...
// Queue finishing the test after any other pending events.
setTimeout(SimpleTest.finish, 0);
```
Comment 9•8 years ago
|
||
Ah, the event is spec'd as async. https://html.spec.whatwg.org/multipage/embedded-content.html#pending-text-track-change-notification-flag uses the "Queue a task that...fires a simple event" language.
Comment hidden (mozreview-request) |
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8787109 [details]
Bug 882674 - Implement "pending text track change notification flag".
https://reviewboard.mozilla.org/r/75986/#review78510
Marking r- based on the previous issue. Please ask for review again when you've had a chance to address the comment.
Attachment #8787109 -
Flags: review?(giles) → review-
Comment 12•8 years ago
|
||
mozreview-review |
Comment on attachment 8787109 [details]
Bug 882674 - Implement "pending text track change notification flag".
https://reviewboard.mozilla.org/r/75986/#review78512
Assignee | ||
Comment 13•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8787109 [details]
Bug 882674 - Implement "pending text track change notification flag".
https://reviewboard.mozilla.org/r/75986/#review78510
At the latest patch, I have modify the testcase as you suggest |setTimeout(SimpleTest.finish, 0)|. Anything else need to modify?
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(giles)
Comment 14•8 years ago
|
||
mozreview-review |
Comment on attachment 8787109 [details]
Bug 882674 - Implement "pending text track change notification flag".
https://reviewboard.mozilla.org/r/75986/#review78708
This should be sufficient with the parentheses removed. Thanks!
::: dom/media/test/test_texttracklist.html:46
(Diff revisions 3 - 4)
> ok(!event.bubbles, "change event should not bubble.");
> ok(event.isTrusted, "change event should be trusted.");
> ok(!event.cancelable, "change event should not be cancelable.");
>
> - SimpleTest.finish();
> + // Delay the finish function call for testing the change event count.
> + setTimeout(SimpleTest.finish(), 0);
You need to pass SimpleTest.finish directly here. With the parentheses, this invokes the finish function inline and then queues the return value to execute later.
Attachment #8787109 -
Flags: review- → review+
Updated•8 years ago
|
Flags: needinfo?(giles)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 16•8 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d82676c65f7a
Implement "pending text track change notification flag". r=rillian
Keywords: checkin-needed
Comment 17•8 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 12 years ago → 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment 18•8 years ago
|
||
Is there any change here that affects documentation? Does this change have any effect that would be noticed by any web developers?
Updated•8 years ago
|
Flags: needinfo?(bechen)
Assignee | ||
Comment 19•8 years ago
|
||
Could you point me out where the document is?
I didn't see the TextTrack/TextTrackLest object under:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/track
https://developer.mozilla.org/en-US/docs/Web/API/Web_Video_Text_Tracks_Format
https://developer.mozilla.org/en-US/docs/Web/API/TextTrack
The behavior change is that:
If js change the mode of TextTrack multiple times in one iteration, the TextTrackList object which contain the TextTrack will receive only one "change" event. Before the code change, the TextTrackList object will receive multiple "change" events.
https://html.spec.whatwg.org/multipage/embedded-content.html#pending-text-track-change-notification-flag
Flags: needinfo?(bechen)
Comment 20•8 years ago
|
||
I’ve now created the following pages:
https://developer.mozilla.org/en-US/docs/Web/API/TextTrack
Only one of the subpages has been written so far, and that was done expressly to document this bug:
https://developer.mozilla.org/en-US/docs/Web/API/TextTrack/mode
This change is also now included on Firefox 52 for developers.
Now then, looking over the VTT stuff, I know there’s a good bit to do yet, but that will need to wait until later, unless someone wants to volunteer to do it. But this specific issue is resolved.
Keywords: dev-doc-needed → dev-doc-complete
Comment 21•8 years ago
|
||
Thanks, sheppy!
You need to log in
before you can comment on or make changes to this bug.
Description
•