Adding overflow menu to media player

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 439d99210e77e9af92bf438d7df587fef0aa61a0..1fd533eb97e38c99b37adeaa4239f4f77ab91cf3 100644
--- a/third_party/WebKit/Source/core/html/shadow/MediaControls.cpp
+++ b/third_party/WebKit/Source/core/html/shadow/MediaControls.cpp
@@ -117,6 +117,11 @@ MediaControls::MediaControls(HTMLMediaElement& mediaElement)
     , m_volumeSlider(nullptr)
     , m_toggleClosedCaptionsButton(nullptr)
     , m_textTrackList(nullptr)
+    , m_overflowList(nullptr)
+    , m_muteOverflowButton(nullptr)
+    , m_castOverflowButton(nullptr)
+    , m_ClosedCaptionsOverflowButton(nullptr)
+    , m_fullscreenOverflowButton(nullptr)
     , m_castButton(nullptr)
     , m_fullScreenButton(nullptr)
     , m_hideMediaControlsTimer(this, &MediaControls::hideMediaControlsTimerFired)
@@ -242,6 +247,24 @@ void MediaControls::initializeControls()
     MediaControlTextTrackListElement* textTrackList = MediaControlTextTrackListElement::create(*this);
     m_textTrackList = textTrackList;
     appendChild(textTrackList);
+
+    MediaControlOverflowMenu* overflowMenu = MediaControlOverflowMenu::create(*this);
+    m_overflowMenu = overflowMenu;
+    panel->appendChild(overflowMenu);
+
+    MediaControlOverflowMenuListElement* overflowList = MediaControlOverflowMenuListElement::create(*this);
+    m_overflowList = overflowList;
+    appendChild(overflowList);
+
+    m_muteOverflowButton = MediaControlMuteButtonElement::create(*this);
+    m_castOverflowButton = MediaControlCastButtonElement::create(*this, false);
+    m_ClosedCaptionsOverflowButton = MediaControlToggleClosedCaptionsButtonElement::create(*this);
+    m_fullscreenOverflowButton = MediaControlFullscreenButtonElement::create(*this);
+
+    m_muteOverflowButton->setShadowPseudoId(AtomicString("-internal-media-controls-overflow-menu-list-item-element"));
+    m_castOverflowButton->setShadowPseudoId(AtomicString("-internal-media-controls-overflow-menu-list-item-element"));
+    m_ClosedCaptionsOverflowButton->setShadowPseudoId(AtomicString("-internal-media-controls-overflow-menu-list-item-element"));
+    m_fullscreenOverflowButton->setShadowPseudoId(AtomicString("-internal-media-controls-overflow-menu-list-item-element"));

[whywhat, 2016/08/19 23:56:55]

do we have these elements in the media element's dom tree all the time? I
thought we would add them in run-time when needed.

[kdsilva, 2016/08/24 05:33:27]

liberato@, Anton mentioned you might have recommendations on when these media
elements should be created. PTAL?

[liberato (no reviews please), 2016/08/24 16:44:22]

i've had bad luck doing anything too complicated in ComputeWhichControlsFit --
it's called while blink is in some unexpected states.  Creating them up front
seems safer.

that said, it looks like the code is synamically creating new labels and a div
when constructing the overflow menu.  as it sits now, it's halfway between
dynamically allocated and preallocated.

i'd suggest either make it entirely one way or the other.

one might also hide a lot of this inside the MediaControlElement subclasses
themselves -- an element could equally well create its own overflow button. 
haven't thought it through, but it seems like most of the MediaControl code
could work entirely on the original panel elements, rather than maintaining both
lists.

 }
 
 void MediaControls::reset()
