[Media Controls] Prevent flicker when enter/exit fullscreen

[Media Controls] Prevent flicker when enter/exit fullscreen

Previously, entering/exiting fullscreen whilst the media controls are
hidden due to inactivity would cause the media controls to briefly show
then almost immediately hide again, resulting in an ugly flicker.

They were shown by HTMLMediaElement::UpdateControlsVisibility (called
from HTMLMediaElement::Did{Enter,Exit}Fullscreen) and then hidden again
by MediaControlsImpl::OnTimeUpdate since ShouldHideMediaControls()
returns true.

(This was often masked by the fact that clicking the fullscreen button
with a mouse results in the mouse hovering over the media controls at
the start of the fullscreen transition, in which case it's ok to show
the media controls. It became particularly noticeable with the recent
video-rotate-to-fullscreen feature, which enters/exits fullscreen
without interacting with the media controls, but it can be reproduced
without that feature as well).

This patch renames MediaControls::Show to MaybeShow and makes it only
show the controls if they won't soon be hidden again.

BUG=717520
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2873493004
Cr-Commit-Position: refs/heads/master@{#472180}
Committed: https://chromium.googlesource.com/chromium/src/+/1fb662149d40ec4b4a19d984007528ea0b8798e3

[johnme, 2017-05-09 15:53:28]

johnme@chromium.org changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2017-05-09 17:35:46]

lgtm with the test files using a 2 space indentation.

[johnme, 2017-05-09 17:52:46]

On 2017/05/09 17:35:46, mlamouri wrote:
> lgtm with the test files using a 2 space indentation.

Done, thanks!

[johnme, 2017-05-09 17:52:49]

The CQ bit was checked by johnme@chromium.org

[johnme, 2017-05-09 17:52:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2873493004/#ps20001
(title: "2-space indent for tests")

[commit-bot: I haz the power, 2017-05-09 17:54:07]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 19:48:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 19:48:16]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[mlamouri (slow - plz ping), 2017-05-15 14:42:16]

https://codereview.chromium.org/2873493004/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/controls/video-enter-exit-fullscreen-while-hovering-shows-controls.html
(right):

https://codereview.chromium.org/2873493004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/video-enter-exit-fullscreen-while-hovering-shows-controls.html:22:
clickAtCoordinates(...mediaControlsButtonCoordinates(video, "play-button"));
What's the "..."

