Media Controls: Use events to update controls for closed captions.

Media Controls: Use events to update controls for closed captions.

This is using addtrack, removetrack and change events on the TextTrackList exposed
on the media element to be notified and update the closed captions button. Because
the events are asynchronous, some tests have to be modified to be asynchronous.
They have been moved to media/controls/ too.

There is one hook that was added in order to notify the controls that an
HTMLTrackElement failed to load. There is no clear way to figure that out otherwise.
An HTML issue has been opened: https://github.com/whatwg/html/issues/2121

BUG=662761
Committed: https://crrev.com/a069837e9fe584243f357ca535ca0bebd0b7de85
Cr-Commit-Position: refs/heads/master@{#436321}

[mlamouri (slow - plz ping), 2016-11-30 18:29:39]

Description was changed from

==========
Media Controls: Use events to update controls for closed captions.

WORK IN PROGRESS.

<TODO>

BUG=<TODO>
==========

to

==========
Media Controls: Use events to update controls for closed captions.

This is using addtrack, removetrack and change events on the TextTrackList
exposed
on the media element to be notified and update the closed captions button.
Because
the events are asynchronous, some tests have to be modified to be asynchronous.
They have been moved to media/controls/ too.

There is one hook that was added in order to notify the controls that an
HTMLTrackElement failed to load. There is no clear way to figure that out
otherwise.
An HTML issue has been opened: https://github.com/whatwg/html/issues/2121

BUG=662761
==========

[mlamouri (slow - plz ping), 2016-11-30 18:29:43]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 18:30:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mlamouri (slow - plz ping), 2016-11-30 18:30:26]

mlamouri@chromium.org changed reviewers:
+ foolip@chromium.org, zqzhang@chromium.org

[mlamouri (slow - plz ping), 2016-11-30 18:30:27]

zqzhang@ for local review.
foolip@ mostly FYI regarding the spec issue.

[commit-bot: I haz the power, 2016-11-30 20:24:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 20:24:10]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[mlamouri (slow - plz ping), 2016-12-01 10:43:17]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 10:43:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 11:58:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 11:58:11]

Dry run: This issue passed the CQ dry run.

[Zhiqiang Zhang (Slow), 2016-12-01 12:20:46]

lgtm w/ nits

https://codereview.chromium.org/2539023002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/controls/closed-captions-dynamic-update.html
(right):

https://codereview.chromium.org/2539023002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/closed-captions-dynamic-update.html:39:
addUnloadableHTMLTrackElement();
nit: This test seems too large to me. Maybe you can split the test steps from
here into another test?

https://codereview.chromium.org/2539023002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/closed-captions-dynamic-update.html:76:
});
nit: If the test is split, it's better to add description to each test

https://codereview.chromium.org/2539023002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/controls/closed-captions-single-track.html
(right):

https://codereview.chromium.org/2539023002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/closed-captions-single-track.html:43:
// Captions track should become visible after the closed caption button is
pressed.
s/visible/invisible

https://codereview.chromium.org/2539023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right):

https://codereview.chromium.org/2539023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.h:154: void
onTextTracksAddedRemoved();
nit: s/AddedRemoved/AddedOrRemoved?

[mlamouri (slow - plz ping), 2016-12-01 22:22:54]

zqzhang comments applied.

foolip@, PTAL

https://codereview.chromium.org/2539023002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/controls/closed-captions-dynamic-update.html
(right):

https://codereview.chromium.org/2539023002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/closed-captions-dynamic-update.html:39:
addUnloadableHTMLTrackElement();
On 2016/12/01 at 12:20:46, Zhiqiang Zhang wrote:
> nit: This test seems too large to me. Maybe you can split the test steps from
here into another test?

Because I moved the test from media/ to media/controls/ with some cleanup, I
prefer not to change the test that much.

https://codereview.chromium.org/2539023002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/closed-captions-dynamic-update.html:76:
});
On 2016/12/01 at 12:20:46, Zhiqiang Zhang wrote:
> nit: If the test is split, it's better to add description to each test

Ack.

https://codereview.chromium.org/2539023002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/controls/closed-captions-single-track.html
(right):

https://codereview.chromium.org/2539023002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/closed-captions-single-track.html:43:
// Captions track should become visible after the closed caption button is
pressed.
On 2016/12/01 at 12:20:46, Zhiqiang Zhang wrote:
> s/visible/invisible

Done.

https://codereview.chromium.org/2539023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right):

https://codereview.chromium.org/2539023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.h:154: void
onTextTracksAddedRemoved();
On 2016/12/01 at 12:20:46, Zhiqiang Zhang wrote:
> nit: s/AddedRemoved/AddedOrRemoved?

Done.

[foolip, 2016-12-02 11:29:17]

