Media Controls: delegate 'volumechange' and 'focusin' handling to an EventListener.

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 74780f9bb3b4caf07eb5bfa8e8e0a71b02a0eb85..4e0866d39209d8f34bcaa8cfb4047bbe1b4da357 100644
--- a/third_party/WebKit/Source/core/html/shadow/MediaControls.cpp
+++ b/third_party/WebKit/Source/core/html/shadow/MediaControls.cpp
@@ -32,6 +32,7 @@
 #include "core/events/MouseEvent.h"
 #include "core/frame/Settings.h"
 #include "core/html/HTMLMediaElement.h"
+#include "core/html/shadow/MediaControlsMediaEventListener.h"
 #include "core/html/shadow/MediaControlsWindowEventListener.h"
 #include "core/html/track/TextTrackContainer.h"
 #include "core/html/track/TextTrackList.h"
@@ -122,6 +123,7 @@ MediaControls::MediaControls(HTMLMediaElement& mediaElement)
       m_castButton(nullptr),
       m_fullscreenButton(nullptr),
       m_downloadButton(nullptr),
+      m_mediaEventListener(new MediaControlsMediaEventListener(this)),
       m_windowEventListener(MediaControlsWindowEventListener::create(
           this,
           WTF::bind(&MediaControls::hideAllMenus, wrapWeakPersistent(this)))),
@@ -336,7 +338,7 @@ void MediaControls::reset() {
   m_timeline->setDuration(duration);
   m_timeline->setPosition(mediaElement().currentTime());
 
-  updateVolume();
+  onVolumeChange();
 
   refreshClosedCaptionsButtonVisibility();
 
@@ -360,13 +362,6 @@ void MediaControls::show() {
     m_overlayPlayButton->updateDisplayType();
 }
 
-void MediaControls::mediaElementFocused() {
-  if (mediaElement().shouldShowControls()) {
-    show();
-    resetHideMediaControlsTimer();
-  }
-}
-
 void MediaControls::hide() {
   m_panel->setIsWanted(false);
   m_panel->setIsDisplayed(false);
@@ -485,32 +480,6 @@ void MediaControls::updateCurrentTimeDisplay() {
   m_currentTimeDisplay->setCurrentValue(now);
 }
 
-void MediaControls::updateVolume() {
-  m_muteButton->updateDisplayType();
-  // Invalidate the mute button because it paints differently according to
-  // volume.
-  invalidate(m_muteButton);

[foolip, 2016/11/08 10:28:29]

Did you mean to remove this? Some bots are sad, related?

[mlamouri (slow - plz ping), 2016/11/11 05:29:17]

I don't think it's needed. Apart that it would allow the mute button to be
invalidated when the volume changed, it doesn't seem needed. I changed the mute
button to set it style (creating invalidation) based on muted and volume state
to match the expected rendering.

[foolip, 2016/11/11 09:50:20]

Acknowledged.

-
-  if (mediaElement().muted())
-    m_volumeSlider->setVolume(0);
-  else
-    m_volumeSlider->setVolume(mediaElement().volume());
-
-  // Update the visibility of our audio elements.
-  // We never want the volume slider if there's no audio.
-  // If there is audio, then we want it unless we prefer to hide it.
-  BatchedControlUpdate batch(this);
-  m_volumeSlider->setIsWanted(mediaElement().hasAudio() &&
-                              !preferHiddenVolumeControls(document()));
-
-  // If there is no audio track, then hide the mute button.
-  m_muteButton->setIsWanted(mediaElement().hasAudio());
-
-  // Invalidate the volume slider because it paints differently according to
-  // volume.
-  invalidate(m_volumeSlider);
-}
-
 void MediaControls::changedClosedCaptionsVisibility() {
   m_toggleClosedCaptionsButton->updateDisplayType();
 }
@@ -727,6 +696,27 @@ bool MediaControls::containsRelatedTarget(Event* event) {
   return contains(relatedTarget->toNode());
 }
 
+void MediaControls::onVolumeChange() {
+  m_muteButton->updateDisplayType();
+  m_volumeSlider->setVolume(mediaElement().muted() ? 0

[foolip, 2016/11/08 10:28:29]

Does mediaElement().effectiveMediaVolume() work?

[mlamouri (slow - plz ping), 2016/11/11 05:29:17]

I did that initially but given that in the long run, I want to make
MediaControls use regular web exposed things, I moved back to this. I'm happy to
change if you prefer.

[foolip, 2016/11/11 09:50:20]

That's fine.

+                                                   : mediaElement().volume());
+
+  // Update visibility of volume controls.
+  // TODO(mlamouri): it should not be part of the volumechange handling.
+  BatchedControlUpdate batch(this);
+  m_volumeSlider->setIsWanted(mediaElement().hasAudio() &&
+                              !preferHiddenVolumeControls(document()));
+  m_muteButton->setIsWanted(mediaElement().hasAudio());
+}
+
+void MediaControls::onFocusIn() {
+  if (!mediaElement().shouldShowControls())
+    return;
+
+  show();
+  resetHideMediaControlsTimer();
+}
+
 void MediaControls::notifyPanelWidthChanged(const LayoutUnit& newWidth) {
   // Don't bother to do any work if this matches the most recent panel
   // width, since we're called after layout.
@@ -918,6 +908,7 @@ DEFINE_TRACE(MediaControls) {
   visitor->trace(m_overflowList);
   visitor->trace(m_castButton);
   visitor->trace(m_overlayCastButton);
+  visitor->trace(m_mediaEventListener);
   visitor->trace(m_windowEventListener);
   HTMLDivElement::trace(visitor);
 }