Make scrubbing a MediaControls-internal concept

Source/core/html/shadow/MediaControls.cpp

Index: Source/core/html/shadow/MediaControls.cpp
diff --git a/Source/core/html/shadow/MediaControls.cpp b/Source/core/html/shadow/MediaControls.cpp
index 59d1a761f8f057338027c3287128b302c6b96a4d..da71a336f7e7a29998135c17e1505d3e2f785795 100644
--- a/Source/core/html/shadow/MediaControls.cpp
+++ b/Source/core/html/shadow/MediaControls.cpp
@@ -65,6 +65,7 @@ MediaControls::MediaControls(HTMLMediaElement& mediaElement)
     , m_hideFullscreenControlsTimer(this, &MediaControls::hideFullscreenControlsTimerFired)
     , m_isFullscreen(false)
     , m_isMouseOverControls(false)
+    , m_isPausedForScrubbing(false)
 {
 }
 
@@ -176,7 +177,7 @@ void MediaControls::reset()
     m_durationDisplay->setInnerText(RenderTheme::theme().formatMediaControlsTime(duration), ASSERT_NO_EXCEPTION);
     m_durationDisplay->setCurrentValue(duration);
 
-    m_playButton->updateDisplayType();
+    updatePlayState();
 
     updateCurrentTimeDisplay();
 
@@ -233,9 +234,7 @@ void MediaControls::playbackStarted()
     m_currentTimeDisplay->show();
     m_durationDisplay->hide();
 
-    if (m_overlayPlayButton)
-        m_overlayPlayButton->updateDisplayType();
-    m_playButton->updateDisplayType();
+    updatePlayState();
     m_timeline->setPosition(mediaControllerInterface().currentTime());
     updateCurrentTimeDisplay();
 
@@ -254,9 +253,7 @@ void MediaControls::playbackProgressed()
 
 void MediaControls::playbackStopped()
 {
-    if (m_overlayPlayButton)
-        m_overlayPlayButton->updateDisplayType();
-    m_playButton->updateDisplayType();
+    updatePlayState();
     m_timeline->setPosition(mediaControllerInterface().currentTime());
     updateCurrentTimeDisplay();
     makeOpaque();
@@ -264,6 +261,32 @@ void MediaControls::playbackStopped()
     stopHideFullscreenControlsTimer();
 }
 
+void MediaControls::updatePlayState()
+{
+    if (m_isPausedForScrubbing)
+        return;
+
+    if (m_overlayPlayButton)
+        m_overlayPlayButton->updateDisplayType();
+    m_playButton->updateDisplayType();
+}
+
+void MediaControls::beginScrubbing()
+{
+    if (!mediaControllerInterface().paused()) {
+        m_isPausedForScrubbing = true;
+        mediaControllerInterface().pause();

[acolwell GONE FROM CHROMIUM, 2014/03/20 00:16:35]

I have concerns that this doesn't appear to be calling setPausedInternal()
anymore. It isn't clear to me that this change won't have unintended
consequences since it looks like HTMLMediaElement::updatePlayState() short
circuits a bunch of logic if m_pausedInternal is set and many other methods call
that function.

[philipj_slow, 2014/03/20 03:01:42]

This is the code in question:

if (m_pausedInternal) {
    if (!m_player->paused())
        m_player->pause();
    refreshCachedTime();
    m_playbackProgressTimer.stop();
    if (hasMediaControls())
        mediaControls()->playbackStopped();
    return;
}

When calling HTMLMediaElement::pause() instead of setPauseInternal(true), all of
these steps are called later in the function instead. There are also additional
side effects that setPauseInternal(true) avoids, but per the commit message
these code paths must already be exercised, even though it's possible that there
are bugs that will now become more visible.

Other ideas are very welcome. Just leaving it as is doesn't seem great, because
it can get media elements stuck with m_pausedInternal with unlucky timing.

+    }
+}
+
+void MediaControls::endScrubbing()
+{
+    if (m_isPausedForScrubbing) {
+        m_isPausedForScrubbing = false;
+        mediaControllerInterface().play();

[acolwell GONE FROM CHROMIUM, 2014/03/21 22:55:47]

Should we put a mediaControllerInterface().paused() guard on this call, just in
case JS happens to trigger a play before the controls get a chance to?

[philipj_slow, 2014/03/23 17:43:41]

Done.

+    }
+}
+
 void MediaControls::updateCurrentTimeDisplay()
 {
     double now = mediaControllerInterface().currentTime();