Make scrubbing a MediaControls-internal concept

Source/core/html/shadow/MediaControls.cpp

 /*
  * Copyright (C) 2011, 2012 Apple Inc. All rights reserved.
  * Copyright (C) 2011, 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 47 matching lines @@
     , m_timeline(0)
     , m_muteButton(0)
     , m_volumeSlider(0)
     , m_toggleClosedCaptionsButton(0)
     , m_fullScreenButton(0)
     , m_durationDisplay(0)
     , m_enclosure(0)
     , m_hideFullscreenControlsTimer(this, &MediaControls::hideFullscreenControlsTimerFired)
     , m_isFullscreen(false)
     , m_isMouseOverControls(false)
+    , m_isPausedForScrubbing(false)
 {
 }
 
 PassRefPtr<MediaControls> MediaControls::create(HTMLMediaElement& mediaElement)
 {
     RefPtr<MediaControls> controls = adoptRef(new MediaControls(mediaElement));
 
     if (controls->initializeControls())
         return controls.release();
 
@@ skipping 91 matching lines @@
         return *m_mediaElement.controller();
     return m_mediaElement;
 }
 
 void MediaControls::reset()
 {
     double duration = mediaControllerInterface().duration();
     m_durationDisplay->setInnerText(RenderTheme::theme().formatMediaControlsTime(duration), ASSERT_NO_EXCEPTION);
     m_durationDisplay->setCurrentValue(duration);
 
-    m_playButton->updateDisplayType();
+    updatePlayState();
 
     updateCurrentTimeDisplay();
 
     m_timeline->setDuration(mediaControllerInterface().duration());
     m_timeline->setPosition(mediaControllerInterface().currentTime());
 
     if (!mediaControllerInterface().hasAudio()) {
         m_volumeSlider->hide();
     } else {
         m_volumeSlider->show();
@@ skipping 36 matching lines @@
 bool MediaControls::shouldHideFullscreenControls()
 {
     return alwaysHideFullscreenControls || !m_panel->hovered();
 }
 
 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();
 
     if (m_isFullscreen)
         startHideFullscreenControlsTimer();
 }
 
 void MediaControls::playbackProgressed()
 {
     m_timeline->setPosition(mediaControllerInterface().currentTime());
     updateCurrentTimeDisplay();
 
     if (!m_isMouseOverControls && mediaElement().hasVideo())
         makeTransparent();
 }
 
 void MediaControls::playbackStopped()
 {
-    if (m_overlayPlayButton)
-        m_overlayPlayButton->updateDisplayType();
-    m_playButton->updateDisplayType();
+    updatePlayState();
     m_timeline->setPosition(mediaControllerInterface().currentTime());
     updateCurrentTimeDisplay();
     makeOpaque();
 
     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();
     double duration = mediaControllerInterface().duration();
 
     // After seek, hide duration display and show current time.
     if (now > 0) {
         m_currentTimeDisplay->show();
         m_durationDisplay->hide();
     }
@@ skipping 162 matching lines @@
 
 void MediaControls::updateTextTrackDisplay()
 {
     if (!m_textDisplayContainer)
         createTextTrackDisplay();
 
     m_textDisplayContainer->updateDisplay();
 }
 
 }