https://codereview.chromium.org/2873493004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/video-enter-exit-fullscreen-while-hovering-shows-controls.html:33:
chrome.gpuBenchmarking.pointerActionSequence([{
Did you consider using "eventSender.mouseMoveTo"? or the equivalent pointer
method?

https://codereview.chromium.org/2873493004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/video-enter-exit-fullscreen-while-hovering-shows-controls.html:87:
video.src = findMediaFile("video", "../content/test-25fps");
Maybe you should use "../content/test"? The 25fps one is used only for one test
that is related to seek and precision.

[johnme, 2017-05-15 14:56:03]

https://codereview.chromium.org/2873493004/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/controls/video-enter-exit-fullscreen-while-hovering-shows-controls.html
(right):

https://codereview.chromium.org/2873493004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/video-enter-exit-fullscreen-while-hovering-shows-controls.html:22:
clickAtCoordinates(...mediaControlsButtonCoordinates(video, "play-button"));
On 2017/05/15 14:42:15, mlamouri wrote:
> What's the "..."

It's
https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Operators/Spre....
In this case, it's equivalent to:

var [x, y] = mediaControlsButtonCoordinates(video, "play-button");
clickAtCoordinates(x, y);

or

var coords = mediaControlsButtonCoordinates(video, "play-button");
clickAtCoordinates(coords[0], coords[1]);

https://codereview.chromium.org/2873493004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/video-enter-exit-fullscreen-while-hovering-shows-controls.html:33:
chrome.gpuBenchmarking.pointerActionSequence([{
On 2017/05/15 14:42:15, mlamouri wrote:
> Did you consider using "eventSender.mouseMoveTo"? or the equivalent pointer
> method?

That's deprecated and fails presubmit (though I realise that we still use it
behind the scenes of clickAtCoordinates). But sure, I'll upload a new patch that
uses it, to see if it changes anything on the trybots.

https://codereview.chromium.org/2873493004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/video-enter-exit-fullscreen-while-hovering-shows-controls.html:87:
video.src = findMediaFile("video", "../content/test-25fps");
On 2017/05/15 14:42:15, mlamouri wrote:
> Maybe you should use "../content/test"? The 25fps one is used only for one
test
> that is related to seek and precision.

With ../content/test the video ends before the test is over (due to the 2x
runAfterHideMediaControlsTimerFired delays). The 25fps one is longer and doesn't
seem to cause problems.

[mlamouri (slow - plz ping), 2017-05-15 15:17:51]

https://codereview.chromium.org/2873493004/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/controls/video-enter-exit-fullscreen-while-hovering-shows-controls.html
(right):

https://codereview.chromium.org/2873493004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/video-enter-exit-fullscreen-while-hovering-shows-controls.html:22:
clickAtCoordinates(...mediaControlsButtonCoordinates(video, "play-button"));
On 2017/05/15 at 14:56:02, johnme wrote:
> On 2017/05/15 14:42:15, mlamouri wrote:
> > What's the "..."
> 
> It's
https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Operators/Spre....
In this case, it's equivalent to:
> 
> var [x, y] = mediaControlsButtonCoordinates(video, "play-button");
> clickAtCoordinates(x, y);
> 
> or
> 
> var coords = mediaControlsButtonCoordinates(video, "play-button");
> clickAtCoordinates(coords[0], coords[1]);

TIL :)

https://codereview.chromium.org/2873493004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/video-enter-exit-fullscreen-while-hovering-shows-controls.html:87:
video.src = findMediaFile("video", "../content/test-25fps");
On 2017/05/15 at 14:56:02, johnme wrote:
> On 2017/05/15 14:42:15, mlamouri wrote:
> > Maybe you should use "../content/test"? The 25fps one is used only for one
test
> > that is related to seek and precision.
> 
> With ../content/test the video ends before the test is over (due to the 2x
runAfterHideMediaControlsTimerFired delays). The 25fps one is longer and doesn't
seem to cause problems.

Would content/counting work?

[johnme, 2017-05-16 13:42:49]

Description was changed from

==========
[Media Controls] Prevent flicker when enter/exit fullscreen

Previously, entering/exiting fullscreen whilst the media controls are
hidden due to inactivity would cause the media controls to briefly show
then almost immediately hide again, resulting in an ugly flicker.

They were shown by HTMLMediaElement::UpdateControlsVisibility (called
from HTMLMediaElement::Did{Enter,Exit}Fullscreen) and then hidden again
by MediaControlsImpl::OnTimeUpdate since ShouldHideMediaControls()
returns true.

(This was often masked by the fact that clicking the fullscreen button
with a mouse results in the mouse hovering over the media controls at
the start of the fullscreen transition, in which case it's ok to show
the media controls. It became particularly noticeable with the recent
video-rotate-to-fullscreen feature, which enters/exits fullscreen
without interacting with the media controls, but it can be reproduced
without that feature as well).

This patch renames MediaControls::Show to MaybeShow and makes it only
show the controls if they won't soon be hidden again.

BUG=717520
==========

to

==========
[Media Controls] Prevent flicker when enter/exit fullscreen

Previously, entering/exiting fullscreen whilst the media controls are
hidden due to inactivity would cause the media controls to briefly show
then almost immediately hide again, resulting in an ugly flicker.

They were shown by HTMLMediaElement::UpdateControlsVisibility (called
from HTMLMediaElement::Did{Enter,Exit}Fullscreen) and then hidden again
by MediaControlsImpl::OnTimeUpdate since ShouldHideMediaControls()
returns true.

(This was often masked by the fact that clicking the fullscreen button
with a mouse results in the mouse hovering over the media controls at
the start of the fullscreen transition, in which case it's ok to show
the media controls. It became particularly noticeable with the recent
video-rotate-to-fullscreen feature, which enters/exits fullscreen
without interacting with the media controls, but it can be reproduced
without that feature as well).

This patch renames MediaControls::Show to MaybeShow and makes it only
show the controls if they won't soon be hidden again.

BUG=717520
NOPRESUBMIT=true
==========

[johnme, 2017-05-16 13:42:56]

The CQ bit was checked by johnme@chromium.org

[johnme, 2017-05-16 13:42:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2873493004/#ps100001
(title: "Update SlowTests bug number")

[commit-bot: I haz the power, 2017-05-16 13:43:27]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[johnme, 2017-05-16 14:54:50]

The CQ bit was checked by johnme@chromium.org

[johnme, 2017-05-16 14:54:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2873493004/#ps120001
(title: "Add virtual/new-remote-playback-pipeline variants to SlowTests")

[commit-bot: I haz the power, 2017-05-16 14:55:24]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 19:28:39]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494946490772770,
"parent_rev": "f0a3d1dc467162b8c481fe146b86d9f938eedf20", "commit_rev":
"1fb662149d40ec4b4a19d984007528ea0b8798e3"}

[commit-bot: I haz the power, 2017-05-16 19:28:54]

Description was changed from

==========
[Media Controls] Prevent flicker when enter/exit fullscreen

Previously, entering/exiting fullscreen whilst the media controls are
hidden due to inactivity would cause the media controls to briefly show
then almost immediately hide again, resulting in an ugly flicker.

They were shown by HTMLMediaElement::UpdateControlsVisibility (called
from HTMLMediaElement::Did{Enter,Exit}Fullscreen) and then hidden again
by MediaControlsImpl::OnTimeUpdate since ShouldHideMediaControls()
returns true.

(This was often masked by the fact that clicking the fullscreen button
with a mouse results in the mouse hovering over the media controls at
the start of the fullscreen transition, in which case it's ok to show
the media controls. It became particularly noticeable with the recent
video-rotate-to-fullscreen feature, which enters/exits fullscreen
without interacting with the media controls, but it can be reproduced
without that feature as well).

This patch renames MediaControls::Show to MaybeShow and makes it only
show the controls if they won't soon be hidden again.

BUG=717520
NOPRESUBMIT=true
==========

to

==========
[Media Controls] Prevent flicker when enter/exit fullscreen

Previously, entering/exiting fullscreen whilst the media controls are
hidden due to inactivity would cause the media controls to briefly show
then almost immediately hide again, resulting in an ugly flicker.

They were shown by HTMLMediaElement::UpdateControlsVisibility (called
from HTMLMediaElement::Did{Enter,Exit}Fullscreen) and then hidden again
by MediaControlsImpl::OnTimeUpdate since ShouldHideMediaControls()
returns true.

(This was often masked by the fact that clicking the fullscreen button
with a mouse results in the mouse hovering over the media controls at
the start of the fullscreen transition, in which case it's ok to show
the media controls. It became particularly noticeable with the recent
video-rotate-to-fullscreen feature, which enters/exits fullscreen
without interacting with the media controls, but it can be reproduced
without that feature as well).

This patch renames MediaControls::Show to MaybeShow and makes it only
show the controls if they won't soon be hidden again.

BUG=717520
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2873493004
Cr-Commit-Position: refs/heads/master@{#472180}
Committed:
https://chromium.googlesource.com/chromium/src/+/1fb662149d40ec4b4a19d9840075...
==========

[commit-bot: I haz the power, 2017-05-16 19:28:56]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/1fb662149d40ec4b4a19d9840075...