Closed Bug 882674 Opened 7 years ago Closed 3 years ago

[webvtt] Add Pending Text Track Change notification flag to the Media Element

Categories

(Core :: Audio/Video: Playback, defect)

defect
Not set

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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Component: Audio/Video → Audio/Video: Playback
Assignee: nobody → bechen
I'm going to implement the "change" event soon.
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-
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 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 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);
```
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 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 on attachment 8787109 [details]
Bug 882674 - Implement "pending text track change notification flag".

https://reviewboard.mozilla.org/r/75986/#review78512
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?
Flags: needinfo?(giles)
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+
Flags: needinfo?(giles)
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/d82676c65f7a
Status: REOPENED → RESOLVED
Closed: 7 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Is there any change here that affects documentation? Does this change have any effect that would be noticed by any web developers?
Flags: needinfo?(bechen)
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)
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.
Thanks, sheppy!
You need to log in before you can comment on or make changes to this bug.