@@ -433,6 +456,8 @@ void MediaControls::updateCurrentTimeDisplay()
 void MediaControls::updateVolume()
 {
     m_muteButton->updateDisplayType();
+    m_muteOverflowButton->updateDisplayType();
+
     // Invalidate the mute button because it paints differently according to volume.
     invalidate(m_muteButton);
 
@@ -466,6 +491,7 @@ void MediaControls::updateVolume()
 void MediaControls::changedClosedCaptionsVisibility()
 {
     m_toggleClosedCaptionsButton->updateDisplayType();
+    m_ClosedCaptionsOverflowButton->updateDisplayType();
 }
 
 void MediaControls::refreshClosedCaptionsButtonVisibility()
@@ -689,8 +715,8 @@ void MediaControls::computeWhichControlsFit()
     // Controls that we'll hide / show, in order of decreasing priority.
     MediaControlElement* elements[] = {
         // Exclude m_playButton; we handle it specially.
-        m_toggleClosedCaptionsButton.get(),
         m_fullScreenButton.get(),
+        m_toggleClosedCaptionsButton.get(),

[whywhat, 2016/08/19 23:56:55]

I thought this was fixed by another cl? Do you need to sync and rebase this cl?

[kdsilva, 2016/08/24 05:33:26]

Done.

         m_timeline.get(),
         m_currentTimeDisplay.get(),
         m_volumeSlider.get(),
@@ -730,6 +756,10 @@ void MediaControls::computeWhichControlsFit()
         return;
     }
 
+    // Controls that don't fit within the media player that could be included
+    // in an overflow menu.
+    std::set<MediaControlElementType> overflowControls;
+
     // For each control that fits, enable it in order of decreasing priority.
     bool droppedCastButton = false;
     for (MediaControlElement* element : elements) {
@@ -744,10 +774,21 @@ void MediaControls::computeWhichControlsFit()
                 element->setDoesFit(false);
                 if (element == m_castButton.get())
                     droppedCastButton = true;
+                overflowControls.insert(element->displayType());
             }
         }
     }
 
+    // We display an overflow menu only when we have at least two items
+    // within it.
+    if (overflowControls.size() >= 2 && !mediaElement().isFullscreen()) {

[whywhat, 2016/08/19 23:56:55]

2 is a magic constant (which is not 0 or 1). please create a name for it and
define it somewhere (like at the top of the file in unnamed namespace)

is it safe to assume that in fullscreen (portrait mode) all the controls will
fit?

[kdsilva, 2016/08/24 05:33:26]

Done.

+        m_overflowList->setOverflowMenuControls(overflowControls);
+        m_overflowMenu->setIsWanted(true);
+    } else {
+        m_overflowMenu->setIsWanted(false);
+        m_overflowList->setVisible(false);
+    }
+
     // Special case for cast: if we want a cast button but dropped it, then
     // show the overlay cast button instead.
     if (m_castButton->isWanted()) {
@@ -784,6 +825,22 @@ void MediaControls::networkStateChanged()
     invalidate(m_volumeSlider);
 }
 
+bool MediaControls::overflowMenuVisible()
+{
+    return m_overflowList->isWanted();
+}
+
+void MediaControls::toggleOverflowMenu()
+{
+    m_overflowList->setVisible(!m_overflowList->isWanted());
+}
+
+std::vector<MediaControlInputElement*> MediaControls::getOverflowMenuButtons()
+{
+    std::vector<MediaControlInputElement*> controlsListOverflow = { m_muteOverflowButton, m_ClosedCaptionsOverflowButton, m_castOverflowButton, m_fullscreenOverflowButton };

[whywhat, 2016/08/19 23:56:55]

creating a new vector and passing it around seems wasteful. can this be const-ed
or cached somehow?

[kdsilva, 2016/08/24 05:33:27]

Done. I made controlsListOverflow a private member variable. I'm not sure if
that's the best approach. PTAL?

+    return controlsListOverflow;
+}
+
 DEFINE_TRACE(MediaControls)
 {
     visitor->trace(m_mediaElement);
@@ -800,6 +857,12 @@ DEFINE_TRACE(MediaControls)
     visitor->trace(m_durationDisplay);
     visitor->trace(m_enclosure);
     visitor->trace(m_textTrackList);
+    visitor->trace(m_overflowMenu);
+    visitor->trace(m_overflowList);
+    visitor->trace(m_muteOverflowButton);
+    visitor->trace(m_castOverflowButton);
+    visitor->trace(m_ClosedCaptionsOverflowButton);
+    visitor->trace(m_fullscreenOverflowButton);
     visitor->trace(m_castButton);
     visitor->trace(m_overlayCastButton);
     HTMLDivElement::trace(visitor);