Support text track selection in video controls

Support text track selection in video controls

Enable the user to select between text tracks with a menu that is displayed when the CC button is clicked.

BUG=353105, 495851
TEST=run-webkit-tests media*

Committed: https://crrev.com/c060c876bf0fb1aafc6fe0cab783583142e20007
Cr-Commit-Position: refs/heads/master@{#389201}

[srivats, 2015-04-11 00:56:02]

srivats@amazon.com changed reviewers:
+ avi@chromium.org, sky@chromium.org

[srivats, 2015-04-11 00:57:12]

srivats@amazon.com changed reviewers:
+ philipj@opera.com

[srivats, 2015-04-11 01:00:28]

Hi all,

I've added strings for the text track selection menu in Blink. Here's the
corresponding Blink CR: https://codereview.chromium.org/1082533002/
PTAL

avi: content/
sky: mojo/services/html_viewer/
philipj: fyi

Thanks,
Sunitha

[Avi (use Gerrit), 2015-04-13 21:10:40]

lgtm

Owner stamp; get media reviewer OK.

https://codereview.chromium.org/1079323002/diff/1/content/child/blink_platfor...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/1079323002/diff/1/content/child/blink_platfor...
content/child/blink_platform_impl.cc:865: {
"mediaplayerTrackSelectionCheckmark",
No space after { to match context

[srivats, 2015-04-16 23:39:27]

https://codereview.chromium.org/1079323002/diff/1/content/child/blink_platfor...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/1079323002/diff/1/content/child/blink_platfor...
content/child/blink_platform_impl.cc:865: {
"mediaplayerTrackSelectionCheckmark",
On 2015/04/13 21:10:39, Avi wrote:
> No space after { to match context

Done.

[philipj_slow, 2015-04-23 13:53:37]

https://codereview.chromium.org/1079323002/diff/40001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/1079323002/diff/40001/content/child/blink_pla...
content/child/blink_platform_impl.cc:879: {"mediaplayerTrackSelectionCheckmark",
Is there a bitmap to go with this too?

[philipj_slow, 2015-04-23 14:02:45]

https://codereview.chromium.org/1079323002/diff/40001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/1079323002/diff/40001/content/child/blink_pla...
content/child/blink_platform_impl.cc:879: {"mediaplayerTrackSelectionCheckmark",
On 2015/04/23 13:53:37, philipj_UTC2 wrote:
> Is there a bitmap to go with this too?

Oh, it's in https://codereview.chromium.org/1082533002/

Looks like these recently moved to Blink. It's a bit unfortunate that any
Chromium-side changes are needed for something that looks to be entirely
Blink-internal, but oh well.

[srivats, 2015-04-27 20:08:30]

srivats@amazon.com changed reviewers:
- sky@chromium.org

[srivats, 2016-02-23 01:33:50]

Description was changed from

==========
Support text track selection in video controls

Add strings for the text track selection menu in Blink.

Blink-side CL: https://codereview.chromium.org/1082533002/

BUG=353105
==========

to

==========
Support text track selection in video controls

Enable the user to select between text tracks with a menu that is displayed when
the CC button is clicked.

BUG=353105

TEST=run-webkit-tests media*
==========

[srivats, 2016-02-23 01:33:50]

srivats@amazon.com changed reviewers:
+ fs@opera.com, msw@chromium.org

[srivats, 2016-02-23 01:37:24]

Hey all,

Due to the Blink repo merge, this code review is now combined with:
https://codereview.chromium.org/1082533002/

Added msw to review: components/html_viewer/blink_resource_constants.h

Added fs to review: third_party/WebKit changes.

Thanks,
Sunitha

[fs, 2016-02-23 13:16:44]

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/media-controls.js (right):

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/media-controls.js:140: function
textTrackListItem(video, index)
...AtIndex? ...WithIndex?

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/text-track-selection-menu-add-track.html
(right):

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/text-track-selection-menu-add-track.html:17:
testExpected("track.mode", "hidden");
Using the longer expression "video.textTracks[1].mode" here might make sense.
Similarly below (then you don't need to put |track| on the global object.)

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/text-track-selection-menu-add-track.html:26:
testExpected("track.mode", "showing");
Verify that [0] is now 'disabled' too?

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/text-track-selection-menu-add-track.html:35:
track.mode = "hidden";
'hidden' is the default in this case.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/text-track-selection-menu-multiple-tracks.html
(right):

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/text-track-selection-menu-multiple-tracks.html:21:
testExpected("video.textTracks[" + i + "].mode", "showing");
Check that other tracks are disabled?

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/text-track-selection-menu-multiple-tracks.html:33:
track.mode = "hidden";
'hidden' is the default.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp
(right):

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp:109:
bool MediaControlElement::isHidden()
Maybe put this just next to updateShownState, so that it's easy to "glance" that
they are in synch.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:35: #include
"core/dom/DOMTokenList.h"
What needs this?

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:411: int
trackIndex = toElement(target)->getAttribute(trackIndexAttr).toInt(&validIndex);
getIntegralAttribute

(loses the validity checking but that isn't much use anyway)

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:413: if
(trackIndex != trackIndexOffValue) {
Nit: { } not needed

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:422:
HTMLDivElement::defaultEventHandler(event);
MediaControlDivElement (unless there's a reason to skip the ancestors between
here and HTMLDivElement - which there doesn't seem to be.)

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:427:
disableTextTracks();
Could we hoist this out into callers instead? (Else this would be
showTextTrackAtIndexAndDisableOtherTracks.)

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:429: if
(trackList && trackList->length() == 0)
I guess you want: !trackList || trackList->length() == 0 instead, but I think
that the textTracks() accessor guarantees a non-null return value so
null-checking is likely not needed at all.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:434: if
(track && track->isRenderable())
Since we know indexToEnable is in range, the null-check should not be needed
(the list will not store null TextTrack*s)

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:436:
mediaElement().disableAutomaticTextTrackSelection();
It could make sense to do this in the caller too (to avoid unnecessary
side-effects.)

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:439: void
MediaControlTextTrackListElement::disableTextTracks()
disableShowingTextTracks?

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:477:
trackItemInput->setAttribute(trackIndexAttr, AtomicString::number(trackIndex),
ASSERT_NO_EXCEPTION);
setIntegralAttribute

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:506:
removeChildren();
removeChildren(OmitSubtreeModifiedEvent)

I'm guessing (we don't want/need events from this.)

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:510:
Document& document = this->document();
Could just use document() directly in the single place that needs it?

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:512:
tracksSection->setAttribute(roleAttr, radiogroupAttr.localName());
I'm on the fence on whether this is "too smart" or not =)

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:521:
RefPtrWillBeRawPtr<Element> trackItem = createTextTrackListItem(track);
No need for this temporary.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.h (right):

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:164: String
getTextTrackLabel(TextTrack*);
Nit: This looks a bit too "spacious", so maybe group this one with the following
three of something.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:175: 
Nit: The norm here seems to be single blank lines after the separator, so drop
one of these.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:156: //     |
+-MediaControlTextTrackListHeader            
(-internal-media-controls-text-track-list-header)
Maybe add some indication that there can one/zero or more of some of these.
Likewise either/or etc.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:157: //     |
+-MediaControlTextTrackListItem              
(-internal-media-controls-text-track-list-item)
uNit: I think we've used '\' for these kinds of "turns" elsewhere.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:471:
m_textTrackList->setIsWanted(false);
I'm not sure the "is wanted" mechanism is really appropriate for this list? I
mean it probably works, but it sort of suggests that it operates within the same
"constraints" system as the control buttons - which I don't think it does? Maybe
hide this semantic twist in a helper method?

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/track/TextTrack.cpp (right):

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/track/TextTrack.cpp:446: bool
TextTrack::isRenderable()
Maybe move this closer to isRendered() and format it similarly so that it
becomes easy to see what they have in common and how they differ? Maybe it
should be "canBeRendered()" to avoid it looking to similar to isRendered() (and
to provide some form of relation between them)?

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/track/TextTrack.cpp:448: // A track can be
displayed when it's of kind captions or subtitles and hasn't failed to load
uNit: Add full stop.

[msw, 2016-02-23 18:17:02]

https://codereview.chromium.org/1079323002/diff/80001/components/html_viewer/...
File components/html_viewer/blink_resource_constants.h (right):

https://codereview.chromium.org/1079323002/diff/80001/components/html_viewer/...
components/html_viewer/blink_resource_constants.h:47:
{"mediaplayerClosedCaptionsIcon", IDR_MEDIAPLAYER_CLOSEDCAPTIONS_ICON},
This file no longer exists, please sync and rebase, then you likely don't need
my review.

[srivats, 2016-02-24 05:20:15]

srivats@amazon.com changed reviewers:
- msw@chromium.org

[srivats, 2016-02-24 05:20:18]

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/media-controls.js (right):

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/media-controls.js:140: function
textTrackListItem(video, index)
On 2016/02/23 13:16:43, fs wrote:
> ...AtIndex? ...WithIndex?

Done.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/text-track-selection-menu-add-track.html
(right):

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/text-track-selection-menu-add-track.html:17:
testExpected("track.mode", "hidden");
On 2016/02/23 13:16:43, fs wrote:
> Using the longer expression "video.textTracks[1].mode" here might make sense.
> Similarly below (then you don't need to put |track| on the global object.)

Done.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/text-track-selection-menu-add-track.html:26:
testExpected("track.mode", "showing");
On 2016/02/23 13:16:43, fs wrote:
> Verify that [0] is now 'disabled' too?

Done.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/text-track-selection-menu-add-track.html:35:
track.mode = "hidden";
On 2016/02/23 13:16:43, fs wrote:
> 'hidden' is the default in this case.

Done.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/text-track-selection-menu-multiple-tracks.html
(right):

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/text-track-selection-menu-multiple-tracks.html:21:
testExpected("video.textTracks[" + i + "].mode", "showing");
On 2016/02/23 13:16:43, fs wrote:
> Check that other tracks are disabled?

Done.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/text-track-selection-menu-multiple-tracks.html:33:
track.mode = "hidden";
On 2016/02/23 13:16:43, fs wrote:
> 'hidden' is the default.

Changed it to disabled so I can perform the disabled check in your above
comment.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp
(right):

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.cpp:109:
bool MediaControlElement::isHidden()
On 2016/02/23 13:16:43, fs wrote:
> Maybe put this just next to updateShownState, so that it's easy to "glance"
that
> they are in synch.

Done.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:35: #include
"core/dom/DOMTokenList.h"
On 2016/02/23 13:16:43, fs wrote:
> What needs this?

Removed the stale import

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:411: int
trackIndex = toElement(target)->getAttribute(trackIndexAttr).toInt(&validIndex);
On 2016/02/23 13:16:44, fs wrote:
> getIntegralAttribute
> 
> (loses the validity checking but that isn't much use anyway)

Done.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:413: if
(trackIndex != trackIndexOffValue) {
On 2016/02/23 13:16:43, fs wrote:
> Nit: { } not needed

Done.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:422:
HTMLDivElement::defaultEventHandler(event);
On 2016/02/23 13:16:44, fs wrote:
> MediaControlDivElement (unless there's a reason to skip the ancestors between
> here and HTMLDivElement - which there doesn't seem to be.)

Done.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:427:
disableTextTracks();
On 2016/02/23 13:16:44, fs wrote:
> Could we hoist this out into callers instead? (Else this would be
> showTextTrackAtIndexAndDisableOtherTracks.)

Done.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:429: if
(trackList && trackList->length() == 0)
On 2016/02/23 13:16:44, fs wrote:
> I guess you want: !trackList || trackList->length() == 0 instead, but I think
> that the textTracks() accessor guarantees a non-null return value so
> null-checking is likely not needed at all.

Done.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:434: if
(track && track->isRenderable())
On 2016/02/23 13:16:44, fs wrote:
> Since we know indexToEnable is in range, the null-check should not be needed
> (the list will not store null TextTrack*s)

Done.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:436:
mediaElement().disableAutomaticTextTrackSelection();
On 2016/02/23 13:16:43, fs wrote:
> It could make sense to do this in the caller too (to avoid unnecessary
> side-effects.)

Done.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:439: void
MediaControlTextTrackListElement::disableTextTracks()
On 2016/02/23 13:16:44, fs wrote:
> disableShowingTextTracks?

Done.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:477:
trackItemInput->setAttribute(trackIndexAttr, AtomicString::number(trackIndex),
ASSERT_NO_EXCEPTION);
On 2016/02/23 13:16:43, fs wrote:
> setIntegralAttribute

Done.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:506:
removeChildren();
On 2016/02/23 13:16:44, fs wrote:
> removeChildren(OmitSubtreeModifiedEvent)
> 
> I'm guessing (we don't want/need events from this.)

Done.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:510:
Document& document = this->document();
On 2016/02/23 13:16:43, fs wrote:
> Could just use document() directly in the single place that needs it?

Done.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:521:
RefPtrWillBeRawPtr<Element> trackItem = createTextTrackListItem(track);
On 2016/02/23 13:16:43, fs wrote:
> No need for this temporary.

Done.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.h (right):

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:164: String
getTextTrackLabel(TextTrack*);
On 2016/02/23 13:16:44, fs wrote:
> Nit: This looks a bit too "spacious", so maybe group this one with the
following
> three of something.

Done.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:175: 
On 2016/02/23 13:16:44, fs wrote:
> Nit: The norm here seems to be single blank lines after the separator, so drop
> one of these.

Done.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:156: //     |
+-MediaControlTextTrackListHeader            
(-internal-media-controls-text-track-list-header)
On 2016/02/23 13:16:44, fs wrote:
> Maybe add some indication that there can one/zero or more of some of these.
> Likewise either/or etc.

Would a comment be appropriate here?
The structure looks very simple with no comments for any of the other elements.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:157: //     |
+-MediaControlTextTrackListItem              
(-internal-media-controls-text-track-list-item)
On 2016/02/23 13:16:44, fs wrote:
> uNit: I think we've used '\' for these kinds of "turns" elsewhere.

Done.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:471:
m_textTrackList->setIsWanted(false);
On 2016/02/23 13:16:44, fs wrote:
> I'm not sure the "is wanted" mechanism is really appropriate for this list? I
> mean it probably works, but it sort of suggests that it operates within the
same
> "constraints" system as the control buttons - which I don't think it does?
Maybe
> hide this semantic twist in a helper method?

Added a wrapper that calls setIsWanted within that. I still feel this is a bit
redundant though.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/track/TextTrack.cpp (right):

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/track/TextTrack.cpp:446: bool
TextTrack::isRenderable()
On 2016/02/23 13:16:44, fs wrote:
> Maybe move this closer to isRendered() and format it similarly so that it
> becomes easy to see what they have in common and how they differ? Maybe it
> should be "canBeRendered()" to avoid it looking to similar to isRendered()
(and
> to provide some form of relation between them)?

Done.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/track/TextTrack.cpp:448: // A track can be
displayed when it's of kind captions or subtitles and hasn't failed to load
On 2016/02/23 13:16:44, fs wrote:
> uNit: Add full stop.

Done.

[srivats, 2016-02-24 05:20:47]

On 2016/02/23 18:17:02, msw wrote:
>
https://codereview.chromium.org/1079323002/diff/80001/components/html_viewer/...
> File components/html_viewer/blink_resource_constants.h (right):
> 
>
https://codereview.chromium.org/1079323002/diff/80001/components/html_viewer/...
> components/html_viewer/blink_resource_constants.h:47:
> {"mediaplayerClosedCaptionsIcon", IDR_MEDIAPLAYER_CLOSEDCAPTIONS_ICON},
> This file no longer exists, please sync and rebase, then you likely don't need
> my review.

Done

[fs, 2016-02-24 10:14:57]

This mostly looks LG-TM (a few minor nits still), but I imagine philipj might
want the final word =)

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:156: //     |
+-MediaControlTextTrackListHeader            
(-internal-media-controls-text-track-list-header)
On 2016/02/24 at 05:20:17, srivats wrote:
> On 2016/02/23 13:16:44, fs wrote:
> > Maybe add some indication that there can one/zero or more of some of these.
> > Likewise either/or etc.
> 
> Would a comment be appropriate here?
> The structure looks very simple with no comments for any of the other
elements.

Could be something like:

N*MediaControlTextTrackList...

or

| <for each renderable track>
+-MediaControlTextTrackList...

or

+-MediaControlTextTrackList...<0>
+-...
+-MediaControlTextTrackList...<N-1>

or some variant thereof. Bonus points if it actually somewhat captures the
actual DOM structure.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:471:
m_textTrackList->setIsWanted(false);
On 2016/02/24 at 05:20:18, srivats wrote:
> On 2016/02/23 13:16:44, fs wrote:
> > I'm not sure the "is wanted" mechanism is really appropriate for this list?
I
> > mean it probably works, but it sort of suggests that it operates within the
same
> > "constraints" system as the control buttons - which I don't think it does?
Maybe
> > hide this semantic twist in a helper method?
> 
> Added a wrapper that calls setIsWanted within that. I still feel this is a bit
redundant though.

Well, the refresh call could be handled by the helper too and then we'd just
have:

m_textTrackList->displayTextTrackList(m_textTrackList->isHidden());

here (and having TextTrackList in the method name might be a bit redundant...)

https://codereview.chromium.org/1079323002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSValueKeywords.in (right):

https://codereview.chromium.org/1079323002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSValueKeywords.in:626:
media-track-selection-checkmark
I think we could (and should) make these -internal-*

https://codereview.chromium.org/1079323002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/1079323002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:512:
EventDispatchForbiddenScope::AllowUserAgentEvents allowEvents;
What events are you seeing here? It'd really be preferable to have a "plain"
EventDispatchForbiddenScope here.

https://codereview.chromium.org/1079323002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/track/TextTrack.cpp (right):

https://codereview.chromium.org/1079323002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/track/TextTrack.cpp:431: if ((kind() ==
captionsKeyword() || kind() == subtitlesKeyword()) && readinessState() !=
FailedToLoad)
Could you write this like:

if (kind() != captionsKeyword() && kind() != subtitlesKeyword())
    return false;
if (readinessState() == FailedToLoad)
    return false;
return true;

instead?

https://codereview.chromium.org/1079323002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/blink_image_resources.grd (right):

https://codereview.chromium.org/1079323002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/blink_image_resources.grd:48: <structure
type="chrome_scaled_image" name="IDR_MEDIAPLAYER_TRACKSELECTION_CHECKMARK_NEW"
file="blink/mediaplayer_trackselection_checkmark_new.png" />
Do we still need to have both "old" and "new" resources? Couldn't we clean that
up now? If not, maybe just gate this on the "new UI" being enabled?

[philipj_slow, 2016-02-24 10:30:26]

On 2016/02/24 10:14:57, fs wrote:
> This mostly looks LG-TM (a few minor nits still), but I imagine philipj might
> want the final word =)

I was hoping to not spend too much time on this, but I can give it a quick look.
Is there some particularly interesting aspect that you think I should focus on?

[fs, 2016-02-24 12:07:58]

On 2016/02/24 at 10:30:26, philipj wrote:
> On 2016/02/24 10:14:57, fs wrote:
> > This mostly looks LG-TM (a few minor nits still), but I imagine philipj
might
> > want the final word =)
> 
> I was hoping to not spend too much time on this, but I can give it a quick
look. Is there some particularly interesting aspect that you think I should
focus on?

Well, it's fairly contained to MediaControl*, so it'd be around there I suppose.

[srivats, 2016-02-25 21:47:56]

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:156: //     |
+-MediaControlTextTrackListHeader            
(-internal-media-controls-text-track-list-header)
On 2016/02/24 10:14:57, fs wrote:
> On 2016/02/24 at 05:20:17, srivats wrote:
> > On 2016/02/23 13:16:44, fs wrote:
> > > Maybe add some indication that there can one/zero or more of some of
these.
> > > Likewise either/or etc.
> > 
> > Would a comment be appropriate here?
> > The structure looks very simple with no comments for any of the other
> elements.
> 
> Could be something like:
> 
> N*MediaControlTextTrackList...
> 
> or
> 
> | <for each renderable track>
> +-MediaControlTextTrackList...
> 
> or
> 
> +-MediaControlTextTrackList...<0>
> +-...
> +-MediaControlTextTrackList...<N-1>
> 
> or some variant thereof. Bonus points if it actually somewhat captures the
> actual DOM structure.

Done.

https://codereview.chromium.org/1079323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:471:
m_textTrackList->setIsWanted(false);
On 2016/02/24 10:14:57, fs wrote:
> On 2016/02/24 at 05:20:18, srivats wrote:
> > On 2016/02/23 13:16:44, fs wrote:
> > > I'm not sure the "is wanted" mechanism is really appropriate for this
list?
> I
> > > mean it probably works, but it sort of suggests that it operates within
the
> same
> > > "constraints" system as the control buttons - which I don't think it does?
> Maybe
> > > hide this semantic twist in a helper method?
> > 
> > Added a wrapper that calls setIsWanted within that. I still feel this is a
bit
> redundant though.
> 
> Well, the refresh call could be handled by the helper too and then we'd just
> have:
> 
> m_textTrackList->displayTextTrackList(m_textTrackList->isHidden());
> 
> here (and having TextTrackList in the method name might be a bit redundant...)

Done.

https://codereview.chromium.org/1079323002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSValueKeywords.in (right):

https://codereview.chromium.org/1079323002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSValueKeywords.in:626:
media-track-selection-checkmark
On 2016/02/24 10:14:57, fs wrote:
> I think we could (and should) make these -internal-*

Done.

https://codereview.chromium.org/1079323002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/1079323002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:512:
EventDispatchForbiddenScope::AllowUserAgentEvents allowEvents;
On 2016/02/24 10:14:57, fs wrote:
> What events are you seeing here? It'd really be preferable to have a "plain"
> EventDispatchForbiddenScope here.

Can't remember why this was needed in the first place. I think I was seeing a
DCHECK fail and I added this a long time ago.
Doesn't look like it's needed anymore.

https://codereview.chromium.org/1079323002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/track/TextTrack.cpp (right):

https://codereview.chromium.org/1079323002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/track/TextTrack.cpp:431: if ((kind() ==
captionsKeyword() || kind() == subtitlesKeyword()) && readinessState() !=
FailedToLoad)
On 2016/02/24 10:14:57, fs wrote:
> Could you write this like:
> 
> if (kind() != captionsKeyword() && kind() != subtitlesKeyword())
>     return false;
> if (readinessState() == FailedToLoad)
>     return false;
> return true;
> 
> instead?

Done.

https://codereview.chromium.org/1079323002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/blink_image_resources.grd (right):

https://codereview.chromium.org/1079323002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/blink_image_resources.grd:48: <structure
type="chrome_scaled_image" name="IDR_MEDIAPLAYER_TRACKSELECTION_CHECKMARK_NEW"
file="blink/mediaplayer_trackselection_checkmark_new.png" />
On 2016/02/24 10:14:57, fs wrote:
> Do we still need to have both "old" and "new" resources? Couldn't we clean
that
> up now? If not, maybe just gate this on the "new UI" being enabled?

Looks like all other icons have assets for both new UI and the old one and have
been defined the same way.
The new UI is enabled only on some platforms like Android AFAICT.

[fs, 2016-02-25 22:06:48]

LGTM w/ the EventDispatchForbiddenScope added.

You'll need someone else (presumably philipj) to approve the platform/ and
public/ bits.

https://codereview.chromium.org/1079323002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/1079323002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:512:
EventDispatchForbiddenScope::AllowUserAgentEvents allowEvents;
On 2016/02/25 at 21:47:56, srivats wrote:
> On 2016/02/24 10:14:57, fs wrote:
> > What events are you seeing here? It'd really be preferable to have a "plain"
> > EventDispatchForbiddenScope here.
> 
> Can't remember why this was needed in the first place. I think I was seeing a
DCHECK fail and I added this a long time ago.
> Doesn't look like it's needed anymore.

Ok, I'd suggest adding a "EventDispatchForbiddenScope assertNoEventDispatch;"
instead then (before the removeChildren even.)

https://codereview.chromium.org/1079323002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/blink_image_resources.grd (right):

https://codereview.chromium.org/1079323002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/blink_image_resources.grd:48: <structure
type="chrome_scaled_image" name="IDR_MEDIAPLAYER_TRACKSELECTION_CHECKMARK_NEW"
file="blink/mediaplayer_trackselection_checkmark_new.png" />
On 2016/02/25 at 21:47:56, srivats wrote:
> On 2016/02/24 10:14:57, fs wrote:
> > Do we still need to have both "old" and "new" resources? Couldn't we clean
that
> > up now? If not, maybe just gate this on the "new UI" being enabled?
> 
> Looks like all other icons have assets for both new UI and the old one and
have been defined the same way.
> The new UI is enabled only on some platforms like Android AFAICT.

Right, I see it's disabled by default. This is fine then I suppose.

[srivats, 2016-02-25 22:28:36]

https://codereview.chromium.org/1079323002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/1079323002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:512:
EventDispatchForbiddenScope::AllowUserAgentEvents allowEvents;
On 2016/02/25 22:06:48, fs wrote:
> On 2016/02/25 at 21:47:56, srivats wrote:
> > On 2016/02/24 10:14:57, fs wrote:
> > > What events are you seeing here? It'd really be preferable to have a
"plain"
> > > EventDispatchForbiddenScope here.
> > 
> > Can't remember why this was needed in the first place. I think I was seeing
a
> DCHECK fail and I added this a long time ago.
> > Doesn't look like it's needed anymore.
> 
> Ok, I'd suggest adding a "EventDispatchForbiddenScope assertNoEventDispatch;"
> instead then (before the removeChildren even.)

Adding that causes an assertion failure in Node::dispatchSubtreeModifiedEvent()
when I try to setAttribute on tracksSection element in this method.
Wouldn't assertNoEventDispatch prevent all events?

[philipj_slow, 2016-02-26 08:44:22]

I won't have time to get to this today (i.e. this week) I'm afraid. If you want
to ask someone else for the necessary lg-tms I won't be offended, otherwise I'll
try to get to it early next week.

[fs, 2016-02-26 09:09:12]

https://codereview.chromium.org/1079323002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/1079323002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:512:
EventDispatchForbiddenScope::AllowUserAgentEvents allowEvents;
On 2016/02/25 at 22:28:36, srivats wrote:
> On 2016/02/25 22:06:48, fs wrote:
> > On 2016/02/25 at 21:47:56, srivats wrote:
> > > On 2016/02/24 10:14:57, fs wrote:
> > > > What events are you seeing here? It'd really be preferable to have a
"plain"
> > > > EventDispatchForbiddenScope here.
> > > 
> > > Can't remember why this was needed in the first place. I think I was
seeing a
> > DCHECK fail and I added this a long time ago.
> > > Doesn't look like it's needed anymore.
> > 
> > Ok, I'd suggest adding a "EventDispatchForbiddenScope
assertNoEventDispatch;"
> > instead then (before the removeChildren even.)
> 
> Adding that causes an assertion failure in
Node::dispatchSubtreeModifiedEvent() when I try to setAttribute on tracksSection
element in this method.
> Wouldn't assertNoEventDispatch prevent all events?

No, it will do nothing more than assert. It generally a good thing
(==preferable) to avoid generating events in these cases. If we can't avoid
that, then you should re-add the AllowUserAgentEvents "scope" and document what
it allows and why it is safe (in this presumably safe because this is a closed
shadow root and there should be event listeners attached.)

[philipj_slow, 2016-03-01 08:51:41]

Comments from getting this to build and a few quick tests.

If you click the CC button to open the little menu, and then move the mouse
outside the video, the controls panel will fade out while the track menu remains
visible. The most sensible behavior is probably if an open track menu prevents
the controls from being hidden, and that click anywhere outside it when it is
open should close it.

Also, there's an extra divider line at the bottom of the list, not just between
each entry. This looks strange.

Another issue is when the label is too long. It's just clipped, and that's good,
but it's clipped at the edge of the background box instead of the edge of the
box that is highlighted when hovering, which would look better I think.
(text-overflow: ellipsis would have been great here, but it's not implemented
yet.)

These issues are both with the *old* controls, I haven't tested the new.

https://codereview.chromium.org/1079323002/diff/120001/content/app/strings/co...
File content/app/strings/content_strings.grd (right):

https://codereview.chromium.org/1079323002/diff/120001/content/app/strings/co...
content/app/strings/content_strings.grd:858: <message
name="IDS_PLUGIN_INITIALIZATION_ERROR" desc="A message displayed when a plug-in
failed to load">
It looks like you have accidentally reverted part of
https://codereview.chromium.org/1007443004 here, presumably when resolving a
conflict.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp (left):

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:362:
mediaElement().setClosedCaptionsVisible(!mediaElement().closedCaptionsVisible());
This was the only call site, can you remove
HTMLMediaElement::setClosedCaptionsVisible entirely?

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/track/TextTrack.cpp (right):

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/track/TextTrack.cpp:434: if
(readinessState() == FailedToLoad)
This has been renamed to getReadinessState() in
https://codereview.chromium.org/1740483004

[fs, 2016-03-01 09:01:00]

On 2016/03/01 at 08:51:41, philipj wrote:
...
> Another issue is when the label is too long. It's just clipped, and that's
good, but it's clipped at the edge of the background box instead of the edge of
the box that is highlighted when hovering, which would look better I think.
(text-overflow: ellipsis would have been great here, but it's not implemented
yet.)

It is - you need to know where the stones lie though...
(https://jsfiddle.net/dveqr9vc/)

[philipj_slow, 2016-03-01 09:06:49]

On 2016/03/01 09:01:00, fs wrote:
> On 2016/03/01 at 08:51:41, philipj wrote:
> ...
> > Another issue is when the label is too long. It's just clipped, and that's
> good, but it's clipped at the edge of the background box instead of the edge
of
> the box that is highlighted when hovering, which would look better I think.
> (text-overflow: ellipsis would have been great here, but it's not implemented
> yet.)
> 
> It is - you need to know where the stones lie though...
> (https://jsfiddle.net/dveqr9vc/)

Oh, cool, I must have typod when trying it. What is this second stone? It would
be nice if the icons were always shown but the text is truncated.

[philipj_slow, 2016-03-01 09:08:28]

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/track/text-track-selection-menu-add-track.html
(right):

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/track/text-track-selection-menu-add-track.html:3:
<script src="../video-test.js"></script>
In the new tests that use video-test.js, can you add TODOs like I've recently
added everywhere else?

<!-- TODO(srivats): Convert test to testharness.js. crbug.com/588956
     (Please avoid writing new tests using video-test.js) -->

It looks like you don't use much of the video-test.js framework, so it should be
pretty easy to rewrite, but not in this CL, it's been in-progress for so long
already :)

[philipj_slow, 2016-03-01 09:11:40]

third_party/WebKit/public/default_100_percent/blink/mediaplayer_closedcaptions_icon.png
looks quite out of place, and it turns out that it's a 31x28 image. Is the the
asset you got from Gabriel, and if so can you ask for one that's the same size
as mediaplayer_subtitles_icon.png, i.e. 128x128?

If you could link to issue 495851 in the description as well that would be
helpful, since that's where some of the relevant discussion is.

[fs, 2016-03-01 09:20:52]

On 2016/03/01 at 09:06:49, philipj wrote:
> On 2016/03/01 09:01:00, fs wrote:
> > On 2016/03/01 at 08:51:41, philipj wrote:
> > ...
> > > Another issue is when the label is too long. It's just clipped, and that's
> > good, but it's clipped at the edge of the background box instead of the edge
of
> > the box that is highlighted when hovering, which would look better I think.
> > (text-overflow: ellipsis would have been great here, but it's not
implemented
> > yet.)
> > 
> > It is - you need to know where the stones lie though...
> > (https://jsfiddle.net/dveqr9vc/)
> 
> Oh, cool, I must have typod when trying it. What is this second stone? It
would be nice if the icons were always shown but the text is truncated.

I think/suspect that overflow and text-overflow should be applied to ...-item
for the ellipsis to apply that way.

[philipj_slow, 2016-03-01 13:01:17]

Sorry for the delay! I've tried to not repeat things already discussed, please
just set me straight if I've raised an old and resolved issue.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/mediaControls.css (right):

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControls.css:350:
video::-webkit-scrollbar {
It's a bit weird that this pseudo-element is on the video element itself, but I
guess it doesn't work to do
video::-internal-media-controls-text-track-list::-webkit-scrollbar?

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/mediaControlsNew.css (right):

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControlsNew.css:47: font-family: Segoe,
"Helvetica Neue", Roboto, Arial, Helvetica, sans-serif ;
While you're moving this, drop the extra space before the ;

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControlsNew.css:372: position: absolute;
https://bugs.chromium.org/p/chromium/issues/detail?id=566499 is a bug report
about video controls overlapping things on the page, and the cause appears to be
positioned (position: relative or absolute) elements that change the paint
order. If you know of a simple way to avoid adding new cases of this that would
be good, but if it seems like it'd take a lot of changes to this CL, feel free
to leave it, it's not making matters worse right now.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControlsNew.css:376: max-width: 50%;
This doesn't seem to be achieving the intended effect, because the width is
limited to 150px even when the video is much wider than 300px and the text
doesn't fit.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControlsNew.css:381: text-overflow:
ellipsis;
Oh look, you're already using text-overflow:ellipsis. But it doesn't seem to
actually work, right? Should be on the item instead, perhaps?

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControlsNew.css:404: top: -2px;
Discussed first in
https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...

Do you mean that if you use a PNG that's just a single color, it will still look
misaligned compared to the text? Does this mean that vertical-align isn't
working as hoped, or why is that?

Fixing this would avoid the position:relative, which would be nice.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1179:
m_didPerformAutomaticTrackSelection = true;
This is a bit of a lie, automatic track selection actually didn't happen. Change
it to m_shouldPerform... and set it to false here instead? There are already a
bunch of things initialized to true, so any hope of initializing
HTMLMediaElement with all zeroes is lost.

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

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h:86: bool
isHidden();
Cannot isWanted be used instead? Both m_isWanted and m_doesFit are initially
true, so as long as you never call setDoesFit, it should be equivalent.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:64: static
const QualifiedName& trackIndexAttrName()
This is inside an anonymous namespace, so the static keyword isn't needed,
neither here nor for trackIndexOffValue.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:66: // Save
the track index in an attribute to avoid holding a pointer to the text track
element.
Remove "element"? Presumably the alternative would be a TextTrack* pointer, and
that's not an element.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:411: int
trackIndex = toElement(target)->getIntegralAttribute(trackIndexAttrName());
Nit: Can you switch the order of this and the following line?

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:414:
showTextTrackAtIndex(trackIndex);
trackIndex is int but this takes unsigned. ASSERT(trackIndex >= 0).

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:415:
mediaElement().disableAutomaticTextTrackSelection();
If a script implements its own track selection menu, it should ideally also
disable automatic text track selection. Did you try to just disable automatic
selection in HTMLMediaElement::textTrackModeChanged? Does it break something
else?

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:437: if
(trackList->length() == 0)
TextTrackList::anonymousIndexedGetter already handles out-of-bounds by returning
nullptr, I suggest removing these checks and relying on that, which would also
make it obvious that track is never dereferenced if null.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:464:
trackLabel = track->language();
This was discussed in
https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
with no resolution.

To make it abundantly clear that the srclang attribute is only for matching the
user language and not part of the UI, I suggest simply not falling back to it,
so that if no label is given it will show as "Unknown".

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:477:
RefPtrWillBeRawPtr<HTMLLabelElement> trackItem =
HTMLLabelElement::create(document, 0);
s/0/nullptr/

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:480:
RefPtrWillBeRawPtr<HTMLInputElement> trackItemInput =
HTMLInputElement::create(document, 0, false);
s/0/nullptr/

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:483:
trackItemInput->setType(InputTypeNames::radio);
Radio seems semantically accurate in the normal case, but it is possible for
multiple of these to be enabled at the same time. They don't belong to the same
radio button group it seems, so that's no problem in practice it seems. Still,
can you make these checkboxes instead? They're even rendered with a check mark
:)

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:485: if
(!mediaElement().closedCaptionsVisible()) {
This is now not terribly accurate, it's something more like textTracksVisible()
we' be interested in.  To make this all a bit more obvious, can you handle it in
MediaControlTextTrackListElement::refreshTextTrackListMenu by checking if any of
the tracks actually added are visible, and if not set the "Off" option as the
selected one? Simply creating this item last and inserting it before the other
tracks ought to do it.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:498: } else
if (track && track->kind() == track->subtitlesKeyword()) {
Can you use an else branch and ASSERT(track->kind() ==
track->subtitlesKeyword()) to not give the impression that there are other
possibilities here? (Only makes sense if you wrap both in if (track), so that's
what I mean.)

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:516:
RefPtrWillBeRawPtr<HTMLDivElement> tracksSection =
HTMLDivElement::create(this->document());
This div element doesn't seem to be included in the "The media controls DOM
structure looks like" documentation, can you add it?

Also looks like you can just use document() here, there's no other document
variable to disambiguate from.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:517:
tracksSection->setAttribute(roleAttr, radiogroupAttr.localName());
Does this change how this looks to a11y tools, and are there any tests for it?

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

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:157: void
changeVisibility(bool);
The meaning of this bool argument isn't obvious, and is the opposite of what I
would guess. Just show() and hide() would be OK, or setVisible(bool) with the
opposite meaning compared to now would work if you think the if (x) { show(); }
else { hide(); } is going to be ugly.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:166: String
getTextTrackLabel(TextTrack*);
Can you document here and below that null is allowed and what it means?

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

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:155: //    
+-MediaControlTextTrackListElement             
(-internal-media-controls-text-track-list)
This doesn't match initializeControls(), the MediaControlTextTrackListElement is
the last child of MediaControlPanelEnclosureElement and not a child of
MediaControlPanelElement. Not sure how to represent that in this ASCII art, it
would probably make sense to just make MediaControlTextTrackListElement a direct
child of MediaControls since it's absolutely positioned anyway. I think there's
also a better chance of doing the layout with flexbox and avoiding
position:absolute in that case.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:468: return;
Won't this early return leave the track list perpetually visible if all of the
text tracks are removed while the menu is open?

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/track/TextTrack.h (right):

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/track/TextTrack.h:107: bool canBeRendered();
Can this not be const? Can you drive-by fix isRendered too?

[philipj_slow, 2016-03-01 13:02:06]

linux_chromium_rel_ng is a sad, red bot. srivats@, do you have try bot access so
that you can iterate on this until all the bots are green?

[srivats, 2016-03-02 18:58:03]

On 2016/03/01 13:02:06, philipj_UTC7 wrote:
> linux_chromium_rel_ng is a sad, red bot. srivats@, do you have try bot access
so
> that you can iterate on this until all the bots are green?

I don't know what trybot access is required. I can pick try bots and do a CQ dry
run if that's what you meant by trybot access?
The linux bot failure should be fixed by changing readinessState() to
getReadinessState().
I'll rebase and try them again.

[philipj_slow, 2016-03-03 09:54:23]

On 2016/03/02 18:58:03, srivats wrote:
> On 2016/03/01 13:02:06, philipj_UTC7 wrote:
> > linux_chromium_rel_ng is a sad, red bot. srivats@, do you have try bot
access
> so
> > that you can iterate on this until all the bots are green?
> 
> I don't know what trybot access is required. I can pick try bots and do a CQ
dry
> run if that's what you meant by trybot access?
> The linux bot failure should be fixed by changing readinessState() to
> getReadinessState().
> I'll rebase and try them again.

If you can start the bots you have try bot access, so that's good :)

[srivats, 2016-03-30 00:44:01]

Description was changed from

==========
Support text track selection in video controls

Enable the user to select between text tracks with a menu that is displayed when
the CC button is clicked.

BUG=353105

TEST=run-webkit-tests media*
==========

to

==========
Support text track selection in video controls

Enable the user to select between text tracks with a menu that is displayed when
the CC button is clicked.

BUG=353105,495851

TEST=run-webkit-tests media*
==========

[srivats, 2016-03-30 00:46:44]

https://codereview.chromium.org/1079323002/diff/120001/content/app/strings/co...
File content/app/strings/content_strings.grd (right):

https://codereview.chromium.org/1079323002/diff/120001/content/app/strings/co...
content/app/strings/content_strings.grd:858: <message
name="IDS_PLUGIN_INITIALIZATION_ERROR" desc="A message displayed when a plug-in
failed to load">
On 2016/03/01 08:51:40, philipj_UTC7 wrote:
> It looks like you have accidentally reverted part of
> https://codereview.chromium.org/1007443004 here, presumably when resolving a
> conflict.

Sorry. Fixed it

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/track/text-track-selection-menu-add-track.html
(right):

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/track/text-track-selection-menu-add-track.html:3:
<script src="../video-test.js"></script>
On 2016/03/01 09:08:27, philipj_UTC7 wrote:
> In the new tests that use video-test.js, can you add TODOs like I've recently
> added everywhere else?
> 
> <!-- TODO(srivats): Convert test to testharness.js. crbug.com/588956
>      (Please avoid writing new tests using video-test.js) -->
> 
> It looks like you don't use much of the video-test.js framework, so it should
be
> pretty easy to rewrite, but not in this CL, it's been in-progress for so long
> already :)

Of course. Added

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/mediaControls.css (right):

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControls.css:350:
video::-webkit-scrollbar {
On 2016/03/01 13:01:15, philipj_UTC7 wrote:
> It's a bit weird that this pseudo-element is on the video element itself, but
I
> guess it doesn't work to do
> video::-internal-media-controls-text-track-list::-webkit-scrollbar?

Unfortunately not. I tried doing that before having to resort to doing it this
way.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/mediaControlsNew.css (right):

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControlsNew.css:47: font-family: Segoe,
"Helvetica Neue", Roboto, Arial, Helvetica, sans-serif ;
On 2016/03/01 13:01:15, philipj_UTC7 wrote:
> While you're moving this, drop the extra space before the ;

Done.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControlsNew.css:372: position: absolute;
On 2016/03/01 13:01:15, philipj_UTC7 wrote:
> https://bugs.chromium.org/p/chromium/issues/detail?id=566499 is a bug report
> about video controls overlapping things on the page, and the cause appears to
be
> positioned (position: relative or absolute) elements that change the paint
> order. If you know of a simple way to avoid adding new cases of this that
would
> be good, but if it seems like it'd take a lot of changes to this CL, feel free
> to leave it, it's not making matters worse right now.

Didn't get a chance to look at this.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControlsNew.css:376: max-width: 50%;
On 2016/03/01 13:01:16, philipj_UTC7 wrote:
> This doesn't seem to be achieving the intended effect, because the width is
> limited to 150px even when the video is much wider than 300px and the text
> doesn't fit.

I am retaining the max-width attribute and I changed min-width to be 150px
instead.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControlsNew.css:381: text-overflow:
ellipsis;
On 2016/03/01 13:01:15, philipj_UTC7 wrote:
> Oh look, you're already using text-overflow:ellipsis. But it doesn't seem to
> actually work, right? Should be on the item instead, perhaps?

Moved it to item and no it still doesn't do ellipsis. :(
The text just gets clipped at the moment.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControlsNew.css:404: top: -2px;
On 2016/03/01 13:01:15, philipj_UTC7 wrote:
> Discussed first in
>
https://codereview.chromium.org/1082533002/diff/80001/Source/core/css/mediaCo...
> 
> Do you mean that if you use a PNG that's just a single color, it will still
look
> misaligned compared to the text? Does this mean that vertical-align isn't
> working as hoped, or why is that?
> 
> Fixing this would avoid the position:relative, which would be nice.

Removed

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1179:
m_didPerformAutomaticTrackSelection = true;
On 2016/03/01 13:01:16, philipj_UTC7 wrote:
> This is a bit of a lie, automatic track selection actually didn't happen.
Change
> it to m_shouldPerform... and set it to false here instead? There are already a
> bunch of things initialized to true, so any hope of initializing
> HTMLMediaElement with all zeroes is lost.

Done.

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

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElementTypes.h:86: bool
isHidden();
On 2016/03/01 13:01:16, philipj_UTC7 wrote:
> Cannot isWanted be used instead? Both m_isWanted and m_doesFit are initially
> true, so as long as you never call setDoesFit, it should be equivalent.

Done.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp (left):

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:362:
mediaElement().setClosedCaptionsVisible(!mediaElement().closedCaptionsVisible());
On 2016/03/01 08:51:40, philipj_UTC7 wrote:
> This was the only call site, can you remove
> HTMLMediaElement::setClosedCaptionsVisible entirely?

Done.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:64: static
const QualifiedName& trackIndexAttrName()
On 2016/03/01 13:01:16, philipj_UTC7 wrote:
> This is inside an anonymous namespace, so the static keyword isn't needed,
> neither here nor for trackIndexOffValue.

Done.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:66: // Save
the track index in an attribute to avoid holding a pointer to the text track
element.
On 2016/03/01 13:01:16, philipj_UTC7 wrote:
> Remove "element"? Presumably the alternative would be a TextTrack* pointer,
and
> that's not an element.

Done.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:411: int
trackIndex = toElement(target)->getIntegralAttribute(trackIndexAttrName());
On 2016/03/01 13:01:16, philipj_UTC7 wrote:
> Nit: Can you switch the order of this and the following line?

Done.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:414:
showTextTrackAtIndex(trackIndex);
On 2016/03/01 13:01:16, philipj_UTC7 wrote:
> trackIndex is int but this takes unsigned. ASSERT(trackIndex >= 0).

Done.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:415:
mediaElement().disableAutomaticTextTrackSelection();
On 2016/03/01 13:01:16, philipj_UTC7 wrote:
> If a script implements its own track selection menu, it should ideally also
> disable automatic text track selection. Did you try to just disable automatic
> selection in HTMLMediaElement::textTrackModeChanged? Does it break something
> else?

I tried it and it seems to be working with some manual testing but timing out
these 3 layout tests:

  media/track/track-kind-user-preference.html [ Timeout ]
  media/track/track-language-preference.html [ Timeout ]
  media/track/track-mode-not-changed-by-new-track.html [ Timeout ]

trackModeChanged also runs when mode changes from hidden to disabled right? So I
guess that would disable auto selection incorrectly.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:437: if
(trackList->length() == 0)
On 2016/03/01 13:01:16, philipj_UTC7 wrote:
> TextTrackList::anonymousIndexedGetter already handles out-of-bounds by
returning
> nullptr, I suggest removing these checks and relying on that, which would also
> make it obvious that track is never dereferenced if null.

Done.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:464:
trackLabel = track->language();
On 2016/03/01 13:01:16, philipj_UTC7 wrote:
> This was discussed in
>
https://codereview.chromium.org/1082533002/diff/80001/Source/core/html/shadow...
> with no resolution.
> 
> To make it abundantly clear that the srclang attribute is only for matching
the
> user language and not part of the UI, I suggest simply not falling back to it,
> so that if no label is given it will show as "Unknown".
Apologies for that. I tried to respond back to as many comments as possible on
that code review.
Removed fallback to srclang like you suggested.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:477:
RefPtrWillBeRawPtr<HTMLLabelElement> trackItem =
HTMLLabelElement::create(document, 0);
On 2016/03/01 13:01:16, philipj_UTC7 wrote:
> s/0/nullptr/

Done.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:480:
RefPtrWillBeRawPtr<HTMLInputElement> trackItemInput =
HTMLInputElement::create(document, 0, false);
On 2016/03/01 13:01:16, philipj_UTC7 wrote:
> s/0/nullptr/

Done.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:483:
trackItemInput->setType(InputTypeNames::radio);
On 2016/03/01 13:01:16, philipj_UTC7 wrote:
> Radio seems semantically accurate in the normal case, but it is possible for
> multiple of these to be enabled at the same time. They don't belong to the
same
> radio button group it seems, so that's no problem in practice it seems. Still,
> can you make these checkboxes instead? They're even rendered with a check mark
> :)

I started off with radio in a radio group but eventually ended up realizing
radio group is not going to work for multiple selected tracks and got rid of the
radio group.
Changed it to a checkbox but retained my check mark :)

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:485: if
(!mediaElement().closedCaptionsVisible()) {
On 2016/03/01 13:01:16, philipj_OOO_til_March29 wrote:
> This is now not terribly accurate, it's something more like
textTracksVisible()
> we' be interested in.  To make this all a bit more obvious, can you handle it
in
> MediaControlTextTrackListElement::refreshTextTrackListMenu by checking if any
of
> the tracks actually added are visible, and if not set the "Off" option as the
> selected one? Simply creating this item last and inserting it before the other
> tracks ought to do it.

Renamed closedCaptionsVisible to textTracksVisible.
Addressing the second bit is not going to be trivial since setChecked is set on
the input element of the track item and has to be done in the context of where
the individual trackItem elements are created.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:498: } else
if (track && track->kind() == track->subtitlesKeyword()) {
On 2016/03/01 13:01:16, philipj_UTC7 wrote:
> Can you use an else branch and ASSERT(track->kind() ==
> track->subtitlesKeyword()) to not give the impression that there are other
> possibilities here? (Only makes sense if you wrap both in if (track), so
that's
> what I mean.)

Done.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:516:
RefPtrWillBeRawPtr<HTMLDivElement> tracksSection =
HTMLDivElement::create(this->document());
On 2016/03/01 13:01:16, philipj_UTC7 wrote:
> This div element doesn't seem to be included in the "The media controls DOM
> structure looks like" documentation, can you add it?
> 
> Also looks like you can just use document() here, there's no other document
> variable to disambiguate from.

Since we don't need a div element that holds both header and track list items
anymore, I got rid of the section entirely.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:517:
tracksSection->setAttribute(roleAttr, radiogroupAttr.localName());
On 2016/03/01 13:01:16, philipj_UTC7 wrote:
> Does this change how this looks to a11y tools, and are there any tests for it?

Got rid of the radio and hence this too

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

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:157: void
changeVisibility(bool);
On 2016/03/01 13:01:16, philipj_UTC7 wrote:
> The meaning of this bool argument isn't obvious, and is the opposite of what I
> would guess. Just show() and hide() would be OK, or setVisible(bool) with the
> opposite meaning compared to now would work if you think the if (x) { show();
}
> else { hide(); } is going to be ugly.

Changed it to setVisible. I can change it to hide and show if this still doesn't
work.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:166: String
getTextTrackLabel(TextTrack*);
On 2016/03/01 13:01:16, philipj_UTC7 wrote:
> Can you document here and below that null is allowed and what it means?

Done.

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

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:155: //    
+-MediaControlTextTrackListElement             
(-internal-media-controls-text-track-list)
On 2016/03/01 13:01:17, philipj_UTC7 wrote:
> This doesn't match initializeControls(), the MediaControlTextTrackListElement
is
> the last child of MediaControlPanelEnclosureElement and not a child of
> MediaControlPanelElement. Not sure how to represent that in this ASCII art, it
> would probably make sense to just make MediaControlTextTrackListElement a
direct
> child of MediaControls since it's absolutely positioned anyway. I think
there's
> also a better chance of doing the layout with flexbox and avoiding
> position:absolute in that case.

I moved track list element under MediaControls directly. Not sure how to get rid
of position:absolute though.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:468: return;
On 2016/03/01 13:01:16, philipj_UTC7 wrote:
> Won't this early return leave the track list perpetually visible if all of the
> text tracks are removed while the menu is open?

Good catch! I set the visibility to false before returning.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/track/TextTrack.cpp (right):

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/track/TextTrack.cpp:434: if
(readinessState() == FailedToLoad)
On 2016/03/01 08:51:40, philipj_UTC7 wrote:
> This has been renamed to getReadinessState() in
> https://codereview.chromium.org/1740483004

Done.

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/track/TextTrack.h (right):

https://codereview.chromium.org/1079323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/track/TextTrack.h:107: bool canBeRendered();
On 2016/03/01 13:01:17, philipj_UTC7 wrote:
> Can this not be const? Can you drive-by fix isRendered too?

Done.

[srivats, 2016-03-30 00:49:51]

On 2016/03/03 09:54:23, philipj_UTC7 wrote:
> On 2016/03/02 18:58:03, srivats wrote:
> > On 2016/03/01 13:02:06, philipj_UTC7 wrote:
> > > linux_chromium_rel_ng is a sad, red bot. srivats@, do you have try bot
> access
> > so
> > > that you can iterate on this until all the bots are green?
> > 
> > I don't know what trybot access is required. I can pick try bots and do a CQ
> dry
> > run if that's what you meant by trybot access?
> > The linux bot failure should be fixed by changing readinessState() to
> > getReadinessState().
> > I'll rebase and try them again.
> 
> If you can start the bots you have try bot access, so that's good :)

Doesn't look like I can start the try bots. Can you give me access to them?

[Avi (use Gerrit), 2016-03-30 02:20:06]

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

[Avi (use Gerrit), 2016-03-30 02:20:25]

On 2016/03/30 00:49:51, srivats wrote:
> On 2016/03/03 09:54:23, philipj_UTC7 wrote:
> > On 2016/03/02 18:58:03, srivats wrote:
> > > On 2016/03/01 13:02:06, philipj_UTC7 wrote:
> > > > linux_chromium_rel_ng is a sad, red bot. srivats@, do you have try bot
> > access
> > > so
> > > > that you can iterate on this until all the bots are green?
> > > 
> > > I don't know what trybot access is required. I can pick try bots and do a
CQ
> > dry
> > > run if that's what you meant by trybot access?
> > > The linux bot failure should be fixed by changing readinessState() to
> > > getReadinessState().
> > > I'll rebase and try them again.
> > 
> > If you can start the bots you have try bot access, so that's good :)
> 
> Doesn't look like I can start the try bots. Can you give me access to them?

Drive by trybot running.

[commit-bot: I haz the power, 2016-03-30 02:20:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1079323002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1079323002/140001

[commit-bot: I haz the power, 2016-03-30 02:23:16]

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

[commit-bot: I haz the power, 2016-03-30 02:23:18]

Dry run: Try jobs failed on following builders:
  linux_blink_oilpan_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oil...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[srivats, 2016-03-30 06:26:18]

The CQ bit was checked by srivats@amazon.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-30 06:26:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1079323002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1079323002/160001

[commit-bot: I haz the power, 2016-03-30 07:53:26]

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

[commit-bot: I haz the power, 2016-03-30 07:53:28]

Dry run: This issue passed the CQ dry run.

[srivats, 2016-04-11 05:42:42]

Patchset #10 (id:180001) has been deleted

[philipj_slow, 2016-04-11 11:05:29]

On 2016/04/11 05:43:07, srivats wrote:

I got an empty message in my inbox, do you want review for this now or are you
still working on it?

[srivats, 2016-04-11 14:46:08]

On 2016/04/11 11:05:29, philipj wrote:
> On 2016/04/11 05:43:07, srivats wrote:
> 
> I got an empty message in my inbox, do you want review for this now or are you
> still working on it?

Yup. I addressed your comments and made the changes rolfe suggested. Can you
give it a review?

Thanks,
Sunitha

[Avi (use Gerrit), 2016-04-11 15:12:08]

On 2016/04/11 14:46:08, srivats wrote:
> On 2016/04/11 11:05:29, philipj wrote:
> > On 2016/04/11 05:43:07, srivats wrote:
> > 
> > I got an empty message in my inbox, do you want review for this now or are
you
> > still working on it?
> 
> Yup. I addressed your comments and made the changes rolfe suggested. Can you
> give it a review?
> 
> Thanks,
> Sunitha

Content bits LGTM

[philipj_slow, 2016-04-19 15:21:09]

Looks good, can you rebase and start the bots? Before landing I'd like to apply
and test a bit locally.

https://codereview.chromium.org/1079323002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/1079323002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:473: bool
MediaControlTextTrackListElement::shouldDisplayTrackKindMarker(TextTrack*
currentTrack, String currentTrackLabel)
How about just a free helper function hasDuplicateLabel(const TextTrack& track)
that uses track.trackList() (need to make it const) and track.label()? Then it'd
be very clear that whether or not the marker is shown depends on what the
web-exposed label is, not anything else.

https://codereview.chromium.org/1079323002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:478: for
(unsigned i = 0; i < trackList->length(); i++) {
This will be O(n^2) in the number of tracks which isn't great, but the
alternative is a map<AtomicString, int> on each TextTrackList to keep track of
the number of tracks with a specific label, or sorting the tracks by label when
updating the UI. Can you add a comment that this is quadratic but that tracks
should be very few?

[srivats, 2016-04-19 23:13:41]

Patchset #11 (id:220001) has been deleted

[srivats, 2016-04-19 23:19:02]

PTAL

https://codereview.chromium.org/1079323002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/1079323002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:473: bool
MediaControlTextTrackListElement::shouldDisplayTrackKindMarker(TextTrack*
currentTrack, String currentTrackLabel)
On 2016/04/19 15:21:09, philipj_slow wrote:
> How about just a free helper function hasDuplicateLabel(const TextTrack&
track)
> that uses track.trackList() (need to make it const) and track.label()? Then
it'd
> be very clear that whether or not the marker is shown depends on what the
> web-exposed label is, not anything else.

Done.

https://codereview.chromium.org/1079323002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:478: for
(unsigned i = 0; i < trackList->length(); i++) {
On 2016/04/19 15:21:09, philipj_slow wrote:
> This will be O(n^2) in the number of tracks which isn't great, but the
> alternative is a map<AtomicString, int> on each TextTrackList to keep track of
> the number of tracks with a specific label, or sorting the tracks by label
when
> updating the UI. Can you add a comment that this is quadratic but that tracks
> should be very few?

Done.

[srivats, 2016-04-19 23:19:31]

The CQ bit was checked by srivats@amazon.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-19 23:20:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1079323002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1079323002/240001

[commit-bot: I haz the power, 2016-04-20 01:02:31]

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

[commit-bot: I haz the power, 2016-04-20 01:02:33]

Dry run: This issue passed the CQ dry run.

[philipj_slow, 2016-04-20 15:27:22]

LGTM % nits. I've also tested this locally, and it seems to work great. Thank
you!

https://codereview.chromium.org/1079323002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/1079323002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:491:
TextTrack* track = trackList->anonymousIndexedGetter(i);
Oh, now I see why TextTrack::trackList() wasn't const, it's because you can get
back the same track without const this way so it's pointless. Can you revert
that and instead just pass in a TextTrack* to this helper, with DCHECK to
document that it can't be null?

https://codereview.chromium.org/1079323002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:492: if
(&currentTrack != track && currentTrack.label() == track->label())
Can you extract currentTrack.label() into a variable outside of the loop, in
case the compiler can't figure out that it's possible?

https://codereview.chromium.org/1079323002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:522: if
(track && (hasDuplicateLabel(*track) || track->label().isEmpty())) {
Put the isEmpty() check first to avoid the loop in those cases?

https://codereview.chromium.org/1079323002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.h (right):

https://codereview.chromium.org/1079323002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.h:166: bool
hasDuplicateLabel(const TextTrack& currentTrack);
This can be in the .cpp file only, in an anonymous namespace, like
preloadTypeToString and others in HTMLMediaElement.cpp.

[srivats, 2016-04-22 15:16:40]

The CQ bit was checked by srivats@amazon.com

[srivats, 2016-04-22 15:16:40]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com,
avi@chromium.org, philipj@opera.com
Link to the patchset: https://codereview.chromium.org/1079323002/#ps260001
(title: "Rebase and lgtm nits")

[commit-bot: I haz the power, 2016-04-22 15:16:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1079323002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1079323002/260001

[commit-bot: I haz the power, 2016-04-22 19:12:49]

Description was changed from

==========
Support text track selection in video controls

Enable the user to select between text tracks with a menu that is displayed when
the CC button is clicked.

BUG=353105,495851

TEST=run-webkit-tests media*
==========

to

==========
Support text track selection in video controls

Enable the user to select between text tracks with a menu that is displayed when
the CC button is clicked.

BUG=353105,495851

TEST=run-webkit-tests media*
==========

[commit-bot: I haz the power, 2016-04-22 19:12:52]

Committed patchset #12 (id:260001)

[commit-bot: I haz the power, 2016-04-22 19:51:21]

Description was changed from

==========
Support text track selection in video controls

Enable the user to select between text tracks with a menu that is displayed when
the CC button is clicked.

BUG=353105,495851

TEST=run-webkit-tests media*
==========

to

==========
Support text track selection in video controls

Enable the user to select between text tracks with a menu that is displayed when
the CC button is clicked.

BUG=353105,495851

TEST=run-webkit-tests media*

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

[commit-bot: I haz the power, 2016-04-22 19:51:23]

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