Keep overlay cast button visible long enough for user action

Keep overlay cast button visible long enough for user action

The overlay cast button should remain visible for some seconds
after the user taps the video, so that they can actually press it.
https://codereview.chromium.org/1156993013/ broke this (as described
in the bug). This reverts that small piece of
https://codereview.chromium.org/1156993013/

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

[aberent, 2015-09-15 18:55:59]

aberent@chromium.org changed reviewers:
+ philipj@opera.com

[liberato (no reviews please), 2015-09-15 21:22:42]

liberato@chromium.org changed reviewers:
+ liberato@chromium.org

[liberato (no reviews please), 2015-09-15 21:22:43]

lg*m.  thanks for fixing this.

-fl

[philipj_slow, 2015-09-16 09:22:52]

Is it possible to write a test for this bug so that it doesn't regress again?

[aberent, 2015-09-21 13:06:28]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-21 13:06:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1336133004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1336133004/20001

[aberent, 2015-09-21 13:12:26]

On 2015/09/16 09:22:52, philipj wrote:
> Is it possible to write a test for this bug so that it doesn't regress again?

Test added. I couldn't without a great deal of messing around add a test for
what happens when the video is clicked, since clicking the video on desktop
pauses it and hence prevents the bug, but the test I have added checks the
timeout when the video is first played, which should catch most regressions. I
have checked that this test fails on master and passes on the branch.

[philipj_slow, 2015-09-21 13:19:10]

The test looks good, but fairly likely to be flaky. Can you try running it a
couple thousand times under load to see how prone to flakiness it is?

https://codereview.chromium.org/1336133004/diff/20001/LayoutTests/media/contr...
File LayoutTests/media/controls-cast-overlay-slow-fade.html (right):

https://codereview.chromium.org/1336133004/diff/20001/LayoutTests/media/contr...
LayoutTests/media/controls-cast-overlay-slow-fade.html:28: setTimeout(function()
These also need to be wrapped in step_func. Note that you can use this.step_func
to avoid passing around a variable t/test, if you like that style better.

https://codereview.chromium.org/1336133004/diff/20001/LayoutTests/media/contr...
LayoutTests/media/controls-cast-overlay-slow-fade.html:53: test = t;
Each step needs to be wrapped in t.step_func, otherwise any failing assertions
will not know which test they belong to. In other words, no global test
variable.

https://codereview.chromium.org/1336133004/diff/20001/LayoutTests/media/contr...
LayoutTests/media/controls-cast-overlay-slow-fade.html:56:
video.addEventListener("playing", playing);
t.step_func(playing).

[aberent, 2015-09-21 14:59:20]

Not yet stress tested to the extent requested since (because of timeouts) each
test run takes a few seconds. Will do so overnight.

https://codereview.chromium.org/1336133004/diff/20001/LayoutTests/media/contr...
File LayoutTests/media/controls-cast-overlay-slow-fade.html (right):

https://codereview.chromium.org/1336133004/diff/20001/LayoutTests/media/contr...
LayoutTests/media/controls-cast-overlay-slow-fade.html:28: setTimeout(function()
On 2015/09/21 13:19:10, philipj wrote:
> These also need to be wrapped in step_func. Note that you can use
this.step_func
> to avoid passing around a variable t/test, if you like that style better.

Done. Making 'playing' an anonymous function, and using t throughout seems more
consistent with the other tests, so that is what I have done.

https://codereview.chromium.org/1336133004/diff/20001/LayoutTests/media/contr...
LayoutTests/media/controls-cast-overlay-slow-fade.html:53: test = t;
On 2015/09/21 13:19:10, philipj wrote:
> Each step needs to be wrapped in t.step_func, otherwise any failing assertions
> will not know which test they belong to. In other words, no global test
> variable.

Done.

https://codereview.chromium.org/1336133004/diff/20001/LayoutTests/media/contr...
LayoutTests/media/controls-cast-overlay-slow-fade.html:56:
video.addEventListener("playing", playing);
On 2015/09/21 13:19:10, philipj wrote:
> t.step_func(playing).

Done.

[philipj_slow, 2015-09-21 15:05:13]

LGTM

I didn't consider how long it would take to test a couple thousand of
iterations, that was a bit excessive, but if it's already running I guess just
let it keep going for a bit until it doesn't seem plausible that there's a
problem to be found.

https://codereview.chromium.org/1336133004/diff/20001/LayoutTests/media/contr...
File LayoutTests/media/controls-cast-overlay-slow-fade.html (right):

https://codereview.chromium.org/1336133004/diff/20001/LayoutTests/media/contr...
LayoutTests/media/controls-cast-overlay-slow-fade.html:28: setTimeout(function()
On 2015/09/21 14:59:20, aberent wrote:
> On 2015/09/21 13:19:10, philipj wrote:
> > These also need to be wrapped in step_func. Note that you can use
> this.step_func
> > to avoid passing around a variable t/test, if you like that style better.
> 
> Done. Making 'playing' an anonymous function, and using t throughout seems
more
> consistent with the other tests, so that is what I have done.

Yep, that looks good.

[aberent, 2015-09-21 15:27:23]

On 2015/09/21 15:05:13, philipj wrote:
> LGTM
> 
> I didn't consider how long it would take to test a couple thousand of
> iterations, that was a bit excessive, but if it's already running I guess just
> let it keep going for a bit until it doesn't seem plausible that there's a
> problem to be found.

Okay, ran 200 runs with all processors running at 100% CPU (using the Linux
stress utility) with no failures.

> 
>
https://codereview.chromium.org/1336133004/diff/20001/LayoutTests/media/contr...
> File LayoutTests/media/controls-cast-overlay-slow-fade.html (right):
> 
>
https://codereview.chromium.org/1336133004/diff/20001/LayoutTests/media/contr...
> LayoutTests/media/controls-cast-overlay-slow-fade.html:28:
setTimeout(function()
> On 2015/09/21 14:59:20, aberent wrote:
> > On 2015/09/21 13:19:10, philipj wrote:
> > > These also need to be wrapped in step_func. Note that you can use
> > this.step_func
> > > to avoid passing around a variable t/test, if you like that style better.
> > 
> > Done. Making 'playing' an anonymous function, and using t throughout seems
> more
> > consistent with the other tests, so that is what I have done.
> 
> Yep, that looks good.

[aberent, 2015-09-21 15:27:41]

The CQ bit was checked by aberent@chromium.org

[commit-bot: I haz the power, 2015-09-21 15:27:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1336133004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1336133004/40001

[commit-bot: I haz the power, 2015-09-21 18:38:20]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202592