Support text track selection in video controls

third_party/WebKit/Source/core/html/shadow/MediaControls.cpp

Index: third_party/WebKit/Source/core/html/shadow/MediaControls.cpp
diff --git a/third_party/WebKit/Source/core/html/shadow/MediaControls.cpp b/third_party/WebKit/Source/core/html/shadow/MediaControls.cpp
index e78e237e5fd378fe3dd1430bdd455906ad4f9cad..ea537e9f9606df0222809d5b55439679d81f601e 100644
--- a/third_party/WebKit/Source/core/html/shadow/MediaControls.cpp
+++ b/third_party/WebKit/Source/core/html/shadow/MediaControls.cpp
@@ -112,6 +112,7 @@ MediaControls::MediaControls(HTMLMediaElement& mediaElement)
     , m_muteButton(nullptr)
     , m_volumeSlider(nullptr)
     , m_toggleClosedCaptionsButton(nullptr)
+    , m_textTrackList(nullptr)
     , m_castButton(nullptr)
     , m_fullScreenButton(nullptr)
     , m_hideMediaControlsTimer(this, &MediaControls::hideMediaControlsTimerFired)
@@ -151,6 +152,12 @@ PassRefPtrWillBeRawPtr<MediaControls> MediaControls::create(HTMLMediaElement& me
 //     +-MediaControlMuteButtonElement                 (-webkit-media-controls-mute-button)
 //     +-MediaControlVolumeSliderElement               (-webkit-media-controls-volume-slider)
 //     +-MediaControlToggleClosedCaptionsButtonElement (-webkit-media-controls-toggle-closed-captions-button)
+//     +-MediaControlTextTrackListElement              (-internal-media-controls-text-track-list)
+//     | +-MediaControlTextTrackListHeader             (-internal-media-controls-text-track-list-header)

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

Maybe add some indication that there can one/zero or more of some of these.
Likewise either/or etc.

[srivats, 2016/02/24 05:20:17]

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

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

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.

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

Done.

+//     | +-MediaControlTextTrackListItem               (-internal-media-controls-text-track-list-item)

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

uNit: I think we've used '\' for these kinds of "turns" elsewhere.

[srivats, 2016/02/24 05:20:17]

Done.

+//       | +-MediaControlTextTrackListItemInput        (-internal-media-controls-text-track-list-item-input)
+//       | +-MediaControlTextTrackListItemCaptions     (-internal-media-controls-text-track-list-kind-captions)
+//       | +-MediaControlTextTrackListItemSubtitles    (-internal-media-controls-text-track-list-kind-subtitles)
 //     +-MediaControlCastButtonElement                 (-internal-media-controls-cast-button)
 //     \-MediaControlFullscreenButtonElement           (-webkit-media-controls-fullscreen-button)
 void MediaControls::initializeControls()
@@ -225,6 +232,10 @@ void MediaControls::initializeControls()
     m_panel = panel.get();
     enclosure->appendChild(panel.release());
 
+    RefPtrWillBeRawPtr<MediaControlTextTrackListElement> textTrackList = MediaControlTextTrackListElement::create(*this);
+    m_textTrackList = textTrackList.get();
+    enclosure->appendChild(textTrackList.release());
+
     m_enclosure = enclosure.get();
     appendChild(enclosure.release());
 }
@@ -451,6 +462,19 @@ void MediaControls::refreshClosedCaptionsButtonVisibility()
     BatchedControlUpdate batch(this);
 }
 
+void MediaControls::toggleTextTrackList()
+{
+    if (!mediaElement().hasClosedCaptions())
+        return;
+
+    if (!m_textTrackList->isHidden()) {
+        m_textTrackList->setIsWanted(false);

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

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?

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

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

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

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

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

Done.

+    } else {
+        m_textTrackList->refreshTextTrackListMenu();
+        m_textTrackList->setIsWanted(true);
+    }
+}
+
 void MediaControls::refreshCastButtonVisibility()
 {
     refreshCastButtonVisibilityWithoutUpdate();
@@ -750,6 +774,7 @@ DEFINE_TRACE(MediaControls)
     visitor->trace(m_fullScreenButton);
     visitor->trace(m_durationDisplay);
     visitor->trace(m_enclosure);
+    visitor->trace(m_textTrackList);
     visitor->trace(m_castButton);
     visitor->trace(m_overlayCastButton);
     HTMLDivElement::trace(visitor);