Rick Eyre (:reyre) Reporter
	Description • 11 years ago

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.

	Benjamin Chen [:bechen] Assignee
	Comment 1 • 8 years ago

I'm going to implement the "change" event soon.

	Ralph Giles (:rillian)
	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.

	Benjamin Chen [:bechen] 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.

	Ralph Giles (:rillian)
	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.

	Ralph Giles (:rillian)
	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);
```

	Ralph Giles (:rillian)
	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.

	Ralph Giles (:rillian)
	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.

	Ralph Giles (:rillian)
	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

	Benjamin Chen [:bechen] 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?

	Ralph Giles (:rillian)
	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.

	Pulsebot
	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

	Iris Hsiao [:ihsiao]
	Comment 17 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/d82676c65f7a

	Eric Shepherd [:sheppy]
	Comment 18 • 7 years ago

Is there any change here that affects documentation? Does this change have any effect that would be noticed by any web developers?

	Benjamin Chen [:bechen] Assignee
	Comment 19 • 7 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

	Eric Shepherd [:sheppy]
	Comment 20 • 7 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.

	Ralph Giles (:rillian)
	Comment 21 • 7 years ago

Thanks, sheppy!