Fix fullscreen video race condition.

Fix fullscreen video race condition.

A newly initialized media player must enter fullscreen if there is a
pending request sent by a previous media player.

BUG=338670
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247941

[Fredrik Öhrn, 2014-01-28 13:41:24]

The issue is a race condition when JavaScript issues a fullscreen request and
immediately sets the video source.

The full screen state is tracked by the RendererMediaPlayerManager, however it
only updates the state after a full IPC round trip to the UI.

In the mean time, setting the source will cause the HTML element to delete and
create a new WebMediaPlayerAndroid instance which will end up inline since the
mentioned IPC round trip has not completed yet.

This patch is the minimal change I could figure out to plug the race condition.

Being completely new to this code I found it to be surprisingly complicated. I
would have expected a source change to just update the existing media player
instance, alleviating lot of state tracking going on in several places.

[qinmin, 2014-01-28 17:41:33]

https://codereview.chromium.org/136113015/diff/1/content/renderer/media/andro...
File content/renderer/media/android/renderer_media_player_manager.cc (right):

https://codereview.chromium.org/136113015/diff/1/content/renderer/media/andro...
content/renderer/media/android/renderer_media_player_manager.cc:391:
pending_fullscreen_frame_ = NULL;
what if ExitFullscreen() is called before this call? Since
pending_fullscreen_frame_ is non-null, if another video element calls
RequestFullscreen(), it wouldn't be able to do so immediately after we fix
CanEnterFullscreen().

https://codereview.chromium.org/136113015/diff/1/content/renderer/media/andro...
content/renderer/media/android/renderer_media_player_manager.cc:403: bool
RendererMediaPlayerManager::ShouldEnterFullscreen(blink::WebFrame* frame) {
This logic is also needed for CanEnterFullscreen(), I think we should merge
these two functions.

[Fredrik Öhrn, 2014-01-29 15:51:33]

On 2014/01/28 17:41:33, qinmin wrote:
>
https://codereview.chromium.org/136113015/diff/1/content/renderer/media/andro...
> File content/renderer/media/android/renderer_media_player_manager.cc (right):
> 
>
https://codereview.chromium.org/136113015/diff/1/content/renderer/media/andro...
> content/renderer/media/android/renderer_media_player_manager.cc:391:
> pending_fullscreen_frame_ = NULL;
> what if ExitFullscreen() is called before this call? Since
> pending_fullscreen_frame_ is non-null, if another video element calls
> RequestFullscreen(), it wouldn't be able to do so immediately after we fix
> CanEnterFullscreen().
> 

I now set pending_fullscreen_frame_ to NULL in ExitFullscreen() to fix this.

>
https://codereview.chromium.org/136113015/diff/1/content/renderer/media/andro...
> content/renderer/media/android/renderer_media_player_manager.cc:403: bool
> RendererMediaPlayerManager::ShouldEnterFullscreen(blink::WebFrame* frame) {
> This logic is also needed for CanEnterFullscreen(), I think we should merge
> these two functions.

I'm unsure how a merged function would work, please elaborate.

In the mean time I've updated CanEnterFullscreen() to take
pending_fullscreen_frame_ into account.

[qinmin, 2014-01-29 23:38:07]

lgtm

[commit-bot: I haz the power, 2014-01-30 09:19:32]

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

[commit-bot: I haz the power, 2014-01-30 13:30:48]

Change committed as 247941