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

third_party/WebKit/Source/core/html/HTMLMediaElement.cpp

Index: third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
diff --git a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp b/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
index d1a8bdf5736319ee04f4d0c88e9b32c2d014af0b..833fef54fe0611793710a84245bee3cf1008c16a 100644
--- a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
@@ -1090,7 +1090,6 @@ void HTMLMediaElement::loadResource(const WebMediaPlayerSource& source,
 
   if (fastHasAttribute(mutedAttr))
     m_muted = true;
-  updateVolume();

[mlamouri (slow - plz ping), 2016/11/07 00:26:05]

FYI, I couldn't find what that was for. I did some manual tests and everything
is fine. If there are no automated tests failing and you have no idea why that
was here, I would be in favour of removing.

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

https://chromium.googlesource.com/chromium/src/+/1f8312e78f2cb0b629dac384ae2b...
doesn't provide much of a clue, except that it came before the muted content
attribute.

If you remove this and use the content attribute, is there another call to
updateVolume() somewhere? Seem like without it things would go wrong.

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

I've added a test for this. It sounds fine. Let me know if I missed something.

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

Do you know why it works? It could be kind of accidental and therefore fragile.
The test should catch any regression, but understanding is nice, and I don't :)

[mlamouri (slow - plz ping), 2016/11/13 06:15:11]

That's because when readyState is changed, HTMLMediaElement calls reset() on the
MediaControls. Something to keep track of when using events for state change
listening but the test will help ;)

[foolip, 2016/11/14 16:07:37]

OK, since there's no volumechange event for this I guess it's fair that the
controls have to live by the same rules. Really the problem is that the muted
attribute should only be consulted once when the element is created, but it's
hard: https://crbug.com/350303

 
   DCHECK(!m_mediaSource);
 
@@ -2419,7 +2418,9 @@ void HTMLMediaElement::setVolume(double vol, ExceptionState& exceptionState) {
   }
 
   m_volume = vol;
-  updateVolume();
+
+  if (webMediaPlayer())
+    webMediaPlayer()->setVolume(effectiveMediaVolume());
   scheduleEvent(EventTypeNames::volumechange);
 }
 
@@ -2461,7 +2462,8 @@ void HTMLMediaElement::setMuted(bool muted) {
 
   // This is called after the volumechange event to make sure isAutoplayingMuted
   // returns the right value when webMediaPlayer receives the volume update.
-  updateVolume();
+  if (webMediaPlayer())
+    webMediaPlayer()->setVolume(effectiveMediaVolume());
 
   // If an element was a candidate for autoplay muted but not visible, it will
   // have a visibility observer ready to start its playback.
@@ -2471,14 +2473,6 @@ void HTMLMediaElement::setMuted(bool muted) {
   }
 }
 
-void HTMLMediaElement::updateVolume() {
-  if (webMediaPlayer())
-    webMediaPlayer()->setVolume(effectiveMediaVolume());
-
-  if (mediaControls())
-    mediaControls()->updateVolume();
-}
-
 double HTMLMediaElement::effectiveMediaVolume() const {
   if (m_muted)
     return 0;
@@ -3327,7 +3321,7 @@ void HTMLMediaElement::updatePlayState() {
       // media engine was setup.  The media engine should just stash the rate
       // and muted values since it isn't already playing.
       webMediaPlayer()->setRate(playbackRate());
-      updateVolume();
+      webMediaPlayer()->setVolume(effectiveMediaVolume());
       webMediaPlayer()->play();
       m_autoplayHelper->playbackStarted();
     }
@@ -3801,14 +3795,6 @@ bool HTMLMediaElement::isInteractiveContent() const {
   return fastHasAttribute(controlsAttr);
 }
 
-void HTMLMediaElement::defaultEventHandler(Event* event) {
-  if (event->type() == EventTypeNames::focusin) {
-    if (mediaControls())
-      mediaControls()->mediaElementFocused();
-  }
-  HTMLElement::defaultEventHandler(event);
-}
-
 DEFINE_TRACE(HTMLMediaElement) {
   visitor->trace(m_playedTimeRanges);
   visitor->trace(m_asyncEventQueue);