[Cast] Disable autohide of media controls for remote playback

[Cast] Disable autohide of media controls for remote playback

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

[whywhat, 2014-10-13 13:44:29]

avayvod@chromium.org changed reviewers:
+ aberent@chromium.org

[whywhat, 2014-10-13 13:44:29]

PTaL

[whywhat, 2014-10-13 13:55:07]

avayvod@chromium.org changed reviewers:
+ mkwst@chromium.org

[aberent, 2014-10-13 13:56:18]

lgtm

[Mike West, 2014-10-13 14:01:41]

Are there any tests for this functionality, remote-playback-related or local?

-mike

[whywhat, 2014-10-13 14:21:51]

On 2014/10/13 14:01:41, Mike West wrote:
> Are there any tests for this functionality, remote-playback-related or local?
> 
> -mike

There's a cast related test and a few local auto-fade related tests. I'll look
into adding a test for my change (will require some additional code exposed via
window.internals)

[whywhat, 2014-10-13 16:28:43]

PTAL

[Peter Beverloo, 2014-10-13 16:37:23]

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

[Peter Beverloo, 2014-10-13 16:37:24]

Please remove "Android" from the title since this patch doesn't contain any
Android-specific code. Platform support is the embedder's concern.

https://codereview.chromium.org/650893002/diff/250001/LayoutTests/media/contr...
File LayoutTests/media/controls-cast-do-not-fade-out.html (right):

https://codereview.chromium.org/650893002/diff/250001/LayoutTests/media/contr...
LayoutTests/media/controls-cast-do-not-fade-out.html:51:
video.addEventListener("loadedmetadata", loadedmetadata);
Can we just bind to the |playing| event here, call video.play() and skip the
loadedmetadata step? Why is that important here?

https://codereview.chromium.org/650893002/diff/250001/Source/core/testing/Int...
File Source/core/testing/Internals.h (right):

https://codereview.chromium.org/650893002/diff/250001/Source/core/testing/Int...
Source/core/testing/Internals.h:257: void
mediaPlayerPlayingRemotelyChanged(HTMLMediaElement *, bool);
nit: no space before the *. you should fix line 256 as well.

[whywhat, 2014-10-13 16:56:28]

https://codereview.chromium.org/650893002/diff/250001/LayoutTests/media/contr...
File LayoutTests/media/controls-cast-do-not-fade-out.html (right):

https://codereview.chromium.org/650893002/diff/250001/LayoutTests/media/contr...
LayoutTests/media/controls-cast-do-not-fade-out.html:51:
video.addEventListener("loadedmetadata", loadedmetadata);
On 2014/10/13 16:37:24, Peter Beverloo wrote:
> Can we just bind to the |playing| event here, call video.play() and skip the
> loadedmetadata step? Why is that important here?

Done.

https://codereview.chromium.org/650893002/diff/250001/Source/core/testing/Int...
File Source/core/testing/Internals.h (right):

https://codereview.chromium.org/650893002/diff/250001/Source/core/testing/Int...
Source/core/testing/Internals.h:257: void
mediaPlayerPlayingRemotelyChanged(HTMLMediaElement *, bool);
On 2014/10/13 16:37:24, Peter Beverloo wrote:
> nit: no space before the *. you should fix line 256 as well.

Done.

[Mike West, 2014-10-14 03:55:17]

LGTM % one comment, thanks for the test.

https://codereview.chromium.org/650893002/diff/500001/LayoutTests/SlowTests
File LayoutTests/SlowTests (right):

https://codereview.chromium.org/650893002/diff/500001/LayoutTests/SlowTests#n...
LayoutTests/SlowTests:52: crbug.com/24182
media/controls-cast-do-not-fade-out.html [ Slow ]
How slow is this test? It's unfortunate that we have to add another item to
SlowTests. :(

It's also surprising that _all_ the tests here point to the same bug. If the
media tests all have the same root cause of slowness, perhaps it makes sense to
tie them together with a separate bug.

[mlamouri (slow - plz ping), 2014-10-14 15:37:36]

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

[mlamouri (slow - plz ping), 2014-10-14 15:37:37]

I have a few questions related to the test:
- why not using a proper test-harness check instead of console.write()?
- the setTimeout() you use in runAfterHideMediaControlsTimerFired scares me out.
This is going to make the test flaky but also makes it very slow. Do we really
care about keeping the same delay in tests? Can't we use 0 instead of whatever
constant when running a layout test so we can have faster tests and also more
reliable? (ie. going trough the event loop once instead of being delayed for x
amount of time)
An alternative would be to poll for the change instead of rely on a timeout. It
would be as slow but definitely less flaky.

[whywhat, 2014-10-14 19:30:57]

https://codereview.chromium.org/650893002/diff/500001/LayoutTests/SlowTests
File LayoutTests/SlowTests (right):

https://codereview.chromium.org/650893002/diff/500001/LayoutTests/SlowTests#n...
LayoutTests/SlowTests:52: crbug.com/24182
media/controls-cast-do-not-fade-out.html [ Slow ]
On 2014/10/14 03:55:17, Mike West wrote:
> How slow is this test? It's unfortunate that we have to add another item to
> SlowTests. :(

It's slightly longer than the default 6 seconds timeout. One has to wait for
about 3.5 seconds until the first check and then the same amount of time until
the second check.

> 
> It's also surprising that _all_ the tests here point to the same bug. If the
> media tests all have the same root cause of slowness, perhaps it makes sense
to
> tie them together with a separate bug.

AFAIK, the media tests have different cause of slowness. No idea why all these
exceptions point to the same bug.

[whywhat, 2014-10-14 19:34:12]

On 2014/10/14 15:37:37, Mounir Lamouri wrote:
> I have a few questions related to the test:
> - why not using a proper test-harness check instead of console.write()?

No idea, I based my test on the existing one for audio element.

> - the setTimeout() you use in runAfterHideMediaControlsTimerFired scares me
out.
> This is going to make the test flaky but also makes it very slow. Do we really
> care about keeping the same delay in tests? Can't we use 0 instead of whatever
> constant when running a layout test so we can have faster tests and also more
> reliable? (ie. going trough the event loop once instead of being delayed for x
> amount of time)

I guess I could see if I can easily override the timeout for the auto-hide in
the test.

> An alternative would be to poll for the change instead of rely on a timeout.
It
> would be as slow but definitely less flaky.

Would polling for a few times after the timeout is over be better?

I'm just using the existing method that seems to be stable enough for another
similar test.

[whywhat, 2014-10-14 19:34:46]

The CQ bit was checked by avayvod@chromium.org

[commit-bot: I haz the power, 2014-10-14 19:35:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/650893002/500001

[commit-bot: I haz the power, 2014-10-14 22:26:50]

Committed patchset #5 (id:500001) as 183701