Improve caption button behavior for video player.

Improve caption button behavior for video player.

- If there is only one caption, the CC button should just
toggle it on and off.
- If the track has no label, instead of showing Unknown,
it should show the language label and if that is also
empty, it should show Track X.

BUG=648696
Committed: https://crrev.com/480c4553694769458606f287350d13ff144a05e6
Cr-Commit-Position: refs/heads/master@{#429044}

[Finnur, 2016-10-28 10:44:56]

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

[commit-bot: I haz the power, 2016-10-28 10:45:01]

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

[Finnur, 2016-10-28 10:49:19]

Description was changed from

==========
Improve closed-caption button behavior for video player.

BUG=648696
==========

to

==========
Improve caption button behavior for video player.

BUG=648696
==========

[Finnur, 2016-10-28 10:49:20]

finnur@chromium.org changed reviewers:
+ mlamouri@chromium.org

[commit-bot: I haz the power, 2016-10-28 12:19:14]

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

[commit-bot: I haz the power, 2016-10-28 12:19:15]

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-10-28 13:02:37]

Would be great to test the following:
- with one text track:
 - when pressed, the closed caption button is toggled (change style/state)
 - when pressed, the track is visible
 - wher pressed again, the closed caption button is toggled (change style/state)
 - when pressed again, the track is no longer visible
- with one or more:
 - the label shows the language if no label is provided
 - the label shows "Track X" if no language and no label are provided
 - check an edge case where there are many tracks and has a label, one a
language and one none

https://codereview.chromium.org/2456993003/diff/1/content/app/strings/content...
File content/app/strings/content_strings.grd (right):

https://codereview.chromium.org/2456993003/diff/1/content/app/strings/content...
content/app/strings/content_strings.grd:889: Track <ph
name="NUMBER">$1<ex>1</ex></ph>
Maybe mention that the argument is the track number in the list of tracks.

https://codereview.chromium.org/2456993003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/2456993003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:482: }
Maybe this logic could be moved to the ClosedCaption button instead? Maybe
`disableShowingTextTracks()` and `showTextTrackAtIndex()` could move to a common
place like MediaControls to avoid re-implementing part of the logic in the
closed captions button?

[Finnur, 2016-10-28 14:05:15]

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

[commit-bot: I haz the power, 2016-10-28 14:05:40]

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

[Finnur, 2016-10-28 14:11:41]

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

[commit-bot: I haz the power, 2016-10-28 14:12:03]

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

[Finnur, 2016-10-28 14:12:22]

Done. Let me know if the refactoring is done right. In the meantime I'm working
on a test for the new functionality.

https://codereview.chromium.org/2456993003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/2456993003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:482: }
Yeah, I like that idea. Done.

[mlamouri (slow - plz ping), 2016-10-28 14:20:15]

Thanks :) I think the refactoring looks good with one comment below.

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

https://codereview.chromium.org/2456993003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.h:66: void
disableShowingTextTracks(TextTrackList*);
Instead of passing TextTrackList, could we get the text track from
m_mediaElement?

[commit-bot: I haz the power, 2016-10-28 15:38:05]

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

[commit-bot: I haz the power, 2016-10-28 15:38:06]

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_...)

[Finnur, 2016-10-28 16:28:38]

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

[commit-bot: I haz the power, 2016-10-28 16:29:01]

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-10-28 17:51:04]

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

[commit-bot: I haz the power, 2016-10-28 17:51:06]

Dry run: This issue passed the CQ dry run.

[Finnur, 2016-10-29 11:19:04]

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

[commit-bot: I haz the power, 2016-10-29 11:19:15]

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

[Finnur, 2016-10-29 11:22:17]

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

[commit-bot: I haz the power, 2016-10-29 11:22:25]

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-10-29 12:20:40]

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

[commit-bot: I haz the power, 2016-10-29 12:20:41]

Dry run: This issue passed the CQ dry run.

[Finnur, 2016-10-29 16:36:10]

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

[commit-bot: I haz the power, 2016-10-29 16:36:22]

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

[Finnur, 2016-10-29 16:37:39]

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

[commit-bot: I haz the power, 2016-10-29 16:37:51]

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

[Finnur, 2016-10-29 16:41:31]

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

