Make scrubbing a MediaControls-internal concept

Make scrubbing a MediaControls-internal concept

HTMLMediaElement::beginScrubbing() would setPausedInternal(true), a mode
of pausing that is invisible to scripts. If a media controller was
detached while scrubbing, HTMLMediaElement::endScrubbing() would never
be called and the element would be stuck in this mode.

Since a user can already pause, scrub and then continue playing, making
MediaControls do the same should work just fine. Simply keep track of
the state before scrubbing and avoid updating the play button(s) as long
as we're scrubbing.

Note that the special handling of ended() is not needed, since reaching
the end of the resource pauses. This wasn't the case at the time the
code was originally written.

BUG=341813
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169814

[philipj_slow, 2014-03-19 17:29:15]

PTAL.

I'm not 100% happy with the test coverage for this change. I think only a
screenshot test would find a bug where the m_isPausedForScrubbing early return
in MediaControls::updatePlayState() is missing, but we don't have any screenshot
tests for media controls yet so I'm not sure I want to add one, requiring
rebaselining all the time...

Other ideas welcome, or maybe accept that test coverage isn't perfect. It's
better than it was before, anyway.

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

https://codereview.chromium.org/204803002/diff/1/Source/core/html/shadow/Medi...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/204803002/diff/1/Source/core/html/shadow/Medi...
Source/core/html/shadow/MediaControls.cpp:278:
mediaControllerInterface().pause();
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]

https://codereview.chromium.org/204803002/diff/1/Source/core/html/shadow/Medi...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/204803002/diff/1/Source/core/html/shadow/Medi...
Source/core/html/shadow/MediaControls.cpp:278:
mediaControllerInterface().pause();
On 2014/03/20 00:16:35, acolwell wrote:
> 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.

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.

[acolwell GONE FROM CHROMIUM, 2014-03-21 22:55:46]

lgtm

https://codereview.chromium.org/204803002/diff/1/Source/core/html/shadow/Medi...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/204803002/diff/1/Source/core/html/shadow/Medi...
Source/core/html/shadow/MediaControls.cpp:286:
mediaControllerInterface().play();
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]

https://codereview.chromium.org/204803002/diff/1/Source/core/html/shadow/Medi...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/204803002/diff/1/Source/core/html/shadow/Medi...
Source/core/html/shadow/MediaControls.cpp:286:
mediaControllerInterface().play();
On 2014/03/21 22:55:47, acolwell_OOO_3-24 wrote:
> 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?

Done.

[philipj_slow, 2014-03-23 17:43:49]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-03-23 17:43:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/philipj@opera.com/204803002/20001

[commit-bot: I haz the power, 2014-03-23 18:45:14]

Change committed as 169814