Only show overlay play button when controls are enabled.

Only show overlay play button when controls are enabled.

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

[Fredrik Öhrn, 2014-07-01 13:10:50]

This is a new attempt at fixing the overlay play button bug.

The play button will only show itself if controls are actually enabled (i.e.
shouldShowControls returns true).

The play button is hidden whenever the controls are hidden to make sure it
doesn't linger (i.e. shouldShowControls toggled to false).

[acolwell GONE FROM CHROMIUM, 2014-07-01 17:06:22]

please add a test.

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

https://codereview.chromium.org/363563007/diff/1/Source/core/html/shadow/Medi...
Source/core/html/shadow/MediaControls.cpp:207: m_overlayPlayButton->hide();
It seems like there should coorresponding "show" logic in MediaControls::show()
as well. Otherwise it seems like a "hide() then show()" sequence could cause the
controls to disappear and then never come back.

Perhaps the following in show() would work?
if (m_overlayPlayButton)
    m_overlayPlayButton->updateDisplayType();

[philipj_slow, 2014-07-01 21:57:34]

For that test, you'll need to enable the overlay play button with something like
this:

if (window.internal)
    window.internals.settings.setMediaControlsOverlayPlayButtonEnabled(true)

media-controls.js has some useful helpers, and mediaControlsButton(video,
'overlay-play-button') should give you the element to test its visibility.

[Fredrik Öhrn, 2014-07-02 09:57:11]

On 2014/07/01 17:06:22, acolwell wrote:
> please add a test.
> 

Done.

>
https://codereview.chromium.org/363563007/diff/1/Source/core/html/shadow/Medi...
> File Source/core/html/shadow/MediaControls.cpp (right):
> 
>
https://codereview.chromium.org/363563007/diff/1/Source/core/html/shadow/Medi...
> Source/core/html/shadow/MediaControls.cpp:207: m_overlayPlayButton->hide();
> It seems like there should coorresponding "show" logic in
MediaControls::show()
> as well. Otherwise it seems like a "hide() then show()" sequence could cause
the
> controls to disappear and then never come back.
> 
> Perhaps the following in show() would work?
> if (m_overlayPlayButton)
>     m_overlayPlayButton->updateDisplayType();

Your right, I forgot about that case.

[Fredrik Öhrn, 2014-07-02 11:13:35]

I extended the test to cover all combinations of controls and play/pause.

[acolwell GONE FROM CHROMIUM, 2014-07-02 16:09:26]

lgtm

[Fredrik Öhrn, 2014-07-02 19:15:55]

The CQ bit was checked by ohrn@opera.com

[commit-bot: I haz the power, 2014-07-02 19:17:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ohrn@opera.com/363563007/40001

[commit-bot: I haz the power, 2014-07-02 20:39:31]

Change committed as 177418