[commit-bot: I haz the power, 2016-10-29 16:41:42]

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

[Finnur, 2016-10-29 16:44:32]

Patchset #6 (id:100001) has been deleted

[Finnur, 2016-10-29 16:45:18]

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

https://codereview.chromium.org/2456993003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.h:66: void
disableShowingTextTracks(TextTrackList*);
Yes, indeed. Much better. PTAL.

[commit-bot: I haz the power, 2016-10-29 18:02:16]

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

[commit-bot: I haz the power, 2016-10-29 18:02:17]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2016-10-31 10:23:13]

Looks great, thanks! :)

lgtm with one minor comment. Can you also update the description to maybe be a
bit more verbose about what is being done in the CL :)

https://codereview.chromium.org/2456993003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControls.h (right):

https://codereview.chromium.org/2456993003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.h:37: class
TextTrackList;
You no longer need this forward declaration.

[Finnur, 2016-11-01 15:27:35]

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

[commit-bot: I haz the power, 2016-11-01 15:27:57]

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

[Finnur, 2016-11-01 15:32:24]

Description was changed from

==========
Improve caption button behavior for video player.

BUG=648696
==========

to

==========
Improve caption button behavior for video player.

- If there is only one caption, the CC button should just toggle it on and off.
- If the track has no label, instead of showing Unknown, it should show the
language label and if that is also empty, it should show Track X.

BUG=648696
==========

[Finnur, 2016-11-01 15:32:24]

finnur@chromium.org changed reviewers:
+ piman@chromium.org

[Finnur, 2016-11-01 15:32:58]

Description was changed from

==========
Improve caption button behavior for video player.

- If there is only one caption, the CC button should just toggle it on and off.
- If the track has no label, instead of showing Unknown, it should show the
language label and if that is also empty, it should show Track X.

BUG=648696
==========

to

==========
Improve caption button behavior for video player.

- If there is only one caption, the CC button should just
toggle it on and off.
- If the track has no label, instead of showing Unknown, 
it should show the language label and if that is also 
empty, it should show Track X.

BUG=648696
==========

[Finnur, 2016-11-01 15:33:32]

Done. Antoine, mind reviewing the small change to content_strings.grd?

[piman, 2016-11-01 15:52:49]

lgtm

[commit-bot: I haz the power, 2016-11-01 17:44:50]

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

[commit-bot: I haz the power, 2016-11-01 17:44:51]

Dry run: This issue passed the CQ dry run.

[Finnur, 2016-11-01 17:48:43]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2016-11-01 17:48:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2456993003/#ps140001
(title: "Addresss feedback")

[commit-bot: I haz the power, 2016-11-01 17:49:05]

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-11-01 17:53:29]

Description was changed from

==========
Improve caption button behavior for video player.

- If there is only one caption, the CC button should just
toggle it on and off.
- If the track has no label, instead of showing Unknown, 
it should show the language label and if that is also 
empty, it should show Track X.

BUG=648696
==========

to

==========
Improve caption button behavior for video player.

- If there is only one caption, the CC button should just
toggle it on and off.
- If the track has no label, instead of showing Unknown, 
it should show the language label and if that is also 
empty, it should show Track X.

BUG=648696
==========

[commit-bot: I haz the power, 2016-11-01 17:53:33]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-11-01 18:24:39]

Description was changed from

==========
Improve caption button behavior for video player.

- If there is only one caption, the CC button should just
toggle it on and off.
- If the track has no label, instead of showing Unknown, 
it should show the language label and if that is also 
empty, it should show Track X.

BUG=648696
==========

to

==========
Improve caption button behavior for video player.

- If there is only one caption, the CC button should just
toggle it on and off.
- If the track has no label, instead of showing Unknown,
it should show the language label and if that is also
empty, it should show Track X.

BUG=648696

Committed: https://crrev.com/480c4553694769458606f287350d13ff144a05e6
Cr-Commit-Position: refs/heads/master@{#429044}
==========

[commit-bot: I haz the power, 2016-11-01 18:24:41]

Patchset 7 (id:??) landed as
https://crrev.com/480c4553694769458606f287350d13ff144a05e6
Cr-Commit-Position: refs/heads/master@{#429044}