lgtm % nits

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/media/video-controls-overflow-menu-updates-appropriately.html
(right):

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/media/video-controls-overflow-menu-updates-appropriately.html:53:
setTimeout(t.step_func_done(_ => {
TIL that setTimeout.length==1, but is the default value 0? It's implemented in
v8 so I dunno.

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/controls/closed-captions-dynamic-update.html
(right):

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/closed-captions-dynamic-update.html:20:
assert_equals(video.textTracks[0].mode, "disabled");
Also assert_equals(video.textTracks[1].mode, "disabled")?

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/closed-captions-dynamic-update.html:31:
// Remove DOM node representing the track element.
The DOM node is the element, so "Remove the track elements".

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/closed-captions-dynamic-update.html:41:
setTimeout(t.step_func(_ => {
Comment that this is guaranteed to run before the end of the test, so the reader
need not be concerned. Or move addUnloadableHTMLTrackElement() inside if that
would work.

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/closed-captions-dynamic-update.html:55:
track.setAttribute("kind", "captions");
Don't really need kind or srclang?

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/controls/closed-captions-on-off.html
(right):

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/closed-captions-on-off.html:17:
for (var i = 0; i < captions.length; i++) {
for (var caption of captions) would work.

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/closed-captions-on-off.html:22: //
The controls are updated asynchronously.
Maybe after each of these comments, update that it *hasn't* updated yet, so that
the tests can be simplified if the implementation changes.

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/closed-captions-on-off.html:28:
assert_equals(video.textTracks[0].mode, "hidden");
and video.textTracks[1].mode?

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/controls/closed-captions-single-track.html
(right):

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/closed-captions-single-track.html:2:
<title>A single track should not show overflow on caption button-press, instead
just toggle.</title>
Analogous issues in this file, won't comment on those.

[mlamouri (slow - plz ping), 2016-12-05 15:17:56]

Comments applied. FWIW, these tests were moved so not sure if they needed so
much reviewing :)

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/media/video-controls-overflow-menu-updates-appropriately.html
(right):

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/media/video-controls-overflow-menu-updates-appropriately.html:53:
setTimeout(t.step_func_done(_ => {
On 2016/12/02 at 11:29:17, foolip wrote:
> TIL that setTimeout.length==1, but is the default value 0? It's implemented in
v8 so I dunno.

As far as I know, setTimeout default value is 0.

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/controls/closed-captions-dynamic-update.html
(right):

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/closed-captions-dynamic-update.html:20:
assert_equals(video.textTracks[0].mode, "disabled");
On 2016/12/02 at 11:29:17, foolip wrote:
> Also assert_equals(video.textTracks[1].mode, "disabled")?

Done.

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/closed-captions-dynamic-update.html:31:
// Remove DOM node representing the track element.
On 2016/12/02 at 11:29:17, foolip wrote:
> The DOM node is the element, so "Remove the track elements".

Done.

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/closed-captions-dynamic-update.html:41:
setTimeout(t.step_func(_ => {
On 2016/12/02 at 11:29:17, foolip wrote:
> Comment that this is guaranteed to run before the end of the test, so the
reader need not be concerned. Or move addUnloadableHTMLTrackElement() inside if
that would work.

I removed the function.

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/closed-captions-dynamic-update.html:55:
track.setAttribute("kind", "captions");
On 2016/12/02 at 11:29:17, foolip wrote:
> Don't really need kind or srclang?

Sure.

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/controls/closed-captions-on-off.html
(right):

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/closed-captions-on-off.html:17:
for (var i = 0; i < captions.length; i++) {
On 2016/12/02 at 11:29:17, foolip wrote:
> for (var caption of captions) would work.

It doesn't :(

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/closed-captions-on-off.html:22: //
The controls are updated asynchronously.
On 2016/12/02 at 11:29:17, foolip wrote:
> Maybe after each of these comments, update that it *hasn't* updated yet, so
that the tests can be simplified if the implementation changes.

Sure.

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/closed-captions-on-off.html:28:
assert_equals(video.textTracks[0].mode, "hidden");
On 2016/12/02 at 11:29:17, foolip wrote:
> and video.textTracks[1].mode?

Done.

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/controls/closed-captions-single-track.html
(right):

https://codereview.chromium.org/2539023002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/closed-captions-single-track.html:2:
<title>A single track should not show overflow on caption button-press, instead
just toggle.</title>
On 2016/12/02 at 11:29:17, foolip wrote:
> Analogous issues in this file, won't comment on those.

Ack.

[mlamouri (slow - plz ping), 2016-12-05 15:18:15]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2016-12-05 15:18:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
zqzhang@chromium.org, foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2539023002/#ps100001
(title: "review comments")

[commit-bot: I haz the power, 2016-12-05 15:18:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 16:44:00]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480951095812580,
"parent_rev": "0e274cb67778035d44408e0187b155988a0063c6", "commit_rev":
"c6930200448df40a6d6e400a1eed05e785ef99c3"}

[commit-bot: I haz the power, 2016-12-05 16:44:23]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-05 16:46:04]

Description was changed from

==========
Media Controls: Use events to update controls for closed captions.

This is using addtrack, removetrack and change events on the TextTrackList
exposed
on the media element to be notified and update the closed captions button.
Because
the events are asynchronous, some tests have to be modified to be asynchronous.
They have been moved to media/controls/ too.

There is one hook that was added in order to notify the controls that an
HTMLTrackElement failed to load. There is no clear way to figure that out
otherwise.
An HTML issue has been opened: https://github.com/whatwg/html/issues/2121

BUG=662761
==========

to

==========
Media Controls: Use events to update controls for closed captions.

This is using addtrack, removetrack and change events on the TextTrackList
exposed
on the media element to be notified and update the closed captions button.
Because
the events are asynchronous, some tests have to be modified to be asynchronous.
They have been moved to media/controls/ too.

There is one hook that was added in order to notify the controls that an
HTMLTrackElement failed to load. There is no clear way to figure that out
otherwise.
An HTML issue has been opened: https://github.com/whatwg/html/issues/2121

BUG=662761

Committed: https://crrev.com/a069837e9fe584243f357ca535ca0bebd0b7de85
Cr-Commit-Position: refs/heads/master@{#436321}
==========

[commit-bot: I haz the power, 2016-12-05 16:46:05]

Patchset 6 (id:??) landed as
https://crrev.com/a069837e9fe584243f357ca535ca0bebd0b7de85
Cr-Commit-Position: refs/heads/master@{#436321}

[foolip, 2016-12-06 15:39:54]

On 2016/12/05 15:17:56, mlamouri wrote:
> Comments applied. FWIW, these tests were moved so not sure if they needed so
> much reviewing :)

Oops :( Probably because of the file names, I just assumed obsolete tests were
deleted and new ones were written.